IPPool counters #3072

ksamoray · 2021-12-01T08:58:15Z

Add IPPools usage counters, and expose them via CRD.

Signed-off-by: Kobi Samoray ksamoray@vmware.com

codecov-commenter · 2021-12-01T13:31:15Z

Codecov Report

Merging #3072 (1d4e269) into main (231b09d) will increase coverage by 2.88%.
The diff coverage is 77.02%.

@@            Coverage Diff             @@
##             main    #3072      +/-   ##
==========================================
+ Coverage   64.40%   67.28%   +2.88%     
==========================================
  Files         293      297       +4     
  Lines       43658    45365    +1707     
==========================================
+ Hits        28116    30526    +2410     
+ Misses      13253    12461     -792     
- Partials     2289     2378      +89

Flag	Coverage Δ
e2e-tests	`40.66% <5.40%> (?)`
integration-tests	`36.06% <0.00%> (?)`
kind-e2e-tests	`50.39% <77.02%> (-0.37%)`	⬇️
unit-tests	`44.33% <77.02%> (+0.16%)`	⬆️

Impacted Files	Coverage Δ
pkg/controller/ipam/antrea_ipam_controller.go	`77.89% <73.84%> (+1.47%)`	⬆️
pkg/controller/externalippool/controller.go	`91.51% <100.00%> (+6.69%)`	⬆️
pkg/ipam/ipallocator/allocator.go	`88.14% <100.00%> (-0.40%)`	⬇️
pkg/ipam/poolallocator/allocator.go	`55.71% <100.00%> (+7.52%)`	⬆️
...s/multicluster/memberclusterannounce_controller.go	`56.64% <0.00%> (-15.16%)`	⬇️
pkg/agent/multicast/mcast_controller.go	`44.92% <0.00%> (-7.02%)`	⬇️
pkg/util/k8s/node.go	`80.90% <0.00%> (-6.72%)`	⬇️
pkg/agent/multicast/mcast_discovery.go	`56.44% <0.00%> (-5.17%)`	⬇️
pkg/controller/networkpolicy/tier.go	`50.00% <0.00%> (-5.00%)`	⬇️
pkg/agent/controller/networkpolicy/reject.go	`82.48% <0.00%> (-3.39%)`	⬇️
... and 70 more

annakhm · 2021-12-01T21:50:27Z

pkg/apis/crd/v1alpha2/types.go

+	Usage       IPPoolUsage      `json:"usage,omitempty"`
+}
+
+type IPPoolUsage struct {


Do you think we can combine this type with ExternalIPPoolUsage?

Technically this is doable of course. IDK enough the external pool functionality to conclude whether these two feature will have the same counters forever.
@tnqn do you have an opinion?

I think it's a good suggestion. They should be same from pool's perspective, I can't think of any difference between the two cases for now.
I'm fine with making ExternalIPPoolStatus use IPPoolUsage if it doesn't impact API compatibility.

tnqn · 2021-12-14T11:23:43Z

pkg/apis/crd/v1alpha2/types.go

+	Usage       IPPoolUsage      `json:"usage,omitempty"`
+}
+
+type IPPoolUsage struct {


I think it's a good suggestion. They should be same from pool's perspective, I can't think of any difference between the two cases for now.
I'm fine with making ExternalIPPoolStatus use IPPoolUsage if it doesn't impact API compatibility.

tnqn · 2021-12-14T11:33:04Z

pkg/ipam/poolallocator/allocator.go

@@ -132,6 +132,9 @@ func (a *IPPoolAllocator) appendPoolUsage(ipPool *v1alpha2.IPPool, ip net.IP, st
 	}

 	newPool.Status.IPAddresses = append(newPool.Status.IPAddresses, usageEntry)
+	// CRD is not updated yet, therefore the newly allocated IP will not be reflected by Used(). Therefore +1
+	newPool.Status.Usage.Used = a.Used() + 1


Why don't we use len(newPool.Status.IPAddresses)?

tnqn · 2021-12-14T11:33:20Z

pkg/ipam/poolallocator/allocator.go

@@ -160,6 +163,10 @@ func (a *IPPoolAllocator) removePoolUsage(ipPool *v1alpha2.IPPool, ip net.IP) er

 	newPool.Status.IPAddresses = newList

+	// CRD is not updated yet, therefore the newly deleted allocation will not be reflected by Used(). Therefore -1
+	newPool.Status.Usage.Used = a.Used() - 1


tnqn · 2021-12-14T11:36:17Z

pkg/ipam/poolallocator/allocator.go

@@ -384,3 +391,21 @@ func (a *IPPoolAllocator) HasContainer(containerID string) (bool, error) {
 	}
 	return false, nil
 }
