✨ Add RootShard condition #31
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kcp-ci-bot
added
release-note
xrstf
added a commit
to xrstf/kcp-operator
that referenced
this pull request
Jan 29, 2025
The concept of revision labels was born out of the desire to automatically restart Deployments whenever a mounted Secret changes. Originally the idea came out of another product, which compared to the kcp-operator managed all Secrets completely by itself (instead of relying on external components like cert-manager). However implementing this means that a Deployment can only be created if all Secrets already exist, otherwise an error is generated. This error is a problem because it was only logged by the operator and so was invisible to the actual admins (unless they check the operator's logs). To combat this, in kcp-dev#31 I tried to move the error into a condition on the original objects (like a RootShard). For this we introduced a new condition "CertsReady" and set it according to that error. The problem with that approach is that it conflates Secrets. In additional PRs, we mount additional Secrets, like the audit-webhook-config-file. However from the somewhat generic "not all volumes are ready" we cannot actually tell whether it's because cert-manager isn't ready yet, or if the admin misconfigured a Secret. So the original condition suddenly becomes kind of misleading. We could have extended the condition by renaming it from "CertsReady" to "VolumesReady", but we still cannot with certainty say if an error is just temporary or permanent. If we were to keep the condition as "CertsReady", all controllers would have to manually get and check all the certificate Secrets. This feels like the wrong approach. The original intent was to offload complexity onto Kubernetes, but if we start to "deep-check" each dependency and object, it feels like we're fighting the Kubernetes design. We also considered not having a condition and instead writing empty values for the secret revisions into the labels. This however meant that while the PKI is being created by cert-manager (a process that takes up to a minute), the labels on the Deployment will progressively be updated, each update creating a new Pod that cannot be started (because some secrets were still missing). This felt very noisy and spammy and not the right approach either. Plus it would hide actual issues in the Deployment. Ultimately we could not think of a good, elegant way to handle this and decided that it would be the simplest if we get rid of the whole concept alltogether. This means the Deployments will be created immediately and they will create the desired number of Pods which will sit there until _Kubernetes_ decides they can progress. For other related Secrets, the controllers can actively check and create fitting conditions, like in kcp-dev#31, where a condition for the RootShardRef's status is generated explicitly. On-behalf-of: @SAP [email protected]
|
/hold |
kcp-ci-bot
added
the
do-not-merge/hold
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
On-behalf-of: @SAP [email protected]
kcp-ci-bot
added
the
needs-rebase
…ist of conditions from their reconcile() functions On-behalf-of: @SAP [email protected]
xrstf
force-pushed
the
gentle-reconciling
branch
from
b4c61c6 to
37a5378
Compare
xrstf
changed the title
kcp-ci-bot
removed
the
needs-rebase
On-behalf-of: @SAP [email protected]
xrstf
force-pushed
the
gentle-reconciling
branch
from
26fd958 to
b874d81
Compare
|
/unhold |
kcp-ci-bot
removed
the
do-not-merge/hold
|
LGTM label has been added. Git tree hash: cd0735aa3fddaa64d07664a513fad483b06a1ae7
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This started out small, with the goal of only introducing a condition to signal whether the RootShardRef is valid, so that we do not have to log errors all the time. But then it ballooned into a collection of many smaller improvements. Please see the list of commits for more info :)
Release Notes