-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
[kube-prometheus-stack] Use kubectl replace to upgrade prometheuses CRD #1510
[kube-prometheus-stack] Use kubectl replace to upgrade prometheuses CRD #1510
Conversation
f1cc4c5
to
3b79239
Compare
@@ -5,6 +5,7 @@ apiVersion: apiextensions.k8s.io/v1 | |||
kind: CustomResourceDefinition | |||
metadata: | |||
annotations: | |||
argocd.argoproj.io/sync-options: Replace=true |
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.
i dont think we should add deployment specific annotations to the crd.
imho the change in the docs for manual upgrade should be enough.
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.
@monotek The problem here, is that CRDs are not templated. There is no way to add annotations except hacky workarounds.
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.
@sathieu Great idea to add this directly to the CRD; it solves the problem for ArgoCD users and the annotation will simply be ignored for everyone else. The "manual upgrade" isn't really a viable option when using ArgoCD; every sync attempt will fail because the CRD is too large. Without this change, the only way for an ArgoCD user to fix this is to replace everything on each sync operation (risky) or to extract the CRD from the chart so it can be updated locally.
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.
As helm is not supposed to update crds anyway, most people add and manage these crds "by hand" or via some other automated step, outside of helm..
Therefore the crd does not need such annotations.
In the long term this would make the CRD also kind of messy, if everybody starts to add things for his special deployment method, which in the end are not realy needed.
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.
Guys, we have to pay attention to the way the CRDs are updated in this chart. It's not thru manual changes like the above but thru an automated process that pulls them from the Prometheus Operator repo. So this change here is even pointless because it will get overwritten upon the next major update of the CRDs.
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.
@steinarox Ah, apologies - didn't realize we were updating the CRDs after every pull from the Prometheus Operator repo thru the script. You are right.
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.
And one of the reasons I didn't realize that was because that script was not meant to modify the CRDs.
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.
Maybe the other maintainers might have another take on the matter but I'm with @monotek on this one. The chart is not meant to create new opinionated versions of the used external code.
Having said that - @sathieu how about adding this functionality in a completely optional way. That is to say - adding another section in the README.md
about tools like ArgoCD, Flux, etc. - and state what particular changes are needed to make the CRDs fully compatible with those third-party tools that might operate in a non-standard way. Maybe even putting another small script that does the changes upon manual execution?
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.
@Xtigyro There is no easy way to workaround this on the ArgoCD side.
Possible correct solutions:
- Implement server-side apply in ArgoCD Option to perform K8s v1.16 server-side apply argoproj/argo-cd#2267
- Support helm post renderers in ArgoCD Support helm post renderers argoproj/argo-cd#3698
Heavy workaround:
I won't work more on this as I have a workaround (thanks to Ansible, see https://gitlab.com/kubitus-project/kubitus-installer/-/issues/70). But this workaround is clearly not easily actionable to everyone.
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.
As helm is not supposed to update crds anyway, most people add and manage these crds "by hand" or via some other automated step, outside of helm..
What examples exist that this is how "most people" managed CRDs? The kube-prometheus-stack Helm chart includes the CRDs and applying/creating CRDs through Helm/ArgoCD is common practice and works.
Great idea to add this directly to the CRD; it solves the problem for ArgoCD users and the annotation will simply be ignored for everyone else.
Agreed; this is a simple and harmless 1-line annotation that solves what is otherwise a huge pain for administrators, and does not affect those not using ArgoCD. Is it really worth it to take a philosophical stance against "add[ing] deployment specific annotations to the crd"?
how about adding this functionality in a completely optional way
It's already been pointed out in multiple threads why hackishly going in and editing or sed
'ing a CRD YAML is inarguably suboptimal to the simple change that is being proposed here.
@@ -5,6 +5,7 @@ apiVersion: apiextensions.k8s.io/v1 | |||
kind: CustomResourceDefinition | |||
metadata: | |||
annotations: | |||
argocd.argoproj.io/sync-options: Replace=true |
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.
As helm is not supposed to update crds anyway, most people add and manage these crds "by hand" or via some other automated step, outside of helm..
Therefore the crd does not need such annotations.
In the long term this would make the CRD also kind of messy, if everybody starts to add things for his special deployment method, which in the end are not realy needed.
@monotek While I understand the general rule to avoid using tool-specific annotations, the current situation is broken. Expanding the topic, the crds upgrade handling is currently a manual step, this could be improved. A few charts handling this correctly:
Some other charts use a subchart (like here) but requires extra manual steps. If we want to implement a solution like gatekeeper and velero, this PR is still needed as crds don't allow templating. If we want to implement the subchart solution, we'll have problem as kube-prometheus-stack uses the CRDs it creates (and we'll also have the upgrade problems). |
I also just hit this issue and support this change. |
Would be good to separate the changes between the README change and then new argoCD addition so that the README can go up and users be aware of the new larger CRD from prometheus operator. Is that team aware of the larger CRD and have plans to shrink it? |
I guess one of the chart maintainers should decide how to proceed. |
3b79239
to
9019841
Compare
@monotek What can we do to move this PR forward? It has fixes for upgrade with kubectl, and install/upgrade with ArgoCD. Other methods will need other fixes (using kustomize, like cloudnativedaysjp/dreamkast-infra#1262, or anything else), but at least this PR improves things. While I understand your point about limiting tool-specific annotations (and this should be done when possible), I think that perfect is the enemy of good. |
As i will not approve it you have to wait for one of the chart maintainers to decide. |
9019841
to
d5be1b7
Compare
@bismarck, @gianrubio, @gkarthiks, @scottrigby, @vsliouniaev, @Xtigyro Please review 🙏. This is a blocker for us currently... |
Probably this change should be requested upstream in prometheus-operator. In my opinion this chart should not alter CRDs when importing them, as it is potentially error prone and the chance that someone checks imported CRDs for changes is almost zero. |
@mrueg Upstream use kubebuilder, and I don't see any way to add annotations with it. |
I've reported this upstream prometheus-operator/prometheus-operator#4439. @mrueg What about using a kustomize patch instead of sed? This is the method used in crossplane (crossplane/crossplane#1020). |
by the way... server side apply should work too without error:
|
@monotek My usecase is within ArgoCD (where server-side apply is not implemented argoproj/argo-cd#2267). The only solutions I see is shrinking the CRD, or adding the annotation |
too messy, I even have argocd managed by argocd so modifying the cm without values in helm are not really an option. |
It's not messy at all, and the additional plugin gives you flexibility that can be used in a number of different scenarios. |
I think @irizzant 's solution is actually great. Appreciate you taking the time to share. Working great here. |
One thing in regards to @irizzant solution - be careful with helm-charts that test for k8s versions. See the remarks in the ArgoCD Discussion on using customize with helm together A suggestion from this comment would be, to improve it as following:
|
The above comment is a good improvement, though ArgoCD does not signal any out-of-sync resource in kube-prometheus-stack chart after applying it meaning that this chart does not use customized output based on k8s version. |
Please be aware, that especially for ingress and pdb this is the case for this chart. See: So in your configuration and your use case the k8s version does not make a difference for the final rendered chart, for other users it might very well |
Ah I see, I indeed don't use Ingress and PodDisruptionBudget in my case so that makes sense |
@irizzant Please note, that even if this solution works for you, Helm discourages the use of This issue is a huge blocker for us too and we would be glad, if we could add this annotation as a workaround until a more permanent solution is found upstream (e.g. shrinking the CRD). |
@FrediWeber ArgoCD uses |
@FrediWeber if you refer to the linked discussion you'll see that using Also I don't see where they're discouraging this, since the comment you linked ends up with:
|
Yes, you sure can use it but it is not expected to work exactly as expected by chart developers. Not many projects test their charts against Update: |
There can't be a shrink of the CRD. Please talk to ArgoCD developers to implement server side apply. I still vote against changing the CRD in the chart. |
@FrediWeber Consequently yes, there are differences between manifests rendered with Adding ArgoCD annotations to the upstream CRD is innocuous as a change and I agree on this, but my suggested approach (with this update ) is still a viable way to work with kube-prometheus-stack chart whether they change the upstream CRD or not. My personal vote is against changing the CRD in the chart adding an extraneous annotation though, but this is not up to me |
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.
Please remove update of python code and use kubectl apply --server-side -f ...
instead of kubectl replace
as we're already using it this way here now: https://github.com/prometheus-community/helm-charts/tree/main/charts/kube-prometheus-stack#from-24x-to-25x
@monotek I understand that you're hesitant to implement this workaround because ArgoCD is "just one solution" amongst many and there is the chance, that it will not be removed again. On the other side, there is an imminent problem of many ArgoCD users with this chart. Of course, this needs to be addressed in ArgoCD (e.g. by adding support for server side apply) but what is the downside of temporarily adding the annotation? Even if the CRDs doesn't get shrunk in the upstream chart it will take some time to implement server side apply in ArgoCD. This would be a viable workaround - and I say it again - that would spare many users some time without any downsides. @irizzant Thank you very much for pointing this out. Actually I didn't know that and your solutions seems absolutely fine and we'll also using it if there is no other workaround. I just think that we could implement this workaround and thereby remove the necessity to modify every ArgoCD instance managing this chart. |
I've been following along with the discussion here; I was hoping we'd get the ArgoCD annotation added but I can understand why the maintainers don't want to include something specific to a particular deployment tool. I'd like to offer a compromise; would you folks consider providing a flag in the chart that makes CRD installation optional? The The problem they're working against is that if you remove and replace a chart with CRDs, all objects using the new resources are deleted... this is mighty inconvenient if those resources are certificates that you have to renew. If ArgoCD users could suppress the installation of the CRDs, it's pretty easy to install them independently and make any necessary annotations or changes. This allows |
Helm install already supports "--skip-crds" as argument out if the box. So no need to add it to the chart. |
Workaround with |
@monotek @irizzant Have either of you used ArgoCD before? When using a declarative system, we don't have direct access to "helm install" or "kubectl"... the entire point is to manage your helm charts with version control and to not run CLI commands directly. The other suggestions in the thread (kustomize, etc) require a bunch of additional complexity that will make this installation harder to maintain. Again, I respect that you don't want to make a vendor-specific change; that completely makes sense. But providing the ability to suppress CRD installation seems like it would be trivial, vendor neutral, and help out many of the folks here who want to use Prometheus. Instead, it seems like the approach here is to hold fast and wait for ArgoCD to innovate around you; is that really in the best interests of the community?
|
@jplanza-gr No, never! Indeed this comment was written by having absolutely no knowledge of ArgoCD.
Since here we are ArgoCD noobs, if a Job is added to this chart as I suggested here, would you mind explaining what the required changes would be for your Application definition in ArgoCD? Or for your ArgoCD setup more in general? |
Sure, no problem. Unless there are future changes to ArgoCD, here's what I'd do today.
When the chart deploys the CRDs, the main problem an ArgoCD user faces is that the scope of these sync options is applied to everything in the chart. This means that we run the risk of having resources completely replaced rather than updated. Off the top of my head, the most important thing we could lose is any persistent caching of metrics; a sync would run and those resources (including PVs) could be replaced rather than updated in-place. If we can disable CRD installation from the main chart, it's easy to download the CRDs, store them elsewhere, and manage them with the right options. CRDs don't change very often, so we can get the desired behavior for both the CRD and chart resources. As I mentioned above, the "cert-manager" chart uses this to explicitly separate the CRD from the chart; it stores CA-validated certificates in custom resources, so you'd run the risk of destroying all your certificates during any operation that removed the CRDs. I think the "kustomize" workarounds are viable if you're already using it, but anyone on vanilla Helm 3 charts would need to introduce that just for this one application. Quite frankly, every org has practitioners with varying levels of experience and we'd rather not introduce complexity that will trip up newer folks. Back to my earlier suggestion, a toggle to deploy the CRDs with a default of true would be perfect here; your typical users won't notice any change, but those of us who have to workaround the issue can do so. Thanks for hearing me out on this. |
Fixes prometheus-community#1500 Signed-off-by: Mathieu Parent <[email protected]>
d5be1b7
to
923941d
Compare
No. And reading this issue there is no reason i would. The point here is, that you use tooling, which does not implement all options of Helm and Kubernetes completly, but still trying to use Helm charts. Maybe you want to try out FluxCD, which supports --skip-crds and also server-side apply. As we will not merge this, there a workarounds desribed and to reduce noise from now on i will close this issue and lock the conversation. If you want to discuss the issue further, i suggest using the ArgoCD issue tracker. |
What this PR does / why we need it:
The
prometheuses
CRD is too long forkubectl apply
since prometheus-operator v0.52.0 (#1485).This PR fixes this in the upgrade notes and when using ArgoCD.
Which issue this PR fixes
Special notes for your reviewer:
Checklist
[prometheus-couchdb-exporter]
)