Initial support for CRDs (upgrade) policies #250
Conversation
alex-berger
force-pushed
the
feature/crd-upgrade
branch
from
e4d1931 to
d54ac8b
Compare
|
Unfortunately, I run out of ideas how to make the e2e test working with pull request. E2e tests work on my feature branches which are real branches that can be referenced by the |
alex-berger
force-pushed
the
feature/crd-upgrade
branch
3 times, most recently
from
7dac7b8 to
e32e706
Compare
As mentioned above the test run on branches and tags, but unfortunately not on pull-requests. See my feature branch (the source branch of the PR at https://github.com/alex-berger/helm-controller/actions for test results). I have not yet figured out whether there is a way to reference a GitHub pull-request as branch (I wasted 6 hours trying to find a way, but did not find one yet). As my time is limited I have given up and disabled the (new) tests for pull-requests, such that they only run on branches (and maybe tags). |
Signed-off-by: Alexander Berger <alex-berger@gmx.ch>
Signed-off-by: Alexander Berger <alex-berger@gmx.ch>
Signed-off-by: Alexander Berger <alex-berger@gmx.ch>
alex-berger
force-pushed
the
feature/crd-upgrade
branch
from
31b92ca to
4b60855
Compare
There was a problem hiding this comment.
I am OK with the e2e approach for now, we should move many of the tests to Go anyway, as has been done with a lot of kustomize-controller.
Anyway, thanks a lot @alex-berger. 💯 🌻
| @@ -58,7 +70,7 @@ type Runner struct { | |||
| // namespace configured to the provided values. | |||
| func NewRunner(getter genericclioptions.RESTClientGetter, storageNamespace string, logger logr.Logger) (*Runner, error) { | |||
| runner := &Runner{ | |||
| logBuffer: NewLogBuffer(NewDebugLog(logger).Log, 0), | |||
| logBuffer: NewLogBuffer(NewDebugLog(logger).Log, 100), | |||
There was a problem hiding this comment.
I somehow missed this during review, and this change is not related to any of the CRD things. Can you explain why this was changed? As it changes the default of including the last 5 log lines (after de-duplication) in a failure condition and/or event to 100, which makes any failed event we produce now quite verbose.
There was a problem hiding this comment.
I increased it for debugging during feature development and eventually forgot to decrease it again. 😳
There was a problem hiding this comment.
Thanks, that means my restore PR is justified and I can continue with the release :-)
hiddeco
changed the title
|
Looks like I'm moments late to comment on this, but has the justifications for helm, itself, not supporting this been considered? https://github.com/helm/community/blob/main/hips/hip-0011.md |
|
They have been considered, which is why we default to not enable the behavior as it can be potentially disastrous. However, given that people use the helm-controller to automatically drive operations, having access to a configuration option that does the right thing if you know how your chart (or the application behind it) behaves on Custom Resource Definition upgrades, it is in my opinion justified. |
|
This feature would need to be able to add the conversion webhooks simultaneously. Links to them are defined in the CRD itself: see the cert-manager CRDs for example. Without them, the API server will reject create/change verbs to those CRDs. |
This PR adds preliminary support for Upgrading CRDs which are part of Helm Charts managed via
HelmReleaseobjects.See also fluxcd/flux2#1071 for more information.
Background & Motivation
Helm still does not provide any built-in solution to the CRD upgrade problem, which is well-known and
documented:
This limitation of Helm makes GitOps style "day 2 operations" of Kubernetes resources managed by
HelmReleaseobjects very difficult if not to say impossible. Currently, we have to manuallyupgrade CRDs from
HelmChartsreferenced byHelmReleases, which is cumbersome and needs verytight (timing) coordination between the commit to the GitOps repository and the manual CRD
upgrade on all systems that observe/apply that commit. This apporach is not only very error
prone it also might cause unnecessary long downtime of services.
Note, most of the Helm Charts that we install and operate are created and maintained by
3rd parties and are not under our control. Extracting all CRDs from every Helm Chart
upon each new release and manually applying those CRD resources is, as mentioned above,
very time intensive, error prone and cumbersome.
Our observation is, that most CRD upgrades are non-critical and only evolve an existing CRD
in a backward compatible fashion. Therefore, I am wondering whether it would make sense to
extends the
HelmReleaseresources with an opt-in flag to automatically upgrade CRD objects(if needed).