+
+func (a IPPoolAllocator) Used() int {


Maybe this is not needed if we just use len(newPool.Status.IPAddresses).

tnqn · 2021-12-14T11:45:04Z

pkg/ipam/poolallocator/allocator.go

+	_, allocators, _ := a.readPoolAndInitIPAllocators()
+	total := 0
+	for _, allocator := range allocators {
+		total += allocator.Total()


Instead of reading the pool and init the allocators another time, could we use the existing allocators to get Total directly, maybe pass it as an argument of appendPoolUsage and removePoolUsage? And I think you have added ToTal method to MultiIPAllocator, we could call it directly?

annakhm · 2021-12-20T21:51:26Z

pkg/controller/ipam/metrics.go

+}
+
+func (c *AntreaIPAMMetricsHandler) Run(stopCh <-chan struct{}) {
+	c.ipPoolInformer.Informer().AddEventHandlerWithResyncPeriod(cache.ResourceEventHandlerFuncs{


nit: I notice many places in the code use AddEventHandlerWithResyncPeriod while supplying 0 for resyncPeriod, but I think you can just use AddEventHandler without the resyncPeriod parameter

annakhm · 2021-12-20T21:53:39Z

pkg/controller/ipam/metrics.go

+func (c *AntreaIPAMMetricsHandler) updateIPPoolCounters(obj interface{}) {
+	ipPool := obj.(*crdv1a2.IPPool)
+
+	allocator, err := poolallocator.NewIPPoolAllocator(ipPool.Name, c.crdClient)


This will need to change a bit once #3046 is merged (pool informer is already included in this class so not a big deal)

So maybe it's better if I rebase on top of #3046

annakhm · 2021-12-20T21:55:28Z

pkg/controller/ipam/metrics.go

+	total := allocator.Total()
+
+	// Used is gathered from IP allocation status within the CRD - as it can be set by each one of the agents
+	used := len(ipPool.Status.IPAddresses)


Here I'm not sure whether we want to count reserved addresses? Those would be the addresses reserved for StatefulSet but not actually allocated for Pods.

Makes sense that we count them. These numbers should reflect the usage, whether it's a reserved or allocated for a Pod.
Otherwise total minus used won't reflect the available IPs in the pool, right?

annakhm · 2021-12-20T21:57:55Z

pkg/controller/ipam/metrics.go

+	if pool.Status.Usage.Used == used && pool.Status.Usage.Total == total {
+		return
+	}
+	pool.Status.Usage.Used = used


Would it make more sense to update these counters along with other Status updates (when addresses are actually allocated/released), rather then asynchronously here? This would also save the need for an extra CRD update.

I tried this actually, here is the problems though:
If I update the total number in the other status update, it won't catch IPPool creation or updates when an IPRange is added. So for the total it makes sense to use an informer.
As for updating the used count, I could indeed place it where the other updates are done. It will clutter the code though a bit.
Another option is using a mutating webhook, I didn't try it though.
What do you think?

Yes, this makes sense to me, as long as we perform the update here only if totals are changed.

I guess mutating webhook doesn't work here because "PUT/POST/PATCH requests to the custom resource ignore changes to the status stanza." Spec and Status cannot be updated via same request.
At least for spec update, asynchronous counter update might be the only way.

But does it need to introduce a metrics controller to do this? The name sounds it's for prometheus metrics. We don't expose other counters as prometheus metrics and no real requirements are asking for it, and it's kind of duplicated with the IPPool status. We already have a AntreaIPAMController which basically handles the IPPool Status stuff, I wonder we should just add the logic there.

@tnqn the change does expose Prometheus metrics as well as the stats via CRD. As this PR has been around for while, I can't recall why it's so. I can remove this.
This change has to be revised, maybe discard the Prometheus code as you suggest.

ksamoray · 2022-03-03T07:27:40Z

@annakhm @tnqn is this code still relevant? Or should I abandon that change?

annakhm · 2022-03-03T17:37:45Z

I think its relevant, yes

gran-vmv · 2022-03-31T01:40:21Z

build/yamls/antrea-kind.yml

@@ -0,0 +1,3585 @@
+apiVersion: apiextensions.k8s.io/v1


It seems this file should be removed.

Indeed, thanks!

github-actions · 2022-07-03T00:41:43Z

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment, or this will be closed in 90 days

tnqn · 2022-07-28T13:13:57Z

pkg/controller/ipam/antrea_ipam_controller.go

+	if err != nil {
+		klog.Warningf("Failed to initialize allocator for IPPool %s", ipPool.Name)
+	}


The code shouldn't continue to run if err is not nil, otherwise it would panic

tnqn · 2022-07-28T13:26:59Z

pkg/controller/ipam/antrea_ipam_controller.go

+	pool.Status.Usage.Used = used
+	pool.Status.Usage.Total = total
+
+	_, err = c.crdClient.CrdV1alpha2().IPPools().UpdateStatus(context.TODO(), pool, metav1.UpdateOptions{})


event handlers are executed sequentially, doing all jobs here may not scale well, especially when there is an external API call.

Every update to a pool will call the event handler once, doing all jobs here is not as effecient as doing it in workers, as multiple updates to a pool will only trigger one processing if the time the events happen is close.
A more typical and efficient way is:
Event handlers extract the pool's key and enqueue it; multiple workers get keys from the queue and do the actual job.

tnqn · 2022-08-01T13:55:44Z

pkg/controller/ipam/antrea_ipam_controller.go

+	if ipPool.Status.Usage.Used == used && ipPool.Status.Usage.Total == total {
+		return nil
+	}
+	ipPool.Status.Usage.Used = used


Objects returned here must be treated as read-only as they are shared among all consumers.

ipPoolToUpdate := ipPool.DeepCopy() ipPoolToUpdate.Status.Usage.Used = used ...

tnqn · 2022-08-01T13:58:00Z

pkg/controller/ipam/antrea_ipam_controller.go

+	c.statusQueue.Add(ipPool.Name)
+}
+
+func (c *AntreaIPAMController) updateHandler(oldObj, newObj interface{}) {


it's not registered to informer

tnqn

Code LGTM, but was confused by the new test

tnqn · 2022-08-02T06:49:12Z

pkg/ipam/poolallocator/allocator_test.go

+// This test verifies correct behavior in case of update conflict. Allocation should be retiried
+// Taking into account the latest status
+func TestAllocateConflict(t *testing.T) {


I don't find how the test simulate a case of update conflict and how it is different from TestAllocateNext: there is one line difference about whether check allocator is nil

Seems like a merge error, that test was around in prior revisions in allocator_test.go. I'll remove it.

Add IPPools usage counters, and expose them via CRD. Signed-off-by: Kobi Samoray <ksamoray@vmware.com>

tnqn

LGTM

tnqn · 2022-08-02T10:04:33Z

/test-all

tnqn · 2022-08-02T12:26:34Z

/test-e2e

tnqn · 2022-08-02T13:51:50Z

/skip-e2e which failed on a known flaky test ACNPFQDNPolicy

ksamoray force-pushed the ipam-stats branch from ee531d8 to 6b51d28 Compare December 1, 2021 12:25

ksamoray requested review from annakhm, salv-orlando and gran-vmv December 1, 2021 15:25

annakhm reviewed Dec 1, 2021

View reviewed changes

ksamoray force-pushed the ipam-stats branch 3 times, most recently from e42b216 to 02fffe9 Compare December 14, 2021 09:10

tnqn reviewed Dec 14, 2021

View reviewed changes

ksamoray force-pushed the ipam-stats branch 3 times, most recently from 8deb058 to 3010632 Compare December 19, 2021 15:33

ksamoray requested review from annakhm and tnqn December 20, 2021 07:44

annakhm reviewed Dec 20, 2021

View reviewed changes

ksamoray force-pushed the ipam-stats branch from 3010632 to 3442f71 Compare March 9, 2022 07:43

ksamoray force-pushed the ipam-stats branch from 3442f71 to 3377e10 Compare March 30, 2022 14:18

gran-vmv reviewed Mar 31, 2022

View reviewed changes

ksamoray force-pushed the ipam-stats branch from 3377e10 to 1d04c8b Compare April 3, 2022 06:20

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 3, 2022

tnqn added this to the Antrea v1.8 release milestone Jul 5, 2022

tnqn added api-review Categorizes an issue or PR as actively needing an API review. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 5, 2022

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

ksamoray force-pushed the ipam-stats branch 3 times, most recently from b98890e to b04c91d Compare July 21, 2022 19:12

tnqn reviewed Jul 28, 2022

View reviewed changes

ksamoray force-pushed the ipam-stats branch from b04c91d to 0fa4972 Compare August 1, 2022 11:44

tnqn reviewed Aug 1, 2022

View reviewed changes

ksamoray force-pushed the ipam-stats branch from 0fa4972 to 6d349b3 Compare August 1, 2022 14:37

tnqn added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 2, 2022

tnqn reviewed Aug 2, 2022

View reviewed changes

IPPool counters

1d4e269

Add IPPools usage counters, and expose them via CRD. Signed-off-by: Kobi Samoray <ksamoray@vmware.com>

ksamoray force-pushed the ipam-stats branch from 6d349b3 to 1d4e269 Compare August 2, 2022 07:48

tnqn approved these changes Aug 2, 2022

View reviewed changes

tnqn merged commit b66ffda into antrea-io:main Aug 2, 2022

ksamoray deleted the ipam-stats branch August 2, 2022 15:34

tnqn mentioned this pull request Aug 5, 2022

Unit test TestStatefulSetLifecycle is very flaky #4081

Closed

IPPool counters #3072

IPPool counters #3072

Conversation

ksamoray commented Dec 1, 2021

codecov-commenter commented Dec 1, 2021 • edited by codecov bot Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ksamoray commented Mar 3, 2022

annakhm commented Mar 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 2, 2022

tnqn commented Aug 2, 2022

tnqn commented Aug 2, 2022

codecov-commenter commented Dec 1, 2021 •

edited by codecov bot

Loading