-
Notifications
You must be signed in to change notification settings - Fork 48
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
multus: Delete cni cache at preStop #1253
multus: Delete cni cache at preStop #1253
Conversation
When multus is deleted the CNI resolution has to be forced for already present pods or they will not be able to be removed. This change remove the CNI cache before deleting the multus daemonset. This is related to [1] [1] kubevirt@0703604 Signed-off-by: Quique Llorente <ellorent@redhat.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
/retest |
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 would prefer to only remove the data associated with multus, but I won't object if we have to blow up the entire cni cache.
imagePullPolicy: {{ .ImagePullPolicy }} | ||
lifecycle: | ||
preStop: | ||
exec: | ||
command: ["/bin/sh", "-c", "rm -f /host/etc/cni/net.d/00-multus.conf"] | ||
command: ["/bin/sh", "-c", "rm -rf /host/etc/cni/net.d/00-multus.conf /host/var/lib/cni/*"] |
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 think it would be safer to only delete stuff associated w/ multus; I find it strange that you're destroying the cache for all CNIs.
Did you attempt to only remove the stuff belonging to multus ?
Something like (I did not check if this works, my goal is mainly to show my intention):
command:
- "/bin/sh"
- "-c"
- "find /host/var/lib/cni -name multus* -exec rm -f {} ; && rm -f /host/etc/cni/net.d/00-multus.conf"
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 tried it and didn't work, maybe I can try with grep, also the cache is reconstructed if not found, so this is not a big deal.
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.
Oh, I did not the cache was reconstructed.
I just spoke because removing the cache breaks the CNI spec for runtimes relying on libcni's cache:
A runtime must include a prevResult field in the network configuration containing the Result of the immediately preceding ADD for the container. The runtime may wish to use libcni's support for caching Results
I guess there's nothing wrong then.
/retest
|
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.
Would you update the release notes indicating that runtimes relying on the libcni's cache will have to find a way to reconstruct the cache ?
Let's really verify that first. |
/retest |
@maiqueb Have confirmed that removing just the "multus" entries is not 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.
Thanks.
Awesome work you did figuring this one out.
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.
/lgtm
/approve
Great catch! Thanks
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RamLavi 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 |
/kind bug
What this PR does / why we need it:
When multus is deleted the CNI resolution has to be forced for already
present pods or they will not be able to be removed. This change remove
the CNI cache before deleting the multus daemonset. This is related to
[1]
[1] 0703604
Release note: