Implement ExternalIPPool status #2490

wenqiq · 2021-07-29T09:35:16Z

Implement ExternalIPPool status

Report the number of allocated versus free IPs to the status of the Egress ExternalIPPool.

For example:
kubectl get eip

kubectl get eip 
NAME            TOTAL   USED   AGE
eip-test-ipv4   345     1      20s

kubectl get eip -o yaml

  status:
    usage:
      total: 91
      used: 1

kubectl get eip -o yaml                     
apiVersion: v1
items:
- apiVersion: crd.antrea.io/v1alpha2
  kind: ExternalIPPool
  metadata:
    creationTimestamp: "2021-08-02T17:48:17Z"
    generation: 1
    name: eip-test-ipv4
    resourceVersion: "116090"
    uid: fbc8b2ac-a147-4937-b842-88ce6e261a33
  spec:
    ipRanges:
    - end: 10.10.10.100
      start: 10.10.10.10
    nodeSelector:
      matchExpressions:
      - key: kubernetes.io/hostname
        operator: In
        values:
        - kind-worker2
  status:
    usage:
      total: 91
      used: 1
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""

For #2454

antoninbas

My initial suggestion (and the scope of #2454) was to report usage data (number of allocated IPs vs total number of IPs). It seems that this PR wants to return a list of allocated IPs, which could lead to a significantly large Status. Is this PR trying to address something else besides #2454? It seems that it is missing some context, as there is no explanation of what Preallocated is compared to Allocated.

wenqiq · 2021-07-30T02:00:34Z

My initial suggestion (and the scope of #2454) was to report usage data (number of allocated IPs vs total number of IPs). It seems that this PR wants to return a list of allocated IPs, which could lead to a significantly large Status.

I don't know whether it needs to be consistent with IPPool status describing in #2442?

I also think reporting usage data (number of allocated IPs vs total number of IPs) is better in ExternalIPPool status, this avoids a significantly large Status.

It seems that it is missing some context, as there is no explanation of what Preallocated is compared to Allocated.

‘Preallocated’ represents the IP allocated to Egress but not assigned to Node yet.

‘Allocated’ represents the IP allocated to Egress and has been assigned to Node.

If we report the status with a list of allocated IPs.

Is this PR trying to address something else besides #2454?

This PR is based on #2444, I was thinking rebase after merged, however they don't actually have dependencies. I will separate these two commits.

codecov-commenter · 2021-07-30T04:04:37Z

Codecov Report

Merging #2490 (dfe2676) into main (7f90fc0) will decrease coverage by 0.01%.
The diff coverage is 59.18%.

@@            Coverage Diff             @@
##             main    #2490      +/-   ##
==========================================
- Coverage   42.00%   41.98%   -0.02%     
==========================================
  Files         153      154       +1     
  Lines       18565    18658      +93     
==========================================
+ Hits         7798     7834      +36     
- Misses      10067    10118      +51     
- Partials      700      706       +6

Flag	Coverage Δ
unit-tests	`41.98% <59.18%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/controller/egress/ipallocator/allocator.go	`80.41% <0.00%> (-2.57%)`	⬇️
pkg/controller/egress/controller.go	`84.12% <63.04%> (-3.08%)`	⬇️
pkg/ovs/openflow/ofctrl_packetout.go	`66.96% <0.00%> (-0.61%)`	⬇️
pkg/ovs/openflow/ofctrl_builder.go	`18.34% <0.00%> (-0.27%)`	⬇️
pkg/ovs/openflow/ofctrl_action.go	`11.25% <0.00%> (-0.20%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`4.63% <0.00%> (-0.05%)`	⬇️
pkg/ovs/openflow/ofctrl_group.go	`0.00% <0.00%> (ø)`
pkg/ovs/openflow/ofctrl_nxfields.go	`12.50% <0.00%> (ø)`
pkg/agent/openflow/pipeline.go	`24.62% <0.00%> (+0.03%)`	⬆️
pkg/antctl/command_definition.go	`53.97% <0.00%> (+0.11%)`	⬆️
... and 2 more

tnqn · 2021-07-30T08:58:45Z

pkg/controller/egress/controller.go

 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"

 	"antrea.io/antrea/pkg/apis/controlplane"
-	egressv1alpha2 "antrea.io/antrea/pkg/apis/crd/v1alpha2"
+	. "antrea.io/antrea/pkg/apis/crd/v1alpha2"


This is not an encouraged usage, which misleads people that the types are in same package. Package qualifier is very useful to indicate where the dependency is. The code shouldn't pretend they are in same package which makes dividing them in different packages pointless.

tnqn · 2021-07-30T09:35:45Z

‘Preallocated’ represents the IP allocated to Egress but not assigned to Node yet.

‘Allocated’ represents the IP allocated to Egress and has been assigned to Node.

This will then involve more components to achieve it and need a chain of reactions but I don't see much value it adds to the API. We already have enough information in Egress API about whether the Egress IP is just allocated or is configured on a Node.
My original thinking of the status is also just number of allocated IPs vs total number of IPs, which helps users to understand how many IPs have been used and whether they need to extend the pool.

wenqiq · 2021-08-03T01:06:31Z

As we discussed above, there is no need to keep the ExternalIPPool status consistent with IPPool status CRD. So I implemented the ExternalIPPool status with a new method which only reporting usage data (number of allocated IPs and total number of IPs).

tnqn · 2021-08-10T16:51:03Z

build/yamls/base/crds.yml

+                  type: object
+                  required:
+                    - used
+                    - total


question: if they are set to "required", does it mean we must set it even when creating a new pool?

Thanks for feedbacks. I have verified that it can work and create a ExternalIPPool successfully, It seems status.usage is not required.

tnqn · 2021-08-10T17:00:11Z

pkg/controller/egress/controller.go

@@ -311,6 +315,37 @@ func (c *EgressController) setIPAllocation(egressName string, ip net.IP, poolNam
 	}
 }

+func (c *EgressController) updateExternalIPPoolStatus(poolName string, total, used int) error {


This method can be called concurrently in pool's eventHandler and egress's workers. So it seems that a race condition can make they override each other and the final value in API may be wrong. Maybe creating multiple Egresses can reproduce it.
If this is true, I would suggest have dedicated workers to sync pool's status, other places only enqueue a pool that needs to sync.

Good consideration. Will reproduce and fix it.

tnqn

LGTM overall, some minor comments

tnqn · 2021-08-11T15:53:40Z

pkg/controller/egress/controller.go

@@ -92,6 +95,8 @@ type EgressController struct {
 	egressGroupStore storage.Interface
 	// queue maintains the EgressGroup objects that need to be synced.
 	queue workqueue.RateLimitingInterface
+	// queueEIP maintains the ExternalIPpool objects that need to be synced.
+	queueEIP workqueue.RateLimitingInterface


It's more natural to call it eipQueue, but I think poolQueue is better.

tnqn · 2021-08-11T16:05:04Z

test/e2e/egress_test.go

@@ -311,6 +311,10 @@ func testEgressCRUD(t *testing.T, data *TestData) {
 				require.NoError(t, err, "Failed to check if IP exists on Node")
 				assert.False(t, exists, "Found stale IP on Node")
 			}
+			pool, err = data.crdClient.CrdV1alpha2().ExternalIPPools().Get(context.TODO(), pool.Name, metav1.GetOptions{})
+			assert.NoError(t, err)


Suggested change

assert.NoError(t, err)

require.NoError(t, err)

tnqn · 2021-08-11T16:06:43Z

test/e2e/egress_test.go

+			pool, err = data.crdClient.CrdV1alpha2().ExternalIPPools().Get(context.TODO(), pool.Name, metav1.GetOptions{})
+			assert.NoError(t, err)
+			assert.Equal(t, 1, pool.Status.Usage.Used, "ExternalIPPool status length not match")
+			t.Logf("pool %s status: %v", pool.Name, pool.Status)


Suggested change

t.Logf("pool %s status: %v", pool.Name, pool.Status)

t.Logf("ExternalPool %s status: %v", pool.Name, pool.Status)

tnqn · 2021-08-11T16:07:31Z

test/e2e/egress_test.go

@@ -311,6 +311,10 @@ func testEgressCRUD(t *testing.T, data *TestData) {
 				require.NoError(t, err, "Failed to check if IP exists on Node")
 				assert.False(t, exists, "Found stale IP on Node")
 			}
+			pool, err = data.crdClient.CrdV1alpha2().ExternalIPPools().Get(context.TODO(), pool.Name, metav1.GetOptions{})
+			assert.NoError(t, err)
+			assert.Equal(t, 1, pool.Status.Usage.Used, "ExternalIPPool status length not match")


Maybe you can add expectedTotal to the table and check it too.

tnqn · 2021-08-11T16:08:41Z

pkg/controller/egress/controller.go

@@ -24,12 +24,14 @@ import (
 	"sync"
 	"time"

+	apimachineryerrors "k8s.io/apimachinery/pkg/api/errors"


nit: apierrors

tnqn · 2021-08-16T13:42:38Z

pkg/controller/egress/controller.go

@@ -92,6 +95,8 @@ type EgressController struct {
 	egressGroupStore storage.Interface
 	// queue maintains the EgressGroup objects that need to be synced.
 	queue workqueue.RateLimitingInterface
+	// poolQueue maintains the ExternalIPpool objects that need to be synced.


Suggested change

// poolQueue maintains the ExternalIPpool objects that need to be synced.

// poolQueue maintains the ExternalIPPool objects that need to be synced.

build/yamls/base/crds.yml

tnqn

LGTM. @antoninbas do you want to take a look since you suggested the enhancement?

tnqn · 2021-08-18T02:10:45Z

/test-all

antoninbas

LGTM, thanks for working on this

antoninbas · 2021-08-18T02:38:32Z

pkg/apis/crd/v1alpha2/types.go

+	// The current status of the ExternalIPPool.
+	Status ExternalIPPoolStatus `json:"status"`


@tnqn we don't need omitempty for this? I know that when kubebuilder generates CRD types, it always adds the omitempty tag for the Status field.

I also see it for upstream resources (at least for Pod)

It seems that there is no omitempty tag for all the Status field in the project. I think we can take another PR to implement it? @antoninbas @tnqn

Although it's set for many Status fields of native Kubernetes resources, I just found it doesn't have any impact as long as the field is a customized struct. I tried marshalling an empty ExternalIPPool, it got the same result regardless of omitempty is set:

{ "metadata": { "creationTimestamp": null }, "spec": { "ipRanges": null, "nodeSelector": {} }, "status": { "usage": { "total": 0, "used": 0 } } }

According to https://golang.google.cn/pkg/encoding/json/#Marshal, it works for the built-in types only.

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

Thanks for confirming guys.

pkg/controller/egress/controller.go

antoninbas · 2021-08-18T02:41:48Z

pkg/controller/egress/controller_test.go

+			if gotEgress.Spec.ExternalIPPool != "" && gotEgress.Spec.EgressIP != "" {
+				poolName := gotEgress.Spec.ExternalIPPool
+				eip, err := controller.crdClient.CrdV1alpha2().ExternalIPPools().Get(context.TODO(), poolName, metav1.GetOptions{})
+				assert.NoError(t, err)


Suggested change

assert.NoError(t, err)

require.NoError(t, err)

assert will continue with the test, even though eip will be nil in case of an error

antoninbas · 2021-08-18T02:42:21Z

pkg/controller/egress/controller_test.go

+				eip, err := controller.crdClient.CrdV1alpha2().ExternalIPPools().Get(context.TODO(), poolName, metav1.GetOptions{})
+				assert.NoError(t, err)
+				usage := eip.Status.Usage
+				assert.Equal(t, 1, usage.Used, fmt.Sprintf("ExternalIPPool status not match: %v", usage))


Suggested change

assert.Equal(t, 1, usage.Used, fmt.Sprintf("ExternalIPPool status not match: %v", usage))

assert.Equal(t, 1, usage.Used, "Expected one used IP in EgressIPPool Status")

antoninbas

LGTM

antoninbas · 2021-08-19T17:02:41Z

/test-all

tnqn · 2021-08-20T02:41:24Z

@wenqiq e2e failed:

=== RUN   TestEgress/testEgressCRUD
=== RUN   TestEgress/testEgressCRUD/single_matching_Node
    egress_test.go:320: 
        	Error Trace:	egress_test.go:320
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestEgress/testEgressCRUD/single_matching_Node
        	Messages:   	ExternalIPPool status usage used num not match
    egress_test.go:322: ExternalPool pool-tk58b status: {{2 0}}
=== RUN   TestEgress/testEgressCRUD/two_matching_Nodes
    egress_test.go:320: 
        	Error Trace:	egress_test.go:320
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestEgress/testEgressCRUD/two_matching_Nodes
        	Messages:   	ExternalIPPool status usage used num not match
    egress_test.go:322: ExternalPool pool-jdsn5 status: {{2 0}}
=== RUN   TestEgress/testEgressCRUD/no_matching_Node
    egress_test.go:320: 
        	Error Trace:	egress_test.go:320
        	Error:      	Not equal: 
        	            	expected: 1
        	            	actual  : 0
        	Test:       	TestEgress/testEgressCRUD/no_matching_Node
        	Messages:   	ExternalIPPool status usage used num not match

wenqiq · 2021-08-20T03:48:18Z

Fixed ExternalIPPool status checking logic order in Egress e2e test case.
/test-all

Report the number of allocated versus free IPs to the status of the Egress ExternalIPPool. For example: kubectl get eip ``` NAME TOTAL USED AGE eip-test-ipv4 345 1 20s ``` kubectl get eip -o yaml ``` ... items: - apiVersion: crd.antrea.io/v1alpha2 kind: ExternalIPPool ... spec: ipRanges: - end: 10.10.10.100 start: 10.10.10.10 status: usage: total: 91 used: 1 ... ``` Signed-off-by: Wenqi Qiu <wenqiq@vmware.com>

wenqiq · 2021-08-20T09:06:53Z

/test-all

tnqn

LGTM

antoninbas · 2021-08-20T16:59:52Z

All the e2e test failures are unrelated to this PR: see #2618

antoninbas reviewed Jul 29, 2021

View reviewed changes

wenqiq force-pushed the externalIPPool-status branch from 9f3b3cd to c722aeb Compare July 30, 2021 03:23

tnqn reviewed Jul 30, 2021

View reviewed changes

wenqiq force-pushed the externalIPPool-status branch 2 times, most recently from 862daa8 to 7defc3e Compare August 3, 2021 01:03

wenqiq marked this pull request as ready for review August 3, 2021 03:05

annakhm mentioned this pull request Aug 3, 2021

Introduce IPPool CRD and FlexibleIPAM feature gate #2442

Merged

tnqn reviewed Aug 10, 2021

View reviewed changes

wenqiq force-pushed the externalIPPool-status branch from 7defc3e to 4762b45 Compare August 11, 2021 09:22

tnqn reviewed Aug 11, 2021

View reviewed changes

wenqiq force-pushed the externalIPPool-status branch from 4762b45 to a27418f Compare August 16, 2021 02:14

tnqn reviewed Aug 16, 2021

View reviewed changes

wenqiq closed this Aug 17, 2021

wenqiq reopened this Aug 17, 2021

wenqiq force-pushed the externalIPPool-status branch from db3126f to fe38f58 Compare August 17, 2021 09:24

tnqn previously approved these changes Aug 18, 2021

View reviewed changes

antoninbas previously approved these changes Aug 18, 2021

View reviewed changes

wenqiq dismissed stale reviews from antoninbas and tnqn via fbd2754 August 18, 2021 03:22

antoninbas previously approved these changes Aug 19, 2021

View reviewed changes

wenqiq dismissed antoninbas’s stale review via dfe2676 August 20, 2021 03:44

wenqiq force-pushed the externalIPPool-status branch from dfe2676 to 235022d Compare August 20, 2021 04:38

tnqn approved these changes Aug 20, 2021

View reviewed changes

antoninbas merged commit 9ccddc0 into antrea-io:main Aug 20, 2021

antoninbas added this to the Antrea v1.3 release milestone Aug 20, 2021

	t.Logf("pool %s status: %v", pool.Name, pool.Status)
	t.Logf("ExternalPool %s status: %v", pool.Name, pool.Status)

	// poolQueue maintains the ExternalIPpool objects that need to be synced.
	// poolQueue maintains the ExternalIPPool objects that need to be synced.

		// The current status of the ExternalIPPool.
		Status ExternalIPPoolStatus `json:"status"`

	assert.Equal(t, 1, usage.Used, fmt.Sprintf("ExternalIPPool status not match: %v", usage))
	assert.Equal(t, 1, usage.Used, "Expected one used IP in EgressIPPool Status")

Implement ExternalIPPool status #2490

Implement ExternalIPPool status #2490

Conversation

wenqiq commented Jul 29, 2021 • edited

antoninbas left a comment

Choose a reason for hiding this comment

wenqiq commented Jul 30, 2021

codecov-commenter commented Jul 30, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn commented Jul 30, 2021

wenqiq commented Aug 3, 2021

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

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

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 18, 2021

antoninbas left a 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

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Aug 19, 2021

tnqn commented Aug 20, 2021

wenqiq commented Aug 20, 2021 • edited

wenqiq commented Aug 20, 2021

tnqn left a comment

Choose a reason for hiding this comment

antoninbas commented Aug 20, 2021

wenqiq commented Jul 29, 2021 •

edited

codecov-commenter commented Jul 30, 2021 •

edited

wenqiq commented Aug 20, 2021 •

edited