Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Stop creating immutable resource label that breaks "tutor k8s" on Tutor version changes #533

Merged

Conversation

fghaas
Copy link
Contributor

@fghaas fghaas commented Nov 22, 2021

Through the commonLabels directive in kustomization.yml, all resources get a label named "app.kubernetes.io/version", which is being set to the Tutor version at the time of initial deployment.

When the user then subsequently progresses to a new Tutor version, Kubernetes attempts to update this label — but for Deployment, ReplicaSet, and DaemonSet resources, this is no longer allowed as of kubernetes/kubernetes#50808. This causes tutor k8s start (at the kubectl apply --kustomize step) to break with errors such as:

Deployment.apps "redis" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/instance":"openedx-JIONBLbtByCGUYgHgr4tDWu1", "app.kubernetes.io/managed-by":"tutor", "app.kubernetes.io/name":"redis", "app.kubernetes.io/part-of":"openedx", "app.kubernetes.io/version":"12.1.7"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

Simply removing the app.kubernetes.io/version label from commonLabels in kustomization.yml will permanently fix this issue for newly created Kubernetes deployments, which will "survive" any future Tutor version changes thereafter.

However, existing production Open edX deployments will need to throw the affected Deployments away, and re-create them.

Also, add the Tutor version as a resource annotation instead, using the commonAnnotations directive.

See also:
kubernetes/client-go#508
https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/commonlabels/
https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/commonannotations/

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that your testing should be sufficient. Could you please just add a mention of a breaking change to CHANGELOG-nightly.md?

…n Tutor version changes

Through the commonLabels directive in kustomization.yml, all resources
get a label named "app.kubernetes.io/version", which is being set to
the Tutor version at the time of initial deployment.

When the user then subsequently progresses to a new Tutor version,
Kubernetes attempts to update this label — but for Deployment,
ReplicaSet, and DaemonSet resources, this is no longer allowed as of
kubernetes/kubernetes#50808. This causes
"tutor k8s start" (at the "kubectl apply --kustomize" step) to break
with errors such as:

Deployment.apps "redis" is invalid: spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"app.kubernetes.io/instance":"openedx-JIONBLbtByCGUYgHgr4tDWu1", "app.kubernetes.io/managed-by":"tutor", "app.kubernetes.io/name":"redis", "app.kubernetes.io/part-of":"openedx", "app.kubernetes.io/version":"12.1.7"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable

Simply removing the app.kubernetes.io/version label from
kustomization.yml will permanently fix this issue for newly created
Kubernetes deployments, which will "survive" any future Tutor version
changes thereafter.

However, *existing* production Open edX deployments will need to throw
the affected Deployments away, and re-create them.

Also, add the Tutor version as a resource annotation instead, using
the commonAnnotations directive.

See also:
kubernetes/client-go#508
https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/commonlabels/
https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/commonannotations/

Fixes overhangio#531.
@fghaas fghaas force-pushed the version-label-to-annotation-nightly branch from 1958d9b to b0f80fd Compare November 23, 2021 09:31
@fghaas
Copy link
Contributor Author

fghaas commented Nov 23, 2021

Could you please just add a mention of a breaking change to CHANGELOG-nightly.md?

Done, can you take a look to see whether you judge that that sufficiently describes the change?

@@ -2,6 +2,7 @@

Note: Breaking changes between versions are indicated by "💥".

- 💥[Bugfix] No longer track the Tutor version number in resource labels (and label selectors, which breaks the update of Deployment resources), but instead do so in resource annotations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule of thumb, in the changelog I prefer when we describe what we are fixing instead of how. The former is of more interest to the reader. Thus, a better changelog would be something along the lines of "Fix upgrading Kubernetes deployments when upgrading from one minor tutor version to the next".
But I don't want to block your PR for a changelog line, so let's merge this :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but in this case your abbreviated version doesn't contain any explanation as to why this is a breaking change. (Why should a fix to upgrades be a breaking change?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. The changelog should also explain that the deployments should be deleted prior to upgrading.

@regisb regisb merged commit 5558257 into overhangio:nightly Nov 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants