[Antrea-IPAM] Support pre-allocating continuous IPs for StatefulSet #3281

annakhm · 2022-02-08T01:31:06Z

In order to provide better user experience, AtreamIPAM will try to
preallocate continuous IP range for StatefulSet. If unsuccesful,
IPs for the StatfulSet will be allocated on the fly, as before.

Signed-off-by: Anna Khmelnitsky akhmelnitsky@vmware.com

codecov-commenter · 2022-02-08T01:37:56Z

Codecov Report

Merging #3281 (00dfb7e) into main (2dcc257) will decrease coverage by 11.92%.
The diff coverage is 62.85%.

@@             Coverage Diff             @@
##             main    #3281       +/-   ##
===========================================
- Coverage   65.51%   53.58%   -11.93%     
===========================================
  Files         277      392      +115     
  Lines       27500    43040    +15540     
===========================================
+ Hits        18016    23063     +5047     
- Misses       7567    17655    +10088     
- Partials     1917     2322      +405

Flag	Coverage Δ
integration-tests	`38.23% <ø> (?)`
kind-e2e-tests	`47.80% <0.00%> (-8.03%)`	⬇️
unit-tests	`43.36% <62.85%> (+0.32%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/ipam/poolallocator/allocator.go	`48.87% <14.28%> (-1.13%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`74.13% <65.51%> (-6.15%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go	`80.00% <100.00%> (ø)`
pkg/apis/controlplane/types.go	`0.00% <0.00%> (-100.00%)`	⬇️
...g/agent/apiserver/handlers/featuregates/handler.go	`0.00% <0.00%> (-82.36%)`	⬇️
pkg/controller/networkpolicy/store/group.go	`5.00% <0.00%> (-70.00%)`	⬇️
pkg/apis/controlplane/helper.go	`33.33% <0.00%> (-66.67%)`	⬇️
pkg/controller/networkpolicy/convert.go	`0.00% <0.00%> (-64.52%)`	⬇️
pkg/controller/egress/store/egressgroup.go	`1.38% <0.00%> (-58.34%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`20.45% <0.00%> (-54.55%)`	⬇️
... and 170 more

build/yamls/base/controller-rbac.yml

pkg/ipam/poolallocator/allocator.go

annakhm · 2022-02-16T23:37:37Z

/test-all

gran-vmv

LGTM

gran-vmv · 2022-02-17T09:14:49Z

/test-integration
/test-flexible-ipam-e2e

annakhm · 2022-02-18T00:30:13Z

Would appreciate feedback - we need to distinguish between IPAddressPhaseReserved and v1alpha2.IPAddressPhasePreallocated, or is it better to combine them?

gran-vmv · 2022-02-18T01:00:17Z

Would appreciate feedback - we need to distinguish between IPAddressPhaseReserved and v1alpha2.IPAddressPhasePreallocated, or is it better to combine them?

@annakhm Please use IPAddressPhaseReserved, and IPAddressPhasePreallocated is deprecated.

pkg/controller/ipam/antrea_ipam_controller_test.go

Some change needed.

annakhm · 2022-02-18T21:52:56Z

/test-all

gran-vmv

LGTM

jianjuns

I did not do a thorough review of this PR, but I would trust existing reviews. So, you need not my approval to merge it.

annakhm · 2022-03-23T18:10:43Z

/test-all

gran-vmv · 2022-03-24T02:13:46Z

All e2e failed.

gran-vmv · 2022-03-24T07:45:04Z

/test-e2e

gran-vmv · 2022-03-24T08:48:08Z

/test-flexible-ipam-e2e

gran-vmv

LGTM

gran-vmv · 2022-03-24T11:12:49Z

/test-windows-all
/test-ipv6-all
/test-ipv6-only-all
/test-multicluster-e2e
/test-integration

gran-vmv · 2022-03-24T11:45:42Z

/test-flexible-ipam-e2e

gran-vmv · 2022-03-24T12:16:00Z

/test-windows-proxyall-e2e

tnqn · 2022-03-24T11:29:31Z

pkg/controller/ipam/antrea_ipam_controller.go

+	key := k8s.NamespacedName(ss.Namespace, ss.Name)
+
+	c.statefulSetMap.Store(key, ss)
+	c.statefulSetQueue.Add(addEventIndication + key)


Agree with @jianjuns. And there are some other potential issues:

workqueue is meaningless here if it just needs a FIFO, in which case channel is more proper.

As the key in workqueue has a "a" or "d" prefix, one object's add and delete event will be two items in workqueue, then it's not possible to increase the number of workers that handle statefulSet, which may not scale well.

It cannot support replicas number change, as processNextStatefulSetWorkItem processes the statefulset using the "snapshot" of its spec.

As there is only single worker today, processing the statefulSet based on "snapshot" is more error-prone. For example, if multiple statefulSets are recreated, the worker has to process deletion of the statefulSets serially, then their creation, during which Pods of the new statefulSets may have been created, the work may release the IP allocations by mistake when it handles the deletion event of a statefulSet, leading to duplicate allocation.

With single worker, creation of multiple statefulSets may be handled slower than kubelet creating Pods, leading to many discontinuous IP allocation in practice.

As your comment points out, transient errors when processing delete event will lead to IP leak and can only be fixed until controller restarts, which is not very robust.

All controllers need to consider the recreate case, and the typical controller pattern still works for them. I think it's appliable to IPAM as well.

What the controller does is to ensure statefulSet's replica == number of continuous IPs in IPPool, and we don't really care UID of statefulset. Then even after the statefulSet is deleted and recreated, we don't really need to delete all IP allocations from the IPPool first then re-allocate.

It should work if implementing the controller in this way:

The event handlers only need to enqueue the key (namespace + name) of the statefulSet to the queue, regardless of it's add/update/delete events (some update events can be ingored if fields we care don't change).

The worker gets the key from the queue, checks the latest spec in the lister.

If the statefulSet doesn't exist, it cleans up all IP allocation of this statefulSet.

If the statefulSet exists, it allocates or releases IPs to make sure the number of IP allocation equal to the number of replicas. This is quite similar to ReplicaSetController which ensures the number of Pods equal to the number of ReplicaSet's replica.

In this way, it can run multiple workers to process statefulSet concurrently, can handle replica number change, and prevents introducing another storage and potential race condition coming with it.

pkg/controller/ipam/antrea_ipam_controller.go

annakhm · 2022-03-24T17:27:21Z

Thanks @tnqn and @jianjuns for your comments, I'm refactoring the change based on those recommendations.

In order to provide better user experience, AtreamIPAM will try to reserve continuous IP range for StatefulSet. If unsuccesful, IPs for the StatfulSet will be allocated on the fly, as before. Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

annakhm · 2022-03-24T22:37:21Z

/test-all

annakhm · 2022-03-24T22:46:12Z

/test-flexible-ipam-e2e

pkg/controller/ipam/antrea_ipam_controller.go

jianjuns · 2022-03-24T22:56:42Z

pkg/controller/ipam/antrea_ipam_controller.go

+	}
+
+	size := int(*ss.Spec.Replicas)
+	err = allocator.AllocateStatefulSet(ss.Namespace, ss.Name, size)


