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

[main] Fix CRDs being installed even when they already exist and don't need updating #114

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

mallardduck
Copy link
Member

@mallardduck mallardduck commented Oct 24, 2024

Related Issue: rancher/rancher#46764 / SURE-8872

Per the title, this PR essentially just corrects instances where the CRDs related to this operator might already exist on the cluster it's being installed on. This fix simply ensures that CRDs are only installed when they actually should be installed.

Fixing the bug in this manner does introduce a minor regression related to CRDs being updated. Prior to this change if prometheus-federator wanted to ship an updated CRD it's fine because - in the past - all CRDs are always re-installed essentially at every start up.

While this process is still done at every startup, because we only act on installing missing CRDs it means CRDs we do control and need to update won't be updated currently.


Potential fixes for the update regression:

  • Always install helm locker's CRD since we should be the only ones shipping that ever.
  • Identify if RKE2/k3s and only update CRDs outside of these distros

The description of the original issue isn't very accurate today as it used to be (i.e. using recent k3s/rke2 versions things work differently). However this PR seeks to address the root concern of the issues even if the conditions of modern distros are different.

Copy link

Images built for PR #114:

  • Helm Locker: Link to image:
    Tags: ghcr.io/rancher/prometheus-federator/helm-locker:pr-114 ghcr.io/rancher/prometheus-federator/helm-locker:pr-114-4bc6a44

  • Helm Project Operator: Link to image:
    Tags: ghcr.io/rancher/prometheus-federator/helm-project-operator:pr-114 ghcr.io/rancher/prometheus-federator/helm-project-operator:pr-114-4bc6a44

  • Prometheus Federator: Link to image:
    Tags: ghcr.io/rancher/prometheus-federator:pr-114 ghcr.io/rancher/prometheus-federator:pr-114-4bc6a44

@mallardduck mallardduck changed the title Crd install bugfix [main] Fix CRDs being installed even when they already exist and don't need updating Oct 25, 2024
@mallardduck mallardduck marked this pull request as ready for review October 25, 2024 21:26
@mallardduck mallardduck requested a review from a team as a code owner October 25, 2024 21:26
Copy link

Images built for PR #114:

  • Helm Locker: Link to image:
    Tags: ghcr.io/rancher/prometheus-federator/helm-locker:pr-114 ghcr.io/rancher/prometheus-federator/helm-locker:pr-114-4bc6a44

  • Helm Project Operator: Link to image:
    Tags: ghcr.io/rancher/prometheus-federator/helm-project-operator:pr-114 ghcr.io/rancher/prometheus-federator/helm-project-operator:pr-114-4bc6a44

  • Prometheus Federator: Link to image:
    Tags: ghcr.io/rancher/prometheus-federator:pr-114 ghcr.io/rancher/prometheus-federator:pr-114-4bc6a44

@mallardduck
Copy link
Member Author

mallardduck commented Oct 28, 2024

So with this fix in place, if you create a cluster (using k3s/rke2) and then later install the chart you should see CRDs with 2 sets of timestamps. One for the cluster creation and one for the helm chart install time - the helmcharts and helmchartconfigs will be from creation time. Like:

helmchartconfigs.helm.cattle.io                                   2024-10-28T00:53:16Z
helmcharts.helm.cattle.io                                         2024-10-28T00:53:16Z
helmreleases.helm.cattle.io                                       2024-10-28T15:23:18Z
projecthelmcharts.helm.cattle.io                                  2024-10-28T15:23:18Z

Actually, while this seems like an improvement and the correct behavior...I'm not currently able to replicate this at all on 2.9.2 with 104.0.0+up0.4.0 which should be the same as the reported prometheus-federator-103.0.2+up0.4.0 which has the issue.

I guess I'll try to setup 2.8 cluster and see if I can even replicate the original issue.


Oh right - the creation date is what we're seeing and that doesn't change. So I should have compared status fields to check update at times. Currently rebuilding lab to test on 2.8 anyways but I'll remember that.

@mallardduck
Copy link
Member Author

Summary of Helm-Controller Situation:

Helm Controller Release Date CRD Changes (Compared to Current) RKE2 K3S PromFed PromFed Update Safety
v0.13.1 Nov 23, 2022 N/A v1.26.1+rke2r1, v1.25.6+rke2r1 v1.26.3+k3s1, v1.25.8+k3s1 Current N/A
v0.13.3 Apr 4, 2023 False N/A v1.27.1+k3s1, v1.26.4+k3s1, v1.25.9+k3s1 N/A V Low Risk
v0.14.0 May 10, 2023 True (skipped, not helpful info) (skipped, not helpful info) N/A Higher Risk
v0.15.4 Aug 15, 2023 True v1.29.4+rke2r1, v1.28.9+rke2r1, v1.27.13+rke2r1, v1.26.12+rke2r1, v1.25.16+rke2r2 v1.29.0+k3s1, v1.28.5+k3s1, v1.27.9+k3s1, v1.26.12+k3s1, v1.25.16+k3s4 N/A Higher Risk
v0.16.4 Sep 5, 2024 True; even more v1.30.5+rke2r1 v1.30.5+k3s1 N/A V High Risk

Note that I've been testing with 1.28.14 which matches up with v0.15.13 of helm-controller. And as noted in the table, v0.13.1 of helm-controller currently lines up with the one PromFed ships.

More info on CRD differences: https://gist.github.com/mallardduck/120b1d5d833126ae2cb8ff2df20c53c5

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.

1 participant