IP ranges cleanup #118

maiqueb · 2021-07-05T17:05:52Z

This PR adds a cronjob that runs every 5 minutes, and checks the consistency of the IP addresses currently assigned by the pools.

Depends-on: #113

TODO:

~~retrieve a list of Pods once instead of checking N times if pods exist~~
ensure the ip-reconciler runs at most a little bit less that the provided cron expression
missing unit tests ~~(I'd have to mock the datastore, and I didn't want to establish that precedent...)~~ but it is tested e2e
- ~~I'll rework the public API I'm proposing so I can unit test this without requiring mocking. I've got a fairly good idea on how to do it~~
- the "find orphaned" part misses unit tests, since I'd need to mock the datastore. Refactoring the code to enable that will require plenty of changes, which'll make this PR harder to review; imho, it can happen in a follow up.

coveralls · 2021-07-05T17:11:12Z

Pull Request Test Coverage Report for Build 1056425921

66 of 207 (31.88%) changed or added relevant lines in 6 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+11.4%) to 40.866%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/storage/etcd.go	0	6	0.0%
pkg/reconciler/wrappedPod.go	32	40	80.0%
pkg/allocate/allocate.go	0	10	0.0%
cmd/reconciler/ip.go	0	28	0.0%
pkg/reconciler/iploop.go	32	121	26.45%

Files with Coverage Reduction	New Missed Lines	%
pkg/allocate/allocate.go	1	66.5%

Totals
Change from base Build 1052931641:	11.4%
Covered Lines:	434
Relevant Lines:	1062

💛 - Coveralls

crandles · 2021-07-05T17:31:32Z

Sorry! reviewing this while mobile

maiqueb · 2021-07-05T17:34:55Z

cmd/reconciler/ip_test.go

+				BeforeEach(func() {
+					Expect(k8sClientSet.CoreV1().Pods(namespace).Delete(pod.Name, &metav1.DeleteOptions{})).NotTo(HaveOccurred())
+				})


Now that I think about it, I probably shouldn't even start up the pod; just the pool pointing at it.

maiqueb · 2021-07-06T15:45:34Z

Hey; any clue on what went wrong generating the openshift image, @dougbtv ? https://github.com/k8snetworkplumbingwg/whereabouts/pull/118/checks?check_run_id=2992364690

lgtm-com · 2021-07-12T11:13:48Z

This pull request introduces 1 alert when merging 731e1ff into f1047ee - view on LGTM.com

new alerts:

1 for Useless assignment to local variable

dougbtv

Miguel, my hat is off to you. This is incredible. I'm ready to move forward with it. I have a nit for a doc update. Let's see if we can get you another review too. But, I have to say I'm particularly happy with this PR, major thanks.

dougbtv · 2021-07-14T20:20:53Z

pkg/reconciler/iploop_test.go

+	RunSpecs(t, "Reconcile IP address allocation in the system")
+}
+
+// mock the pool


Nice, I like the mock pool technique, great one.

doc/ip-reconciler-job.yaml

TothFerenc · 2021-07-14T22:06:54Z

pkg/reconciler/iploop.go