Have we checked no IP allocated for this StatefulSet already?

And add comments somewhere to indicate we do not support IPPool changes.

AllocateStatefulSet has this check inside: IPs will not be preallocated if this StatefulSet is already present in the pool: https://github.com/antrea-io/antrea/pull/3281/files#diff-5e4e20e1e087d03ded8c46066bd42db715fc14fa73a9e4ac6fcfeab83e8d7f4cR412

And could we also add a comment about this then? It is just easier to understand.

Sure, will do

We have a note in docs that explains that IPPool annotation can not be changed without recreating the resource: Note that the IP pool annotation cannot be updated or deleted without recreating the resource. We should add this to validation.

That is great. But still feel better to add a comment in the code, so reviewers/readers like me can easily understand why we do not check if pool changes or not.

Added comment in code

annakhm · 2022-03-25T00:02:14Z

Thank you again @tnqn and @jianjuns, indeed when refactored according to your suggestions, the code is more straightforward and less prone to race conditions. @tnqn please note that since the goal is allocation of continuous IP range, today we don't resize preallocations based on replica number. Scaling up is unlikely to succeed (since next IP is likely to be taken), scaling down is possible though.

annakhm · 2022-03-25T00:05:11Z

/test-flexible-ipam-e2e

tnqn

LGTM overall, some minor comments

pkg/controller/ipam/antrea_ipam_controller_test.go

pkg/controller/ipam/antrea_ipam_controller.go

Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

annakhm · 2022-03-25T21:56:05Z

/test-all

tnqn

LGTM

tnqn · 2022-03-26T02:47:25Z

/test-flexible-ipam-e2e

annakhm marked this pull request as draft February 8, 2022 01:31

annakhm force-pushed the antreaipam-preallocate branch 5 times, most recently from bdf284c to bfe3f41 Compare February 14, 2022 21:07

annakhm requested a review from gran-vmv February 14, 2022 23:05

annakhm force-pushed the antreaipam-preallocate branch from bfe3f41 to d62620d Compare February 15, 2022 01:28

annakhm marked this pull request as ready for review February 15, 2022 01:44

gran-vmv reviewed Feb 15, 2022

View reviewed changes

