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

helm: revert parts of CoreDNS Helm chart packaging #3366

Merged
merged 3 commits into from
Sep 19, 2024

Conversation

burgerdev
Copy link
Contributor

@burgerdev burgerdev commented Sep 18, 2024

Context

We are trying to move CoreDNS from a cluster addon managed by kubeadm to a Helm chart managed alongside the Constellation microservices.

After #3236, upgrading a cluster reverted CoreDNS to the version installed by kubeadm. This required #3353 as a workaround - the cluster pretends it does not have CoreDNS to fool kubeadm, details in that PR's description.

This workaround had an unexpected consequence, though: kubeadm has a preflight check for CoreDNS, which now starts failing. Since this check is done in the upgrade-agent, which is part of the image, it does not benefit from the --ignores added in #3353 and fails. Now the kubeadm upgrade step is never executed, which makes us miss important migrations in the worst case.

Proposed change(s)

Extend the migration to two upgrades.

  1. upgrade-agent ignores CoreDNS problems in 2.18+
  2. CoreDNS is installed as Helm chart in 2.19+

Additional info

Checklist

  • Run the E2E tests that are relevant to this PR's changes
  • Add labels (e.g., for changelog category)
  • Is PR title adequate for changelog?
  • Link to Milestone

Copy link

netlify bot commented Sep 18, 2024

Deploy Preview for constellation-docs canceled.

Name Link
🔨 Latest commit 4526107
🔍 Latest deploy log https://app.netlify.com/sites/constellation-docs/deploys/66eae6570ef23a0008a79126

Copy link
Contributor

@elchead elchead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably add a TODO(v2.19) to the helm chart installation / add a ticket. but I think you have this under control @burgerdev

@burgerdev
Copy link
Contributor Author

Note to reviewers:

There are three commits,

Limiting the diff to the last one should be sufficient.

we should probably add a TODO(v2.19) to the helm chart installation / add a ticket. but I think you have this under control @burgerdev

I'll add an AB ticket, thanks for the reminder.

Copy link
Member

@3u13r 3u13r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the upgrade test is still failing, the code itself LGTM

@burgerdev
Copy link
Contributor Author

Interesting - failing upgrades were not even a symptom of the issue addressed here, and this branch is rebased on #3361. The failure is also during initial cluster setup at v2.17.0, which should be sufficiently tested by now. Let's see what the retry does.

Copy link
Contributor

@msanft msanft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for explaining this so thoroughly!

Copy link
Contributor

Coverage report

Package Old New Trend
bootstrapper/internal/kubernetes 37.60% 37.70% ↗️
bootstrapper/internal/kubernetes/k8sapi 11.80% 11.80% 🚧
cli/internal/cmd 41.10% 41.10% 🚧
internal/constellation 18.50% 18.60% ↗️
internal/constellation/helm 34.50% 33.60% ↘️
internal/kubernetes/kubectl 8.20% 6.90% ↘️
terraform-provider-constellation/internal/provider 3.50% 3.50% 🚧

@burgerdev burgerdev merged commit 850b460 into main Sep 19, 2024
25 of 27 checks passed
@burgerdev burgerdev deleted the burgerdev/split-coredns-helm branch September 19, 2024 08:55
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.

4 participants