Best practices for updating a CRD's metadata

[dprotaso]

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

The code below is problematic as it may trigger a generation bump

func (c *Controller) updateLabel(serviceName, namespace string) (*v1alpha1.Service, error) {
	serviceClient := c.KnativeClientSet.ServingV1alpha1().Services(service.Namespace)
        service, _ := serviceClient.Get(service.Name, metav1.GetOptions{})
	service.ObjectMetadata.Labels[key] = someValue
	return serviceClient.Update(service)
}

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.

Few approaches to resolve this

1. (maybe not possible) update the CRD definitions to make sure marshalling always produces the same output. ie. When creating a CRD via kubectl, if a user omits a field in the spec of a CRD, when a controller marshals that CRD it should also omit that field. We shouldn't see null or empty fields from the controller
2. (easiest) use the clientset's Patch call to only update the label

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

[mattmoor]

@dprotaso @bsnchan We need this when 1.11 lands, but we could adopt this now, correct?

[mattmoor]

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.

How do we avoid this today? Is the webhook only called for edits to spec?

[grantr]

Is the webhook only called for edits to spec?

Today the configuration controller uses Spec.Generation to detect when a configuration has changed, ignoring Metadata.Generation. The webhook only updates Spec.Generation when Spec has changed:

serving/pkg/webhook/webhook.go

Line 540 in e1b3aae

// if there's an old object and spec has changed, set generation to oldGeneration + 1

[mattmoor]

@dprotaso is the only place that we do this today the Route controller's labelling of Configurations? (note: this previously included Revisions, but @mdemirhan removed it).

To repeat my earlier question can we tackle this even before we transition to 1.11?

[dprotaso]

is the only place that we do this today the Route controller's labelling of Configurations?

Yup - just the route controller sets labels

To repeat my earlier question can we tackle this even before we transition to 1.11?

Yes - I think we can use the config's clientset's Patch. I'll take it on

/assign @dprotaso