Best practices for updating a CRD's spec

[dprotaso]

Once we transition to k8s 1.11 and merge the subresources PR #786 we need some best practices around updating the CRD spec.

The code below is problematic as it may trigger a generation bump when it doesn't need to

func (c *Controller) updateService(service *v1alpha1.Service) (*v1alpha1.Service, error) {
	serviceClient := c.ElaClientSet.ServingV1alpha1().Services(service.Namespace)
	existing, _ := serviceClient.Get(service.Name, metav1.GetOptions{})

	// Check if there is anything to update.
	if !reflect.DeepEqual(existing.Spec, service.Spec) {
		return serviceClient.Update(existing)
	}
	return existing, nil
}

Triggering a generation bump becomes inherently expensive for certain CRDs. For a Configuration this will trigger a new revision and it's subsequent pods, services etc. to be created.

Whether a generation bump occurs will depend on the CRD definition. In our case it's very likely as we have nested kubernetes core types which although maybe be semantically equivalent will not be considered equal when using reflect.DeepEqual

An approach to resolve this is to use k8s apimachinery pkg/api/equality for comparison when updating our CRD's spec

A second approach is to utilize the clientset's patch

Related:
Best practices for updating [CRD] status (#1107)
Best practices for updating a CRD's metadata (#1127)

[grantr]

Currently the webhook uses jsonpatch to compare specs and determine equality:

serving/pkg/webhook/webhook.go

Lines 573 to 578 in e1b3aae

	specPatches, err := jsonpatch.CreatePatch(oldSpecJSON, newSpecJSON)
	if err != nil {
	fmt.Printf("Error creating JSON patch:%v", err)
	return err
	}
	if len(specPatches) > 0 {

[mattmoor]

@dprotaso Where was your example from?

The only places I would expect one of our controllers to update the Spec of one of our CRDs is the Service controller reconciling itself with Route and Configuration. We also now widely use equality.Semantic.DeepEqual, although I haven't triaged what might remain (we should).

For me, the "top of mind" remaining thing here isn't CRD specific: How do we avoid fighting with Spec defaulting done by another controller? This is the main reason we aren't reconciling changes to the Deployments we create in the Revision controller today. :(

[dprotaso]

Where was your example from?

I most likely tweaked the service's updateStatus for illustrative purposes

serving/pkg/controller/service/service.go

Lines 204 to 217 in 804f5ee

	func (c *Controller) updateStatus(service *v1alpha1.Service) (*v1alpha1.Service, error) {
	existing, err := c.serviceLister.Services(service.Namespace).Get(service.Name)
	if err != nil {
	return nil, err
	}
	// Check if there is anything to update.
	if !reflect.DeepEqual(existing.Status, service.Status) {
	existing.Status = service.Status
	serviceClient := c.ServingClientSet.ServingV1alpha1().Services(service.Namespace)
	// TODO: for CRD there's no updatestatus, so use normal update.
	return serviceClient.Update(existing)
	}
	return existing, nil
	}

This becomes a non-issue once we start using the clientset's UpdateStatus against a 1.11 cluster and subresources enabled on our CRDs

How do we avoid fighting with Spec defaulting done by another controller? This is the main reason we aren't reconciling changes to the Deployments we create in the Revision controller today. :(

@grantr and I had a discussion here that might be relevant https://github.com/grantr/serving/pull/3#pullrequestreview-129649072

A thought could be reconciling updates to a Deployment explicitly using a clientset's Patch operation.

[dprotaso]

Also we could potentially use a Deployment defaulter function when creating our deployment

https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/apps/v1/zz_generated.defaults.go#L191

[mattmoor]

@dprotaso do you want to tackle any follow ups here as part of the 1.11 sub-resource scope?

[dprotaso]

@mattmoor @mattmoor-sockpuppet I think it's a requirement.

The only thing remaining wrt to this issue is to use UpdateStatus during reconciliation.

[mattmoor]

/assign @dprotaso

[mattmoor]

Per above, I think what's left here is tracked by: #643 so closing to dedupe.