build/yamls/base/controller-rbac.yml Outdated Show resolved Hide resolved

pkg/ipam/poolallocator/allocator.go Outdated Show resolved Hide resolved

annakhm force-pushed the antreaipam-preallocate branch from d62620d to de41bf4 Compare February 16, 2022 00:59

gran-vmv self-requested a review February 17, 2022 09:13

gran-vmv previously approved these changes Feb 17, 2022

View reviewed changes

annakhm requested review from tnqn and jianjuns February 18, 2022 00:44

gran-vmv self-requested a review February 18, 2022 01:00

gran-vmv reviewed Feb 18, 2022

View reviewed changes

pkg/controller/ipam/antrea_ipam_controller_test.go Outdated Show resolved Hide resolved

gran-vmv self-requested a review February 18, 2022 01:14

annakhm force-pushed the antreaipam-preallocate branch from de41bf4 to 4f96f3f Compare February 18, 2022 20:02

gran-vmv previously approved these changes Feb 21, 2022

View reviewed changes

gran-vmv requested review from antoninbas and salv-orlando February 21, 2022 07:03

jianjuns reviewed Feb 23, 2022

View reviewed changes

annakhm force-pushed the antreaipam-preallocate branch from 3a299cc to 5abb050 Compare March 23, 2022 17:20

gran-vmv previously approved these changes Mar 24, 2022

View reviewed changes

tnqn reviewed Mar 24, 2022

View reviewed changes

annakhm dismissed gran-vmv’s stale review via 1f7c8a5 March 24, 2022 19:42

annakhm force-pushed the antreaipam-preallocate branch from 5abb050 to 1f7c8a5 Compare March 24, 2022 19:42

jianjuns reviewed Mar 24, 2022

View reviewed changes

annakhm marked this pull request as draft March 24, 2022 23:42

annakhm force-pushed the antreaipam-preallocate branch from e2989d0 to 996ec5e Compare March 25, 2022 00:03

annakhm marked this pull request as ready for review March 25, 2022 00:03

tnqn reviewed Mar 25, 2022

View reviewed changes

Refactor preallocation

00dfb7e

Signed-off-by: Anna Khmelnitsky <akhmelnitsky@vmware.com>

annakhm force-pushed the antreaipam-preallocate branch from 996ec5e to 00dfb7e Compare March 25, 2022 21:42

tnqn approved these changes Mar 26, 2022

View reviewed changes

tnqn merged commit 739b8a1 into antrea-io:main Mar 26, 2022

tnqn added the action/release-note Indicates a PR that should be included in release notes. label Mar 28, 2022

[Antrea-IPAM] Support pre-allocating continuous IPs for StatefulSet #3281

[Antrea-IPAM] Support pre-allocating continuous IPs for StatefulSet #3281

Conversation

annakhm commented Feb 8, 2022

codecov-commenter commented Feb 8, 2022 • edited

Codecov Report

annakhm commented Feb 16, 2022

gran-vmv left a comment

Choose a reason for hiding this comment

gran-vmv commented Feb 17, 2022

annakhm commented Feb 18, 2022

gran-vmv commented Feb 18, 2022

annakhm commented Feb 18, 2022

gran-vmv left a comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

annakhm commented Mar 23, 2022

gran-vmv commented Mar 24, 2022

gran-vmv commented Mar 24, 2022

gran-vmv commented Mar 24, 2022

gran-vmv left a comment

Choose a reason for hiding this comment

gran-vmv commented Mar 24, 2022

gran-vmv commented Mar 24, 2022

gran-vmv commented Mar 24, 2022

tnqn Mar 24, 2022 • edited

Choose a reason for hiding this comment

annakhm commented Mar 24, 2022

annakhm commented Mar 24, 2022

annakhm commented Mar 24, 2022

jianjuns Mar 24, 2022

Choose a reason for hiding this comment

jianjuns Mar 24, 2022

Choose a reason for hiding this comment

annakhm Mar 24, 2022 • edited

Choose a reason for hiding this comment

jianjuns Mar 24, 2022

Choose a reason for hiding this comment

annakhm Mar 24, 2022

Choose a reason for hiding this comment

annakhm Mar 24, 2022

Choose a reason for hiding this comment

jianjuns Mar 25, 2022

Choose a reason for hiding this comment

annakhm Mar 25, 2022

Choose a reason for hiding this comment

annakhm commented Mar 25, 2022

annakhm commented Mar 25, 2022

tnqn left a comment

Choose a reason for hiding this comment

annakhm commented Mar 25, 2022

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Mar 26, 2022

codecov-commenter commented Feb 8, 2022 •

edited

tnqn Mar 24, 2022 •

edited

annakhm Mar 24, 2022 •

edited