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.

Sign up for GitHub

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

Jump to bottom

Add anti-affinity for Cilium pods #11830

Merged
merged 1 commit into from Jun 3, 2020
Merged

Add anti-affinity for Cilium pods #11830

merged 1 commit into from Jun 3, 2020

Conversation

nebril
Copy link
Member

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

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

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 2, 2020
@nebril
Copy link
Member Author

nebril commented Jun 2, 2020

test-focus K8sDemos

@joestringer
Copy link
Member

🤯 I thought Daemonset guaranteed this.

@nebril
Copy link
Member Author

nebril commented Jun 2, 2020 •
edited

@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
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
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
Copy link
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
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
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.5 Jun 2, 2020
@aanm aanm added 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

coveralls commented Jun 2, 2020 •
edited

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
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
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
Member Author

nebril commented Jun 3, 2020

test-net-next

@nebril
Copy link
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
Copy link
Member

aanm commented Jun 3, 2020

test-missed-k8s

@aanm
Copy link
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
1.8.0 automation moved this from In progress to Merged 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
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 4, 2020
@christarazi christarazi mentioned this pull request Jun 4, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.5 Jun 4, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 5, 2020
@christarazi
Copy link
Member

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
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
Member

pchaigno commented Oct 7, 2020

@qmonnet Backport is still in progress at #12077.

@qmonnet
Copy link
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
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
No open projects
1.7.5
Backport pending to v1.7
1.8.0
  
Merged
1.8.0
Backport done to v1.8
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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