Skip to content

Add anti-affinity for Cilium pods#11830

Merged
borkmann merged 1 commit intomasterfrom
pr/fix-preflight-race
Jun 3, 2020
Merged

Add anti-affinity for Cilium pods#11830
borkmann merged 1 commit intomasterfrom
pr/fix-preflight-race

Conversation

@nebril
Copy link
Copy Markdown
Member

@nebril nebril commented Jun 2, 2020 •
edited by aanm
Loading

This change makes sure that two Cilium pods cannot be running on the
same node in parallel. Which can happen in case an daemonset is deleted and immediately created.

Fixes: #11828

Add anti-affinity for Cilium pods to prevent 2 pods being executed on the same node at the same time

@nebril nebril requested a review from a team as a code owner June 2, 2020 16:24
@nebril nebril requested a review from a team June 2, 2020 16:24
@maintainer-s-little-helper
Copy link
Copy Markdown

maintainer-s-little-helper Bot commented Jun 2, 2020

Please set the appropriate release note label.

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 2, 2020

test-focus K8sDemos

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Jun 2, 2020

🤯 I thought Daemonset guaranteed this.

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 2, 2020 •
edited
Loading

@joestringer it actually does, but if you delete the DaemonSet and recreate it (like we do in CI), two Cilium pods can run on the same node in parallel.

Edit: The deletion/recreation causes K8s to treat these pods technically as from two different DaemonSets (which is true I guess).

joestringer
joestringer approved these changes Jun 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Not particularly familiar with the syntax here but it LGTM.

michi-covalent
michi-covalent approved these changes Jun 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@michi-covalent michi-covalent left a comment

Choose a reason for hiding this comment

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

next level stuff

@nebril
Add anti-affinity for Cilium pods
92c545d 
This change makes sure that two Cilium pods cannot be running on the
same node in parallel.

Signed-off-by: Maciej Kwiek <maciej@isovalent.com>
@nebril nebril force-pushed the pr/fix-preflight-race branch from 36fd867 to 92c545d Compare June 2, 2020 16:32
@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 2, 2020

Moved the affinity to values.yaml, since we already had a helm variable for this.

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 2, 2020

test-focus K8sDemos

@nebril nebril added the release-note/misc This PR makes changes that have no direct user impact. label Jun 2, 2020
@aanm aanm added needs-backport/1.7 release-note/bug This PR fixes an issue in a previous release of Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels Jun 2, 2020
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 2, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 36.888% when pulling 92c545d on pr/fix-preflight-race into 82fafa7 on master.

brb
brb reviewed Jun 3, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

I assume this won't interfere with https://github.com/cilium/cilium/blob/master/test/helpers/kubectl.go#L2117.

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 3, 2020

@brb I don't think so, this is AntiAffinity, the code you pointed to adds nodeAffinity , but good catch, I forgot about this bit.

Will run net-next suite just to make sure that nothing breaks

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 3, 2020

test-net-next

@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jun 3, 2020

retest-net-next

@nebril nebril added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 3, 2020
@aanm aanm removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 3, 2020
@aanm
Copy link
Copy Markdown
Member

aanm commented Jun 3, 2020

test-missed-k8s

@aanm
Copy link
Copy Markdown
Member

aanm commented Jun 3, 2020

hit #11867

@aanm aanm added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 3, 2020
@borkmann borkmann merged commit 32d3d36 into master Jun 3, 2020
@borkmann borkmann deleted the pr/fix-preflight-race branch June 3, 2020 20:17
@tklauser tklauser mentioned this pull request Jun 4, 2020
Merged
@christarazi christarazi mentioned this pull request Jun 4, 2020
Merged
@christarazi
Copy link
Copy Markdown
Member

christarazi commented Jun 10, 2020

Note to backporters: @nebril will take over backporting this to 1.7 as it has some conflicts with how the CI is setup.

@qmonnet
Copy link
Copy Markdown
Member

qmonnet commented Oct 7, 2020

Doing some clean-up, I notice the backport-pending/1.7 label on this PR. Is this still relevant, or should it be unmarked by now?

@pchaigno
Copy link
Copy Markdown
Member

pchaigno commented Oct 7, 2020

@qmonnet Backport is still in progress at #12077.

@qmonnet
Copy link
Copy Markdown
Member

qmonnet commented Oct 7, 2020

Oh I missed it, thank you.

@gangseok514 gangseok514 mentioned this pull request Nov 19, 2020
Closed
@qmonnet
Copy link
Copy Markdown
Member

qmonnet commented Feb 2, 2021

#12077 was eventually closed by Joe (#12077 (comment)), I'm removing the label.

@aanm aanm mentioned this pull request Sep 23, 2021
Merged
@aanm aanm mentioned this pull request May 9, 2023
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brb brb brb left review comments

@joestringer joestringer joestringer approved these changes

@aanm aanm aanm approved these changes

@michi-covalent michi-covalent michi-covalent approved these changes

Assignees

No one assigned

Labels

ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cilium-cni gets deleted by delayed cni-uninstall.sh invocation