AKS: Clean up azure-vnet state on Cilium agent start #14452
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
maintainer-s-little-helper
bot
assigned aanm, qmonnet, seanmwinn, tgraf and twpayne and unassigned seanmwinn and qmonnet
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
1 task
christarazi
approved these changes
Dec 18, 2020
twpayne
approved these changes
Dec 21, 2020
qmonnet
approved these changes
Dec 21, 2020
install/kubernetes/cilium/templates/cilium-nodeinit-daemonset.yaml
Outdated
Show resolved
Hide resolved
aanm
approved these changes
Dec 22, 2020
aanm
added
the
dont-merge/needs-rebase
ti-mo
force-pushed
the
pr/tb/azure-vnet-state-cleanup
branch
2 times, most recently
from
3d8083e
to
c4deff1
Compare
|
Addressed comments and rebased. 👍 Also removed |
ti-mo
force-pushed
the
pr/tb/azure-vnet-state-cleanup
branch
from
8528be8
to
7c6a967
Compare
|
Looks like the docs job is not happy with the changes. |
ti-mo
force-pushed
the
pr/tb/azure-vnet-state-cleanup
branch
from
7c6a967
to
08a3e5e
Compare
|
This should be ready @aanm PTAL 🙏 |
aanm
approved these changes
Jan 5, 2021
|
@ti-mo please add the labels for which this PR needs to be backported as well as the proper release label. |
Commit 0b70117 ("AKS: Fix dynamic reconfiguration of bridge mode") and preceding commits would modify an internal state file of the azure-vnet CNI plugin. This is potentially dangerous, because: - azure-vnet uses a lockfile to coordinate access to this state file between independent invocations. - The format of the state file is not controlled by us and subject to change. - How this state file is used can change between versions of azure-vnet. After investigating some reports of connectivity issues, it turns out that: - Modifying the state file directly isn't necessary. azure-vnet obeys the mode it's given in its CNI configuration, regardless of the mode recorded in the state file. - Changing the mode directly _in the state file_ from `bridge` to `transparent` causes azure-vnet to forget cleaning up static ARP entries for deleted Pods that were previously created in bridge mode before Cilium was deployed. This can cause addresses of Pods previously scheduled on the node to become unroutable on said node. - Waiting for the state file to appear causes a deadlock on newly-created nodes in a cluster where Cilium is already running, for example, when scaling out a node pool. azure-vnet only creates the state file when it gets its first CNI `ADD` event, which will never happen with the CNI configurations Cilium installs. Signed-off-by: Timo Beckers <timo@isovalent.com>
…ghbors This aims to address cilium#14233 ("Pods scheduled before Cilium don't get state cleaned up") until a better solution can be implemented for AKS, ideally by gaining support for deploying Cilium directly by Azure. Currently, when Cilium is deployed with Azure IPAM enabled, all workload Pod connectivity is interrupted until the Pod is re-scheduled. Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
ti-mo
force-pushed
the
pr/tb/azure-vnet-state-cleanup
branch
from
08a3e5e
to
d91c7f7
Compare
ti-mo
added
needs-backport/1.9
and removed
dont-merge/needs-rebase
ti-mo
added
the
release-note/minor
maintainer-s-little-helper
bot
removed
the
dont-merge/needs-release-note-label
ti-mo
added
the
ready-to-merge
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.9
in 1.9.2
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.9
in 1.9.2
maintainer-s-little-helper
bot
moved this from Backport pending to v1.9
to Backport done to v1.9
in 1.9.2
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.
Fixes #12113
Fixes #14233
Rationale is documented in the commit messages. We need something to move AKS support forward, as there are multiple breaking issues in 1.9.
@seanmwinn Should a Helm value be added to disable the rm/ebtables/ip neigh path?
@aanm Should we document the fact that deploying with Azure IPAM disconnects all workload Pods in the cluster? (getting rid of this behaviour would be a requirement for declaring Azure IPAM as GA, so this might not be necessary)