daemon: Simplify cilium_host IP restoration
#28781
Conversation
143491c to
6ffb771
Compare
|
I've tested this code locally with multi-pool and cluster-pool IPAM modes. Two open questions:
|
|
/test |
6ffb771 to
e0aac0b
Compare
|
/test |
|
Required CI passed, but there is a typo in one of the commit messages. Repushing. |
e0aac0b to
932a2fe
Compare
|
/test |
|
GKE and external workload failures look legitimate. Will look into it. |
On closer look, the failures actually seem unrelated, and the more likely explanation is a minor GKE outage yesterday:
I've checked the sysdump of the failed jobs and did not see anything related to this PR. In fact, none of these GKE-based workflows did restart the Cilium pods before they failed, so no router restoration was performed. It should also be noted that the same pipelines were previously green on a very similar version of the branch.
Considering the above, I'm fairy confident the failures are unrelated. I'll restart CI. |
|
/ci-gke |
|
/ci-external-workloads |
|
/ci-ipsec-upgrade |
The last IPsec failiure looks a bit suspicious, as it is running the code from |
932a2fe to
26308e2
Compare
|
/ci-ipsec-upgrade |
26308e2 to
6a046cc
Compare
|
/ci-ipsec-upgrade |
6a046cc to
ce889cf
Compare
|
/ci-ipsec-upgrade |
|
Rebased due to conflicts again. Waiting on reviews. ci-ipsec-upgrade failures seem to be unrelated, it also passed recently. I've investigated multiple sysdumps and did not see anything that points towards router IP restoration not working. The workflow is rather unstable on |
ce889cf to
8842226
Compare
|
/test |
Probably something worth bringing up at the next community meeting 🤔 |
|
Changes look good on my end, probably worth discussing the k8s restore question (maybe we can just officially get rid of this?). |
|
Many CI failures due to cloudflare outage. Rebasing and restarting. |
8842226 to
d441afa
Compare
|
/test |
There was a problem hiding this comment.
LGTM, thanks for this, much better looking code now!
A couple of nits below.
Does restoration from K8s even work? We fetch the k8s Node object (not CiliumNode) and it no longer seems to contain an annotation with the router IP? And even with clusterpool, we call UpdateCiliumNodeResource before WaitForNodeInformation, which simply clears out the old router IP (and ignores the router IP from the Node resource if it was present, but it anyway doesn't seem to be). At least with Kind, I have not been able to get it to work both before and after this PR
I'm fairly certain that this was not the case when the K8s restoration code was added. This restoration logic relies on the annotation to be set. So if somewhere else in Cilium stopped setting the annotation, then that is what needs to be investigated. If we no longer set the annotation at all, then we should probably remove the K8s restoration code because we then have no mechanism to restore state from K8s.
d441afa to
cecc24d
Compare
|
Thanks for the reviews! I believe I've address the feedback!
I'll create an issue (edit: #29019). I think the annotation is indeed no longer set for at least one release, and this has been reported before, though in that case for the pod CIDR: #24420 IMO, we should attempt to restore the router IP from the local CiliumNode object, rather than adding annotations to the K8s Node object. But that's a separate discussion. |
The `restoreCiliumHostIPs` function does two things: Determine which IP to restore, and then remove any IPs from the cilium_host device which are not the restored one. This commit splits out the second part, so it can be retried independently. There is no reason to retry the first part, as it is deterministic. This commit prepares the code for further changes following in subsequent commits. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
This commit contains no functional changes and prepares the code for changes coming up in subsequent commits. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Before this commit, we were setting the restored IP during `node.AutoComplete`, getting it `Daemon.restoreCiliumHostIPs`, only to set it again in `node.RestoreHostIPs` if it passed validation. This commit simplifies the logic to extract the IPs from the file system, and only setting them after validation. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Before this commit, the `cilium_host` (aka router) IP restoration was done in two steps: The first step collected previous IPs from the filesystem and Kubernetes and attempted to guess which one might still be valid in the new IPAM configuration. This validation was done using either the Pod CIDR, the VPC CIDR, or the native routing CIDR. In a second step, the IP was then actually re-allocated via the IPAM subsystem. That second step could however still fail - just because an IP is part of the VPC CIDR or native routing CIDR does not mean it actually still assigned to the node. This commit attempts to simplify the logic: Instead of trying to guess if the restore IP(s) could be allocated using IPAM, we just try to allocate them. If the allocation fails, we still have a fall back logic, and the old IP address(es) are still removed from the `cilium_host` interface. This way, the old CIDR check becomes obsolete, as we now use the IPAM subsystem as the source of truth if an IP can be restored or not. This also fixes a bug where Multi-Pool IPAM always reallocated the router IP, because it did not implement the CIDR check in the first step. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
Before the previous commit, `removeOldRouterState` would only be called with a restored IP address, which could be nil. Now however, it is always called with the new router IP, which could have been restored, or could have been a new allocation. In either case, we want to remove any non-matching IPs from `cilium_host`. However, the case with new allocations now also happens the first time Cilium is started, in which case there is no `cilium_host` device yet. This commit therefore does not treat a missing `cilium_host` as an error. This fixes the following warning which was introduced by the previous commit: ``` level=warning msg="Failed to remove old router IPs from cilium_host." attempt=1 error="Link not found\nLink not found" subsys=daemon ``` Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
The previous commit made these functions obsolete. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
This slightly modifies the `reallocateDatapathIPs` function to take a mock allocator as its first argument, and adds a basic unit test which tests for the precedence logic we want. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
This helps troubleshooting potential bugs in CI. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
cecc24d to
d9f60df
Compare
|
/test |
Before this PR, the
cilium_host(aka router) IP restoration was done in two steps:The first step collected previous IPs from the filesystem and Kubernetes and attempted to guess which one might still be valid in the new IPAM configuration. This validation was done using either the Pod CIDR, the VPC CIDR, or the native routing CIDR.
In a second step, the IP was then actually re-allocated via the IPAM subsystem. That second step could however still fail - just because an IP is part of the VPC CIDR or native routing CIDR does not mean it actually still assigned to the node.
This PR attempts to simplify the logic: Instead of trying to guess if the restore IP(s) could be allocated using IPAM, we just try to allocate them. If the allocation fails, we still have a fall back logic, and the old IP address(es) are still removed from the
cilium_hostinterface. This way, the old CIDR check becomes obsolete, as we now use the IPAM subsystem as the source of truth if an IP can be restored or not.This also fixes a bug where Multi-Pool IPAM always reallocated the router IP, because it did not implement the CIDR check in the first step.
Review per commit.
Fixes: #28743