policy/k8s: Fix bug where policy synchronization event was lost #32028
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
gandro
added
sig/k8s
gandro
force-pushed
the
pr/gandro/k8s-policy-fix-sync-race
branch
from
0aecad3
to
f86fa91
Compare
|
/test |
derailed
reviewed
Apr 19, 2024
The policy watcher does not need to be exported. This commit also prepares the code for a minor restructuring in a subsequent commit. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
This commit fixes an issue with the K8s policy ingestor where `InitK8sSubsystem` unblocked the daemon startup before all initial policies were processed by the ingestor. This meant that we created unnecessary endpoint regenerations at startup, due to the initial endpoint regeneration being disrupted by the discovery of additional policies (and thus re-triggering endpoint regeneration). The cause for the race was that we started the K8s policy watcher in the Hive lifecycle start hook, which is too late: The `InitK8sSubsystem` function called from newDaemon constructor is supposed to block on policy resources being synced. But because we registered those resources (via `BlockWaitGroupToSyncResources`) only after the `newDaemon` constructor already ran (we depended on a fully constructed `Daemon` object), `InitK8sSubsystem` would continue without the policy resources being synced and thus causing unnecessary endpoint regenerations. This commit fixes the issue by starting the policy resource watcher earlier, namely directly in `newDaemon`. This unfortunately means that we have to side-step the Hive infrastructure, as we currently require a partially initialized `Daemon` object to implement the `PolicyManager` interface for the time being. Hopefully, in the future we can move the `PolicyManager` logic out of the `Daemon` struct and thereby breaking this cyclic dependency. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
gandro
force-pushed
the
pr/gandro/k8s-policy-fix-sync-race
branch
from
f86fa91
to
d1aa7ae
Compare
|
/test |
squeed
requested review from
a team and
dylandreimerink
and removed request for
a team
|
Tagged sig/foundations here. Edit: Sorry! somehow missed that @derailed was also foundations. Sorry :-) |
squeed
approved these changes
Apr 22, 2024
dylandreimerink
approved these changes
Apr 24, 2024
maintainer-s-little-helper
bot
added
the
ready-to-merge
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.
This PR fixes an issue with the K8s policy ingestor where
InitK8sSubsystemunblocked the daemon startup before all initial policies were processed by the ingestor.This meant that we created unnecessary endpoint regenerations at startup, due to the initial endpoint regeneration being disrupted by the discovery of additional policies (and thus re-triggering endpoint regeneration).
The cause for the race was that we started the K8s policy watcher in the Hive lifecycle start hook, which is too late: The
InitK8sSubsystemfunction called from newDaemon constructor is supposed to block on policy resources being synced. But because we registered those resources (viaBlockWaitGroupToSyncResources) only after thenewDaemonconstructor already ran (we depended on a fully constructedDaemonobject),InitK8sSubsystemwould continue without the policy resources being synced and thus causing unnecessary endpoint regenerations.This PR fixes the issue by starting the policy resource watcher earlier, namely directly in
newDaemon. This unfortunately means that we have to side-step the Hive infrastructure, as we currently require a partially initializedDaemonobject to implement thePolicyManagerinterface for the time being. Hopefully, in the future we can move thePolicyManagerlogic out of theDaemonstruct and thereby breaking this cyclic dependency.I have manually tested this, both by checking the logs and by disabling the CNP sync manually (which then blocks the agent before it restores endpoints). Unfortunately, I didn't find a good way to assert this via unit tests.
Fixes: #31865