+
+func (rl ReconcileLooper) isPodAlive(podRef string) bool {
+	for _, livePodRef := range rl.livePodRefs {
+		if podRef == livePodRef {


Need to check whether the running Pod really owns the reserved IP.

StatefulSet Pods keep their names whenever they are recreated, so it might happen that a StatefulSet Pod got new IP in the meantime, while previous IP is still reserved in IPPool pointing to the same running Pod, so the previous IP is unused, but won't be released by this logic.

StatefulSet Pods keep their names whenever they are recreated, so it might happen that a StatefulSet Pod got new IP in the meantime, while previous IP is still reserved in IPPool pointing to the same running Pod, so the previous IP is unused, but won't be released by this logic.

Really good point, I hadn't considered that scenario - at all. Thanks for raising it.

I've added a couple of commits to handle this particular scenario - the implementation I'm proposing only tackles secondary interfaces (plugged by multus).

Hope this limitation is not a deal breaker for you.

I'll account for @dougbtv 's comments next, and also document the reconciler's limitations described above.

EDIT: nudging for attention @TothFerenc

To preserve the existing code base, we embed the new type within the KubernetesIPAM (old type). In a follow-up commit, this new struct will be used on a separate golang binary (which doesn't require the KubernetesIPAM configuration), to provide a simple connection to the Kubernetes backend (does not support etcd). Signed-off-by: Miguel Duarte Barroso <[email protected]>

Fow now, we only read the kubeconfig file, which is specified via the cmd line - as its only argument. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Introduce a dedicated constructor in the storage pkg to create the newly created `KubernetesClient` struct. The existing `KubernetesIPAM` struct will re-use this new constructor. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Follow-up commits will use this new method for debugging, which makes it simpler to track the IP to pod assignments. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Currently, matching the owner of the IP when deallocating the address happens only based on the containerID. In follow-up commits we will require this behavior to happen based on the pod reference attribute. This commit prepares the code base for that, by enabling the caller to inject the criteria for identifying the owner of an IP into the IP address de-allocation function. Signed-off-by: Miguel Duarte Barroso <[email protected]>

This behavior will be encapsulated in a new struct named `ReconcileLooper`, which features methods for: - CancelContext; cancels the current execution context - FindOrphanedIPsPerPool; returns a list of the orphaned IP addresses, indexed by pool - ReconcileIPPool; receives a list of orphaned IP addresses, and cleans them up from the respective pools where they are currently allocated. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

The service account for whereabouts requires the ability of listing the pods on all namespaces, so it can check which ones feature orphaned IPs. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

Only mark a pod as "alive" when the pod's annotations feature the IP being de-allocated. This makes the reconciler binary *dependent* on multus, which adds these `network-status` annotations into the pod. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

By not returning an `IPReservation` entry - we now return an IP address instead - we simplify the code. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Signed-off-by: Miguel Duarte Barroso <[email protected]>

This commit changes the reconciler logic to only parse the network-status annotations of whereabouts pods - e.g. pods whose pod referecences (<namespace>/<pod name>) feature in any of the whereabouts IPPools. Signed-off-by: Miguel Duarte Barroso <[email protected]>

dougbtv

Awesome one. Working great in my environment. Thank you Miguel!

crandles closed this Jul 5, 2021

crandles reopened this Jul 5, 2021

maiqueb commented Jul 5, 2021

View reviewed changes

maiqueb force-pushed the ip-ranges-cleanup branch from 4b2db21 to 731e1ff Compare July 12, 2021 11:03

maiqueb force-pushed the ip-ranges-cleanup branch from 731e1ff to d9361ad Compare July 12, 2021 12:13

dougbtv requested changes Jul 14, 2021

View reviewed changes

TothFerenc reviewed Jul 14, 2021

View reviewed changes

maiqueb force-pushed the ip-ranges-cleanup branch from 84966c3 to 0b2d5c2 Compare July 19, 2021 12:21

maiqueb requested a review from dougbtv July 19, 2021 13:00

maiqueb added 5 commits July 21, 2021 17:36

ip-reconciler: add new binary

ac7507e

Fow now, we only read the kubeconfig file, which is specified via the cmd line - as its only argument. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Generate the Kubernetes client object

7d70fb8

Introduce a dedicated constructor in the storage pkg to create the newly created `KubernetesClient` struct. The existing `KubernetesIPAM` struct will re-use this new constructor. Signed-off-by: Miguel Duarte Barroso <[email protected]>

Have IPReservation implement the Stringer interface

edf1bf5

Follow-up commits will use this new method for debugging, which makes it simpler to track the IP to pod assignments. Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb force-pushed the ip-ranges-cleanup branch 4 times, most recently from 385e34e to fa3455d Compare July 22, 2021 13:56

maiqueb added 8 commits July 22, 2021 16:05

reconciler: add e2e tests

8f73197

Signed-off-by: Miguel Duarte Barroso <[email protected]>

ip-reconciler: fire up cronjob every 5 mins

94c6bd1

The service account for whereabouts requires the ability of listing the pods on all namespaces, so it can check which ones feature orphaned IPs. Signed-off-by: Miguel Duarte Barroso <[email protected]>

ip-reconciler: add unit tests

2f3f64e

Signed-off-by: Miguel Duarte Barroso <[email protected]>

ip-reconciler: add plumbers network status entity

90c7617

Signed-off-by: Miguel Duarte Barroso <[email protected]>

ip-reconciler, docs: mention the reconciler in the README

5ed98e5

Signed-off-by: Miguel Duarte Barroso <[email protected]>

ip-reconciler: simplify ip looper code

66b75ad

By not returning an `IPReservation` entry - we now return an IP address instead - we simplify the code. Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb added 3 commits July 22, 2021 16:07

reconcile cluster wide IPs

0154617

Signed-off-by: Miguel Duarte Barroso <[email protected]>

e2e test overlapping IPs

5f160d8

Signed-off-by: Miguel Duarte Barroso <[email protected]>

maiqueb force-pushed the ip-ranges-cleanup branch from fa3455d to d1b0bf0 Compare July 22, 2021 14:08

dougbtv approved these changes Jul 22, 2021

View reviewed changes

dougbtv merged commit 0fa6f42 into k8snetworkplumbingwg:master Jul 22, 2021

maiqueb mentioned this pull request Aug 4, 2021

fix: Skip default network in ip reconciler #128

Merged

maiqueb mentioned this pull request Nov 10, 2021

ip-reconciler race condition with Whereabouts, leads to IP cleanup and duplicate IPs #162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IP ranges cleanup #118

IP ranges cleanup #118

maiqueb commented Jul 5, 2021 •

edited

Loading

coveralls commented Jul 5, 2021 •

edited

Loading

crandles commented Jul 5, 2021

maiqueb Jul 5, 2021

maiqueb commented Jul 6, 2021

lgtm-com bot commented Jul 12, 2021

dougbtv left a comment

dougbtv Jul 14, 2021

TothFerenc Jul 14, 2021 •

edited

Loading

maiqueb Jul 15, 2021 •

edited

Loading

dougbtv left a comment

IP ranges cleanup #118

IP ranges cleanup #118

Conversation

maiqueb commented Jul 5, 2021 • edited Loading

coveralls commented Jul 5, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1056425921

💛 - Coveralls

crandles commented Jul 5, 2021

maiqueb Jul 5, 2021

Choose a reason for hiding this comment

maiqueb commented Jul 6, 2021

lgtm-com bot commented Jul 12, 2021

dougbtv left a comment

Choose a reason for hiding this comment

dougbtv Jul 14, 2021

Choose a reason for hiding this comment

TothFerenc Jul 14, 2021 • edited Loading

Choose a reason for hiding this comment

maiqueb Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

dougbtv left a comment

Choose a reason for hiding this comment

maiqueb commented Jul 5, 2021 •

edited

Loading

coveralls commented Jul 5, 2021 •

edited

Loading

TothFerenc Jul 14, 2021 •

edited

Loading

maiqueb Jul 15, 2021 •

edited

Loading