-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enable default apps from cluster chart #262
Conversation
Note As this is a draft PR no triggers from the PR body will be handled. If you'd like to trigger them while draft please add them as a PR comment. |
aa31744
to
4f94b59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to my eyes. However, I'd like to wait for @glitchcrab to review since he has been working a lot on this chart recently.
This is basically what we have already done in CAPA and CAPZ. On other providers it had worked without issues. Here I see only 1 potential issue now, which is around vertical-pod-autoscaler-crd. It used to be installed from default-apps-vsphere as an App CR. Now it's installed from cluster-vsphere (via cluster chart dependency), so what now happens:
|
It should be possible to fix this manually, by pausing vertical-pod-autoscaler-crd Chart CR and deleting it. Then removing the leftover chart-operator finalizer. This way vertical-pod-autoscaler-crd Chart CR would get removed, without chart-operator trying to delete VPA CustomResourceDefinition). It might be fine to just fix this manually, WDYT? |
It looks like that after the migration the VPA CustomResourceDefinition is applied by Flux, meaning it's (corretly) coming from vertical-pod-autoscaler-crd HelmRelease from cluster chart. VPA CustomResourceDefinition has these labels after the migration (meaning it has been correctly migrated to be installed from cluster-vsphere):
So vertical-pod-autoscaler-crd Chart CR in the WC is a leftover. Only danger here is that chart-operator would periodically reconcile this Chart CR, so Flux and chart-operator would potentially periodically fight here. Unless somebody yells objects here, I will just leave this as is, and add instructions to README on how to fix this manually after the cluster has been migrated. |
Tested this, seems to be working. |
7c7d2bd
to
29da345
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with my limited understanding, this makes sense
be8d5bd
to
a0b3767
Compare
Testing this with e2e tests locally, with local e2e tests change (basically this giantswarm/clustertest#283, just with different version numbers as we still haven't released cluster-vsphere with this change). And found an issue - cilium HelmRelease was still in the chart 🤦 not sure if I forgot to remove it, or rebasing put it back somehow (probably the former 😬) |
This global value is defined in cluster chart, but we need it in cluster-vsphere as well
a0b3767
to
3b5c1b0
Compare
I ran CAPV Standard suite locally (with modified e2e tests to support cluster-vsphere without default-apps-vsphere) and the tests passed ✅ 🎉 Didn't run CAPV Upgrade suite, since the upgrade requires manual steps and cannot work in e2e tests. CAPV Standard suite output
|
There were differences in the rendered Helm template, please check! Output
|
CAPV on CAPZ suite passed locally ✅ 🎉 CAPV on CAPZ suite output
|
Towards giantswarm/roadmap#3125
Warning
Cluster upgrade to a version with these changes will require manual upgrade steps.
Changes in this PR
Render default apps from cluster-vsphere.
Changed
Added
Removed
The apps are migrated from default-apps-vsphere to cluster-vsphere in the following way:
.Values.deleteOptions.moveAppsHelmOwnershipToClusterVSphere
to true.app-operator.giantswarm.io/paused: true
annotation, so wait few minutes for the change to get applied by the Helm post-upgrade hook.Trigger e2e tests
/run cluster-test-suites