ipam: Fix inconsistent update of CiliumNodes #19923
Conversation
988f255 to
efcd365
Compare
Currently, when the cilium-operator attaches new ENIs to a node, we
update the corresponding CiliumNode in two steps: first the .Status,
then the .Spec [1]. That can result in an inconsistent state, where the
CiliumNode .Spec.IPAM.Pool contains new IP addresses associated with the
new ENI, while .Status.ENI.ENIs is still missing the ENI. This
inconsistency manisfests as a fatal:
level=fatal msg="Error while creating daemon" error="Unable to allocate router IP for family ipv4: failed to associate IP 10.12.14.5 inside CiliumNode: unable to find ENI eni-9ab538c64feb9f59e" subsys=daemon
This inconsistency occurs because the following can happen:
1. cilium-operator attaches a new ENI to the CiliumNode.
2. Still at cilium-operator, .Spec is synced with kube-apiserver. The IP
pool is updated with a new set of IP addresses and the new ENI.
3. The agent receives this half-updated CiliumNode.
4. It allocates an IP address for the router from the pool of IPs
attached to the new ENI, using .Spec.IPAM.Pool.
5. It fails because the new ENI is not listed in the .Status.ENI.ENIs of
the CiliumNode object.
6. At cilium-operator, .Status is updated with the new ENI.
But wait, you said .Status is updated before .Spec in the function you
linked? Yes, but we read the state to populate CiliumNode from two
separate places (n.ops.manager.instances and n.available) in the
syncToAPIServer function and we don't have anything to prevent having a
half updated (one place only) state in the middle of the update function.
We lock twice, once for each place, instead of once for the while
CiliumNode update. So having a half updated state in the middle of the
function would technically be the same as updating .Spec first and
.Status second.
We can fix this by first creating a snapshot of the pool first, then
write the .Status metadata (which may be more recent than the pool
snapshot, which is safe, see comment in the source code of this patch),
and then write the pool to .Spec. This ensures that the .Status is
always updated before .Spec, but at the same time also ensures that
.Status is still more recent than .Spec.
1 - https://github.com/cilium/cilium/blob/v1.12.0-rc2/pkg/ipam/node.go#L966-L1012
Co-authored-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: Paul Chaignon <paul@cilium.io>
Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
The `node.Spec.IPAM.Pool` value is always overwritten after the removed `if` statement, so there is no need to initialize it. Signed-off-by: Sebastian Wicki <sebastian@isovalent.com>
efcd365 to
559251a
Compare
|
I'm taking over the PR from Paul. Instead of trying to move around the mutexes, I decided to follow a slightly different approach where we take a snapshot of the IPAM Pool first. @christarazi would be great if you can take a look. |
| // When an IP is removed, this is also safe. IP release is done via | ||
| // handshake, where the agent will never use any IP where it has | ||
| // acknowledged the release handshake. Therefore, having an already | ||
| // released IP in the pool is fine, as the agent will ignore it. |
There was a problem hiding this comment.
@hemanthmalla Since you are more familiar with the release logic, could you confirm that this argument is sound?
There was a problem hiding this comment.
Yes, agent will ignore the IPs in the middle of a handshake for allocations. So it's safe. See https://github.com/DataDog/cilium/blob/b5fd71b9f873942764e90e41b39821ea48152832/pkg/ipam/crd.go#L564-L566
|
/test Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed: Click to show.Test NameFailure OutputIf it is a flake and a GitHub issue doesn't already exist to track it, comment |
Not really. It addresses the second part ("there is a short time period with inconsistencies between status and spec, as these are not updated in an atomic fashion"), but #18366 also concerns itself with potential inconsistencies if one of the two updates fails, which could still happen. This PR here only fixes the issue where .Spec and .Status are inconsistent even if the update to the custom resource succeeded. #18366 is broader, as it is also about dealing with cases where the update to the custom resource itself fails. It should also be noted that this PR here only works, because currently the metadata in |
Currently, when the cilium-operator attaches new ENIs to a node, we update the corresponding CiliumNode in two steps: first the
.Status, then the.Spec. That can result in an inconsistent state, where the CiliumNode.Spec.IPAM.Poolcontains new IP addresses associated with the new ENI, while.Status.ENI.ENIsis still missing the ENI. This inconsistency manifests as a fatal:This inconsistency occurs because the following can happen:
.Specis synced with kube-apiserver. The IP pool is updated with a new set of IP addresses and the new ENI..Spec.IPAM.Pool..Status.ENI.ENIsof the CiliumNode object..Statusis updated with the new ENI.But wait, you said
.Statusis updated before.Specin the function you linked? Yes, but we read the state to populate CiliumNode from two separate places (n.ops.manager.instancesandn.available) in thesyncToAPIServerfunction and we don't have anything to prevent having a half updated (one place only) state in the middle of the update function. We lock twice, once for each place, instead of once for the while CiliumNode update. So having a half updated state in the middle of the function would technically be the same as updating.Specfirst and.Statussecond.We can fix this by first creating a snapshot of the pool first, then write the .Status metadata (which may be more recent than the pool snapshot, which is safe, see comment in the source code of this patch), and then write the pool to .Spec. This ensures that the .Status is always updated before .Spec, but at the same time also ensures that .Status is still more recent than .Spec.