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

commonLabels need to be immutable for upgrades - remove version from commonLabels #1131

Closed
jlewi opened this issue Apr 22, 2020 · 1 comment · Fixed by #1140
Closed

commonLabels need to be immutable for upgrades - remove version from commonLabels #1131

jlewi opened this issue Apr 22, 2020 · 1 comment · Fixed by #1140

Comments

@jlewi
Copy link
Contributor

jlewi commented Apr 22, 2020

The commonLabels in our kustomize package currently include the application version.
This breaks upgradability because commonLabels are substituted in selectors which are immutable.

This has been raised in a number of issues

As noted in kubeflow/kubeflow#4873; kustomize commonLabels should only be used for immutable labels
https://kubectl.docs.kubernetes.io/pages/app_management/labels_and_annotations.html

kubeflow/kfctl#304 describes current thinking for how to do upgrades which should make version labels unnecessary.

We should think about unittests or other forms of validation to ensure all manifests adhere to this.

@issue-label-bot
Copy link

Issue Label Bot is not confident enough to auto-label this issue.
See dashboard for more details.

jlewi pushed a commit to jlewi/manifests that referenced this issue Apr 24, 2020
* To fix the race condition with certmanager (kubeflow#1125) we move the KF
  issuer into a separate package from the package deploying kubeflow.
  This way we can wait for cert-manager to start before deploying resources.

* Labels need to be immutable otherwise upgrades won't work (see kubeflow#1131).
  So remove version number from common labels and application selector
  so that apply will work to update resources.
jlewi pushed a commit to jlewi/testing that referenced this issue Apr 29, 2020
* See kubeflow/manifests#1131  we want commonLabels to be immutable
  otherwise upgrades won't work because we end up trying to change
  selectors which are immutable.

* The applications.py script inherently violates that because it was
  mutating commonLabels on every release. We don't want to do that
  anymore so we delete the script.
k8s-ci-robot pushed a commit that referenced this issue Apr 30, 2020
* To fix the race condition with certmanager (#1125) we move the KF
  issuer into a separate package from the package deploying kubeflow.
  This way we can wait for cert-manager to start before deploying resources.

* Labels need to be immutable otherwise upgrades won't work (see #1131).
  So remove version number from common labels and application selector
  so that apply will work to update resources.
k8s-ci-robot pushed a commit to kubeflow/testing that referenced this issue May 1, 2020
* See kubeflow/manifests#1131  we want commonLabels to be immutable
  otherwise upgrades won't work because we end up trying to change
  selectors which are immutable.

* The applications.py script inherently violates that because it was
  mutating commonLabels on every release. We don't want to do that
  anymore so we delete the script.
jlewi pushed a commit to jlewi/manifests that referenced this issue May 1, 2020
* Fix kubeflow#1131

* kustomize commonLabels get subsituted into selector fields. Selector fields
  are immutable. So if commonLabels change (e.g. between versions) then
  we can't reapply/update the existing resources which breaks upgrades
 (kubeflow/kfctl#304)

* For the most part the problematic commonLabels were on our Application
  resources. The following labels were being set

  "app.kubernetes.io/version"
  "app.kubernetes.io/instance"
  "app.kubernetes.io/managed-by"
  "app.kubernetes.io/part-of"

* Version was definetely changing between versions. instance was also changing
  between versions to include the version number.

* managed-by and part-of could also change (e.g. we may not be using kfctl)
* We could still set these labels if we wanted to; we just shouldn't set
  them as commonLabels and/or include them in the selector as the will
  inhibit upgrades with kubectl apply.

* I created a test validate_resources_test.go to ensure none of these
  labels are included in commonLabels

* I created a simple go binary tools/fix_common_labels.go to update
  all the resources.

* generat_tests.py - Delete the code to remove unmatched tests.
  * We no longer generate tests that way and the delete code was going
    to delete valid tests like our new validation test

* Get rid of the clean rule in the Makefile for the same reason.
k8s-ci-robot pushed a commit that referenced this issue May 1, 2020
* Fix #1131

* kustomize commonLabels get subsituted into selector fields. Selector fields
  are immutable. So if commonLabels change (e.g. between versions) then
  we can't reapply/update the existing resources which breaks upgrades
 (kubeflow/kfctl#304)

* For the most part the problematic commonLabels were on our Application
  resources. The following labels were being set

  "app.kubernetes.io/version"
  "app.kubernetes.io/instance"
  "app.kubernetes.io/managed-by"
  "app.kubernetes.io/part-of"

* Version was definetely changing between versions. instance was also changing
  between versions to include the version number.

* managed-by and part-of could also change (e.g. we may not be using kfctl)
* We could still set these labels if we wanted to; we just shouldn't set
  them as commonLabels and/or include them in the selector as the will
  inhibit upgrades with kubectl apply.

* I created a test validate_resources_test.go to ensure none of these
  labels are included in commonLabels

* I created a simple go binary tools/fix_common_labels.go to update
  all the resources.

* generat_tests.py - Delete the code to remove unmatched tests.
  * We no longer generate tests that way and the delete code was going
    to delete valid tests like our new validation test

* Get rid of the clean rule in the Makefile for the same reason.
@Bobgy Bobgy mentioned this issue Nov 16, 2020
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant