From b88f1297f9de9e3c3775ce914e384d1fdb43144f Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Mon, 16 Oct 2023 18:04:20 +0800 Subject: [PATCH] Fix a few corner cases when updating Egress conditions 1. Avoid generating a transient IPAssigned failure by differentiating scheduling failure from unprocessed case. 2. Fix duplicate IPAllocated conditions. 3. Add/update unit tests and e2e tests. Signed-off-by: Quan Tian --- .../controller/egress/egress_controller.go | 60 ++-- .../egress/egress_controller_test.go | 294 +++++++++++++----- pkg/agent/controller/egress/ip_scheduler.go | 14 +- .../controller/egress/ip_scheduler_test.go | 12 +- pkg/apis/crd/v1alpha2/types.go | 6 +- pkg/apis/crd/v1beta1/types.go | 6 +- pkg/apis/crd/v1beta1/util.go | 24 ++ pkg/controller/egress/controller.go | 14 +- pkg/controller/egress/controller_test.go | 138 ++++---- pkg/util/k8s/semantic.go | 26 ++ test/e2e/egress_test.go | 33 +- 11 files changed, 441 insertions(+), 186 deletions(-) create mode 100644 pkg/apis/crd/v1beta1/util.go create mode 100644 pkg/util/k8s/semantic.go diff --git a/pkg/agent/controller/egress/egress_controller.go b/pkg/agent/controller/egress/egress_controller.go index c98c55d8f7e..e91fa87cd54 100644 --- a/pkg/agent/controller/egress/egress_controller.go +++ b/pkg/agent/controller/egress/egress_controller.go @@ -23,7 +23,7 @@ import ( "sync" "time" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -614,7 +614,7 @@ func (c *EgressController) unbindPodEgress(pod, egress string) (string, bool) { return "", false } -func (c *EgressController) updateEgressStatus(egress *crdv1b1.Egress, egressIP string) error { +func (c *EgressController) updateEgressStatus(egress *crdv1b1.Egress, egressIP string, scheduleErr error) error { isLocal := false if egressIP != "" { isLocal = c.localIPDetector.IsLocalIP(egressIP) @@ -628,7 +628,7 @@ func (c *EgressController) updateEgressStatus(egress *crdv1b1.Egress, egressIP s desiredStatus.Conditions = []crdv1b1.EgressCondition{ { Type: crdv1b1.IPAssigned, - Status: v1.ConditionTrue, + Status: corev1.ConditionTrue, LastTransitionTime: metav1.Now(), Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode", @@ -637,6 +637,8 @@ func (c *EgressController) updateEgressStatus(egress *crdv1b1.Egress, egressIP s } } else if egressIP == "" { // Select one Node to update false status among all Nodes. + // We don't care about the value of egress.Spec.EgressIP, just use it to reach a consensus among all agents + // about which one should do the update. nodeToUpdateStatus, err := c.cluster.SelectNodeForIP(egress.Spec.EgressIP, "") if err != nil { return err @@ -647,33 +649,42 @@ func (c *EgressController) updateEgressStatus(egress *crdv1b1.Egress, egressIP s } desiredStatus.EgressNode = "" desiredStatus.EgressIP = "" - if isEgressSchedulable(egress) { + // If the error is nil, it means the Egress hasn't been processed yet. + // The scheduler will get a result for the Egress very soon regardless of success or failure and trigger the + // controller to process it another time, so we avoid generating a transient state here, which may lead to some + // back-off retries due to updating conflict. + if scheduleErr != nil { desiredStatus.Conditions = []crdv1b1.EgressCondition{ { Type: crdv1b1.IPAssigned, - Status: v1.ConditionFalse, + Status: corev1.ConditionFalse, LastTransitionTime: metav1.Now(), - Reason: "NoAvailableNode", - Message: "No available Node can be elected as EgressNode", + Reason: "AssignmentError", + Message: fmt.Sprintf("Failed to assign the IP to EgressNode: %v", scheduleErr), }, } } } else { + // The Egress IP is assigned to a Node (egressIP != "") but it's not this Node (isLocal == false), do nothing. return nil } toUpdate := egress.DeepCopy() var updateErr, getErr error if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - if compareEgressStatus(toUpdate.Status, *desiredStatus) { + if compareEgressStatus(&toUpdate.Status, desiredStatus) { return nil } + // Must make a copy here as we will append more conditions. If it's appended to desiredStatus directly, there + // would be duplicate conditions when the function retries. + statusToUpdate := desiredStatus.DeepCopy() + // Copy conditions other than crdv1b1.IPAssigned to statusToUpdate. for _, c := range toUpdate.Status.Conditions { if c.Type != crdv1b1.IPAssigned { - desiredStatus.Conditions = append(desiredStatus.Conditions, c) + statusToUpdate.Conditions = append(statusToUpdate.Conditions, c) } } - toUpdate.Status = *desiredStatus + toUpdate.Status = *statusToUpdate klog.V(2).InfoS("Updating Egress status", "Egress", egress.Name, "oldNode", egress.Status.EgressNode, "newNode", toUpdate.Status.EgressNode) _, updateErr = c.crdClient.CrdV1beta1().Egresses().UpdateStatus(context.TODO(), toUpdate, metav1.UpdateOptions{}) @@ -717,13 +728,16 @@ func (c *EgressController) syncEgress(egressName string) error { var desiredEgressIP string var desiredNode string + var scheduleErr error // Only check whether the Egress IP should be assigned to this Node when the Egress is schedulable. // Otherwise, users are responsible for assigning the Egress IP to Nodes. if isEgressSchedulable(egress) { - egressIP, egressNode, scheduled := c.egressIPScheduler.GetEgressIPAndNode(egressName) + egressIP, egressNode, err, scheduled := c.egressIPScheduler.GetEgressIPAndNode(egressName) if scheduled { desiredEgressIP = egressIP desiredNode = egressNode + } else { + scheduleErr = err } } else { desiredEgressIP = egress.Spec.EgressIP @@ -739,7 +753,7 @@ func (c *EgressController) syncEgress(egressName string) error { } // Do not proceed if EgressIP is empty. if desiredEgressIP == "" { - if err := c.updateEgressStatus(egress, ""); err != nil { + if err := c.updateEgressStatus(egress, "", scheduleErr); err != nil { return fmt.Errorf("update Egress %s status error: %v", egressName, err) } return nil @@ -778,7 +792,7 @@ func (c *EgressController) syncEgress(egressName string) error { eState.mark = mark } - if err := c.updateEgressStatus(egress, desiredEgressIP); err != nil { + if err := c.updateEgressStatus(egress, desiredEgressIP, nil); err != nil { return fmt.Errorf("update Egress %s status error: %v", egressName, err) } @@ -1067,20 +1081,18 @@ func isEgressSchedulable(egress *crdv1b1.Egress) bool { } // compareEgressStatus compares two Egress Statuses, ignoring LastTransitionTime and conditions other than IPAssigned, returns true if they are equal. -func compareEgressStatus(currentStatus, desiredStatus crdv1b1.EgressStatus) bool { - if currentStatus.EgressIP != desiredStatus.EgressIP || currentStatus.EgressNode != desiredStatus.EgressNode { +func compareEgressStatus(currentStatus, desiredStatus *crdv1b1.EgressStatus) bool { + if currentStatus == nil && desiredStatus == nil { + return true + } + if currentStatus == nil || desiredStatus == nil { return false } - getIPAssignedCondition := func(conditions []crdv1b1.EgressCondition) *crdv1b1.EgressCondition { - for _, c := range conditions { - if c.Type == crdv1b1.IPAssigned { - return &c - } - } - return nil + if currentStatus.EgressIP != desiredStatus.EgressIP || currentStatus.EgressNode != desiredStatus.EgressNode { + return false } - currentIPAssignedCondition := getIPAssignedCondition(currentStatus.Conditions) - desiredIPAssignedCondition := getIPAssignedCondition(desiredStatus.Conditions) + currentIPAssignedCondition := crdv1b1.GetEgressCondition(currentStatus.Conditions, crdv1b1.IPAssigned) + desiredIPAssignedCondition := crdv1b1.GetEgressCondition(desiredStatus.Conditions, crdv1b1.IPAssigned) if currentIPAssignedCondition == nil && desiredIPAssignedCondition == nil { return true } diff --git a/pkg/agent/controller/egress/egress_controller_test.go b/pkg/agent/controller/egress/egress_controller_test.go index f74706b1164..83301e06bf1 100644 --- a/pkg/agent/controller/egress/egress_controller_test.go +++ b/pkg/agent/controller/egress/egress_controller_test.go @@ -60,6 +60,7 @@ const ( fakeLocalEgressIP2 = "1.1.1.2" fakeRemoteEgressIP1 = "1.1.1.3" fakeNode = "node1" + fakeNode2 = "node2" ) type fakeLocalIPDetector struct { @@ -107,7 +108,7 @@ func (c *fakeSingleNodeCluster) ShouldSelectIP(ip string, pool string, filters . func (c *fakeSingleNodeCluster) SelectNodeForIP(ip, externalIPPool string, filters ...func(string) bool) (string, error) { for _, filter := range filters { if !filter(c.node) { - return "", nil + return "", memberlist.ErrNoNodeAvailable } } return c.node, nil @@ -581,12 +582,14 @@ func TestSyncEgress(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1}, - Status: crdv1b1.EgressStatus{EgressIP: fakeLocalEgressIP1, EgressNode: fakeNode, Conditions: nil}, + Status: crdv1b1.EgressStatus{EgressIP: fakeLocalEgressIP1, EgressNode: fakeNode}, }, { ObjectMeta: metav1.ObjectMeta{Name: "egressB", UID: "uidB"}, Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP2, ExternalIPPool: "external-ip-pool"}, - Status: crdv1b1.EgressStatus{EgressIP: fakeLocalEgressIP2, EgressNode: fakeNode, Conditions: []crdv1b1.EgressCondition{{Type: crdv1b1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode", LastTransitionTime: metav1.NewTime(time.Time{})}}}, + Status: crdv1b1.EgressStatus{EgressIP: fakeLocalEgressIP2, EgressNode: fakeNode, Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode"}, + }}, }, }, expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) { @@ -631,12 +634,16 @@ func TestSyncEgress(t *testing.T) { { ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1, ExternalIPPool: "external-ip-pool"}, - Status: crdv1b1.EgressStatus{EgressIP: fakeLocalEgressIP1, EgressNode: fakeNode, Conditions: []crdv1b1.EgressCondition{{Type: crdv1b1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode", LastTransitionTime: metav1.NewTime(time.Time{})}}}, + Status: crdv1b1.EgressStatus{EgressIP: fakeLocalEgressIP1, EgressNode: fakeNode, Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode"}, + }}, }, { ObjectMeta: metav1.ObjectMeta{Name: "egressB", UID: "uidB"}, Spec: crdv1b1.EgressSpec{EgressIP: fakeRemoteEgressIP1, ExternalIPPool: "external-ip-pool"}, - Status: crdv1b1.EgressStatus{}, + Status: crdv1b1.EgressStatus{Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAssigned, Status: v1.ConditionFalse, Reason: "AssignmentError", Message: "Failed to assign the IP to EgressNode: no Node available"}, + }}, }, }, expectedCalls: func(mockOFClient *openflowtest.MockClient, mockRouteClient *routetest.MockInterface, mockIPAssigner *ipassignertest.MockIPAssigner) { @@ -645,8 +652,6 @@ func TestSyncEgress(t *testing.T) { mockOFClient.EXPECT().InstallPodSNATFlows(uint32(1), net.ParseIP(fakeLocalEgressIP1), uint32(1)) mockOFClient.EXPECT().InstallPodSNATFlows(uint32(2), net.ParseIP(fakeLocalEgressIP1), uint32(1)) mockRouteClient.EXPECT().AddSNATRule(net.ParseIP(fakeLocalEgressIP1), uint32(1)) - mockIPAssigner.EXPECT().UnassignIP(fakeRemoteEgressIP1).Times(2) - mockOFClient.EXPECT().InstallPodSNATFlows(uint32(3), net.ParseIP(fakeRemoteEgressIP1), uint32(0)) }, }, { @@ -739,7 +744,7 @@ func TestSyncEgress(t *testing.T) { for _, expectedEgress := range tt.expectedEgresses { gotEgress, err := c.crdClient.CrdV1beta1().Egresses().Get(context.TODO(), expectedEgress.Name, metav1.GetOptions{}) require.NoError(t, err) - equalEgressNoTimestamp(t, expectedEgress, gotEgress) + assert.True(t, k8s.SemanticIgnoringTime.DeepEqual(expectedEgress, gotEgress)) } }) } @@ -941,42 +946,66 @@ func TestUpdateEgressStatus(t *testing.T) { updateError := fmt.Errorf("update Egress error") tests := []struct { name string + egress *crdv1b1.Egress + egressIP string + scheduleErr error updateErrorNum int updateError error getErrorNum int + selectedNodeForIP string expectedUpdateCalled int expectedGetCalled int expectedError error + expectedEgressStatus crdv1b1.EgressStatus }{ { - name: "succeed immediately", - updateErrorNum: 0, - updateError: nil, - getErrorNum: 0, + name: "updating static Egress succeeds immediately", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1}, + }, + egressIP: fakeLocalEgressIP1, expectedUpdateCalled: 1, - expectedGetCalled: 0, - expectedError: nil, + expectedEgressStatus: crdv1b1.EgressStatus{ + EgressNode: fakeNode, + EgressIP: fakeLocalEgressIP1, + }, }, { - name: "succeed after one update conflict failure", + name: "updating static Egress succeeds after one update conflict failure", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1}, + }, + egressIP: fakeLocalEgressIP1, updateErrorNum: 1, updateError: updateConflictError, - getErrorNum: 0, expectedUpdateCalled: 2, expectedGetCalled: 1, - expectedError: nil, + expectedEgressStatus: crdv1b1.EgressStatus{ + EgressNode: fakeNode, + EgressIP: fakeLocalEgressIP1, + }, }, { - name: "fail after one update failure", + name: "updating static Egress fails after one update failure", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1}, + }, + egressIP: fakeLocalEgressIP1, updateErrorNum: 1, updateError: updateError, - getErrorNum: 0, expectedUpdateCalled: 1, - expectedGetCalled: 0, expectedError: updateError, }, { - name: "fail after one update conflict failure and one get failure", + name: "updating static Egress fails after one update conflict failure and one get failure", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1}, + }, + egressIP: fakeLocalEgressIP1, updateErrorNum: 1, updateError: updateConflictError, getErrorNum: 1, @@ -984,41 +1013,150 @@ func TestUpdateEgressStatus(t *testing.T) { expectedGetCalled: 1, expectedError: getError, }, + { + name: "updating static Egress with remote IP does nothing", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeRemoteEgressIP1}, + }, + egressIP: fakeRemoteEgressIP1, + }, + { + name: "updating HA Egress with local IP succeeds immediately", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1, ExternalIPPool: "external-ip-pool"}, + Status: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + egressIP: fakeLocalEgressIP1, + expectedUpdateCalled: 1, + expectedEgressStatus: crdv1b1.EgressStatus{ + EgressNode: fakeNode, + EgressIP: fakeLocalEgressIP1, + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode"}, + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + { + name: "updating HA Egress with remote IP does nothing", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeRemoteEgressIP1, ExternalIPPool: "external-ip-pool"}, + Status: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + egressIP: fakeRemoteEgressIP1, + expectedEgressStatus: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + { + name: "updating HA Egress with schedule error succeeds immediately", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeRemoteEgressIP1, ExternalIPPool: "external-ip-pool"}, + Status: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + scheduleErr: memberlist.ErrNoNodeAvailable, + selectedNodeForIP: fakeNode, + expectedUpdateCalled: 1, + expectedEgressStatus: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAssigned, Status: v1.ConditionFalse, Reason: "AssignmentError", Message: "Failed to assign the IP to EgressNode: no Node available"}, + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + { + name: "updating HA Egress with schedule error succeeds after one update conflict failure", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeRemoteEgressIP1, ExternalIPPool: "external-ip-pool"}, + Status: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + scheduleErr: memberlist.ErrNoNodeAvailable, + selectedNodeForIP: fakeNode, + updateError: updateConflictError, + updateErrorNum: 2, + expectedUpdateCalled: 3, + expectedGetCalled: 2, + expectedEgressStatus: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAssigned, Status: v1.ConditionFalse, Reason: "AssignmentError", Message: "Failed to assign the IP to EgressNode: no Node available"}, + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + { + name: "updating HA Egress with schedule error does nothing when the Node is not selected to update", + egress: &crdv1b1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, + Spec: crdv1b1.EgressSpec{EgressIP: fakeRemoteEgressIP1, ExternalIPPool: "external-ip-pool"}, + Status: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + scheduleErr: memberlist.ErrNoNodeAvailable, + selectedNodeForIP: fakeNode2, // Not this Node. + expectedEgressStatus: crdv1b1.EgressStatus{ + Conditions: []crdv1b1.EgressCondition{ + {Type: crdv1b1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - egress := crdv1b1.Egress{ - ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA", ResourceVersion: "fake-ResourceVersion"}, - Spec: crdv1b1.EgressSpec{EgressIP: fakeLocalEgressIP1}, - } - fakeClient := &fakeversioned.Clientset{} + fakeClient := fakeversioned.NewSimpleClientset(tt.egress) getCalled := 0 - fakeClient.AddReactor("get", "egresses", func(action k8stesting.Action) (bool, runtime.Object, error) { + fakeClient.PrependReactor("get", "egresses", func(action k8stesting.Action) (bool, runtime.Object, error) { getCalled += 1 if getCalled <= tt.getErrorNum { return true, nil, getError } - return true, &egress, nil + return false, nil, nil }) updateCalled := 0 - fakeClient.AddReactor("update", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { + fakeClient.PrependReactor("update", "*", func(action k8stesting.Action) (bool, runtime.Object, error) { updateCalled += 1 if updateCalled <= tt.updateErrorNum { return true, nil, tt.updateError } - return true, &egress, nil + return false, nil, nil }) localIPDetector := &fakeLocalIPDetector{localIPs: sets.New[string](fakeLocalEgressIP1)} - c := &EgressController{crdClient: fakeClient, nodeName: fakeNode, localIPDetector: localIPDetector} - _, err := c.crdClient.CrdV1beta1().Egresses().Create(context.TODO(), &egress, metav1.CreateOptions{}) - assert.NoError(t, err) - err = c.updateEgressStatus(&egress, fakeLocalEgressIP1) + cluster := newFakeMemberlistCluster([]string{tt.selectedNodeForIP}) + c := &EgressController{crdClient: fakeClient, nodeName: fakeNode, localIPDetector: localIPDetector, cluster: cluster} + err := c.updateEgressStatus(tt.egress, tt.egressIP, tt.scheduleErr) if err != tt.expectedError { t.Errorf("Update Egress error not match, got: %v, expected: %v", err, tt.expectedError) } assert.Equal(t, tt.expectedGetCalled, getCalled, "Get called num not match") assert.Equal(t, tt.expectedUpdateCalled, updateCalled, "Update called num not match") + gotEgress, _ := c.crdClient.CrdV1beta1().Egresses().Get(context.TODO(), tt.egress.Name, metav1.GetOptions{}) + assert.True(t, k8s.SemanticIgnoringTime.DeepEqual(tt.expectedEgressStatus, gotEgress.Status), "Expected:\n%v\nGot:\n%v", tt.expectedEgressStatus, gotEgress.Status) }) } } @@ -1203,28 +1341,27 @@ func checkQueueItemExistence(t *testing.T, queue workqueue.RateLimitingInterface } func TestCompareEgressStatus(t *testing.T) { - newIPAssignedCondition := func(c v1.ConditionStatus, reason string, message string) crdv1b1.EgressCondition { + newCondition := func(t crdv1b1.EgressConditionType, c v1.ConditionStatus, reason string, message string) crdv1b1.EgressCondition { return crdv1b1.EgressCondition{ - Type: crdv1b1.IPAssigned, - Status: c, - Message: message, - Reason: reason, - LastTransitionTime: metav1.Now(), + Type: t, + Status: c, + Message: message, + Reason: reason, } } tests := []struct { name string - status1 crdv1b1.EgressStatus - status2 crdv1b1.EgressStatus + status1 *crdv1b1.EgressStatus + status2 *crdv1b1.EgressStatus expectedReturn bool // true if equal, false if not }{ { name: "Different EgressIP", - status1: crdv1b1.EgressStatus{ + status1: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", }, - status2: crdv1b1.EgressStatus{ + status2: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.2", EgressNode: "node1", }, @@ -1232,11 +1369,11 @@ func TestCompareEgressStatus(t *testing.T) { }, { name: "Different EgressNode", - status1: crdv1b1.EgressStatus{ + status1: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", }, - status2: crdv1b1.EgressStatus{ + status2: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node2", }, @@ -1244,56 +1381,83 @@ func TestCompareEgressStatus(t *testing.T) { }, { name: "Egresses are the same", - status1: crdv1b1.EgressStatus{ + status1: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", Conditions: []crdv1b1.EgressCondition{ - newIPAssignedCondition(v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), + newCondition(crdv1b1.IPAssigned, v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), }, }, - status2: crdv1b1.EgressStatus{ + status2: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", Conditions: []crdv1b1.EgressCondition{ - newIPAssignedCondition(v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), + newCondition(crdv1b1.IPAssigned, v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), }, }, expectedReturn: true, }, { name: "EgressStatus Condition is different", - status1: crdv1b1.EgressStatus{ + status1: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", Conditions: []crdv1b1.EgressCondition{ - newIPAssignedCondition(v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), + newCondition(crdv1b1.IPAssigned, v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), }, }, - status2: crdv1b1.EgressStatus{ + status2: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", Conditions: []crdv1b1.EgressCondition{ - newIPAssignedCondition(v1.ConditionFalse, "NoAvailableNode", "No available Node can be elected as EgressNode"), + newCondition(crdv1b1.IPAssigned, v1.ConditionFalse, "NoAvailableNode", "No available Node can be elected as EgressNode"), }, }, expectedReturn: false, }, { - name: "New Status has Condition that old one doesn't", - status1: crdv1b1.EgressStatus{ + name: "New Status has relevant Condition that old one doesn't", + status1: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", Conditions: []crdv1b1.EgressCondition{}, }, - status2: crdv1b1.EgressStatus{ + status2: &crdv1b1.EgressStatus{ EgressIP: "1.1.1.1", EgressNode: "node1", Conditions: []crdv1b1.EgressCondition{ - newIPAssignedCondition(v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), + newCondition(crdv1b1.IPAssigned, v1.ConditionTrue, "Assigned", "EgressIP is successfully assigned to EgressNode"), }, }, expectedReturn: false, }, + { + name: "New Status has irrelevant Condition that old one doesn't", + status1: &crdv1b1.EgressStatus{ + EgressIP: "1.1.1.1", + EgressNode: "node1", + Conditions: []crdv1b1.EgressCondition{}, + }, + status2: &crdv1b1.EgressStatus{ + EgressIP: "1.1.1.1", + EgressNode: "node1", + Conditions: []crdv1b1.EgressCondition{ + newCondition(crdv1b1.IPAllocated, v1.ConditionTrue, "Allocated", "EgressIP is successfully allocated"), + }, + }, + expectedReturn: true, + }, + { + name: "nils are the same", + expectedReturn: true, + }, + { + name: "nil and non empty one are different", + status2: &crdv1b1.EgressStatus{ + EgressIP: "1.1.1.1", + }, + expectedReturn: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1302,17 +1466,3 @@ func TestCompareEgressStatus(t *testing.T) { }) } } - -func equalEgressNoTimestamp(t *testing.T, expectedEgress, actualEgress *crdv1b1.Egress) bool { - if expectedEgress == nil || actualEgress == nil || expectedEgress.Status.Conditions == nil || actualEgress.Status.Conditions == nil { - return assert.Equal(t, expectedEgress, actualEgress) - } - for i := range expectedEgress.Status.Conditions { - expectedEgress.Status.Conditions[i].LastTransitionTime = metav1.NewTime(time.Time{}) - } - for i := range actualEgress.Status.Conditions { - actualEgress.Status.Conditions[i].LastTransitionTime = metav1.NewTime(time.Time{}) - } - t.Log(actualEgress.Status.Conditions) - return assert.Equal(t, expectedEgress, actualEgress) -} diff --git a/pkg/agent/controller/egress/ip_scheduler.go b/pkg/agent/controller/egress/ip_scheduler.go index 439279460a3..2850193faf7 100644 --- a/pkg/agent/controller/egress/ip_scheduler.go +++ b/pkg/agent/controller/egress/ip_scheduler.go @@ -47,6 +47,7 @@ type scheduleEventHandler func(egress string) type scheduleResult struct { ip string node string + err error } // egressIPScheduler is responsible for scheduling Egress IPs to appropriate Nodes according to the Node selector of the @@ -248,15 +249,18 @@ func (s *egressIPScheduler) AddEventHandler(handler scheduleEventHandler) { s.eventHandlers = append(s.eventHandlers, handler) } -func (s *egressIPScheduler) GetEgressIPAndNode(egress string) (string, string, bool) { +func (s *egressIPScheduler) GetEgressIPAndNode(egress string) (string, string, error, bool) { s.mutex.RLock() defer s.mutex.RUnlock() result, exists := s.scheduleResults[egress] if !exists { - return "", "", false + return "", "", nil, false } - return result.ip, result.node, true + if result.err != nil { + return "", "", result.err, false + } + return result.ip, result.node, nil, true } // EgressesByCreationTimestamp sorts a list of Egresses by creation timestamp. @@ -356,6 +360,8 @@ func (s *egressIPScheduler) schedule() { } else { klog.ErrorS(err, "Failed to select Node for Egress", "egress", klog.KObj(egress)) } + // Store error in its result to differentiate scheduling error from unprocessed case. + newResults[egress.Name] = &scheduleResult{err: err} continue } result := &scheduleResult{ @@ -380,7 +386,7 @@ func (s *egressIPScheduler) schedule() { prevResults := s.scheduleResults for egress, result := range newResults { prevResult, exists := prevResults[egress] - if !exists || prevResult.ip != result.ip || prevResult.node != result.node { + if !exists || prevResult.ip != result.ip || prevResult.node != result.node || prevResult.err != result.err { egressesToUpdate = append(egressesToUpdate, egress) } delete(prevResults, egress) diff --git a/pkg/agent/controller/egress/ip_scheduler_test.go b/pkg/agent/controller/egress/ip_scheduler_test.go index 2d2847e200a..e75ab03b200 100644 --- a/pkg/agent/controller/egress/ip_scheduler_test.go +++ b/pkg/agent/controller/egress/ip_scheduler_test.go @@ -142,6 +142,9 @@ func TestSchedule(t *testing.T) { node: "node2", ip: "1.1.1.11", }, + "egressC": { + err: memberlist.ErrNoNodeAvailable, + }, }, }, { @@ -178,6 +181,9 @@ func TestSchedule(t *testing.T) { node: "node3", ip: "1.1.1.11", }, + "egressC": { + err: memberlist.ErrNoNodeAvailable, + }, }, }, } @@ -329,7 +335,7 @@ func TestRun(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "egressD", UID: "uidD", CreationTimestamp: metav1.NewTime(time.Unix(4, 0))}, Spec: crdv1b1.EgressSpec{EgressIP: "1.1.1.1", ExternalIPPool: "pool1"}, }, metav1.CreateOptions{}) - assertReceivedItems(t, egressUpdates, sets.New[string]()) + assertReceivedItems(t, egressUpdates, sets.New[string]("egressD")) assertScheduleResult(t, s, "egressD", "", "", false) // After node2 joins, egressB should be moved to node2 determined by its consistent hash result, and egressD should be assigned to node1. @@ -357,6 +363,7 @@ func TestRun(t *testing.T) { } func assertReceivedItems(t *testing.T, ch <-chan string, expectedItems sets.Set[string]) { + t.Helper() receivedItems := sets.New[string]() for i := 0; i < expectedItems.Len(); i++ { select { @@ -376,7 +383,8 @@ func assertReceivedItems(t *testing.T, ch <-chan string, expectedItems sets.Set[ } func assertScheduleResult(t *testing.T, s *egressIPScheduler, egress, egressIP, egressNode string, scheduled bool) { - gotEgressIP, gotEgressNode, gotScheduled := s.GetEgressIPAndNode(egress) + t.Helper() + gotEgressIP, gotEgressNode, _, gotScheduled := s.GetEgressIPAndNode(egress) assert.Equal(t, egressIP, gotEgressIP) assert.Equal(t, egressNode, gotEgressNode) assert.Equal(t, scheduled, gotScheduled) diff --git a/pkg/apis/crd/v1alpha2/types.go b/pkg/apis/crd/v1alpha2/types.go index 841f3a320f0..6de016631b1 100644 --- a/pkg/apis/crd/v1alpha2/types.go +++ b/pkg/apis/crd/v1alpha2/types.go @@ -212,8 +212,12 @@ type EgressStatus struct { type EgressConditionType string const ( + // IPAllocated means at least one IP has been allocated to the Egress from ExternalIPPool. + // It is not applicable for Egresses with empty ExternalIPPool. IPAllocated EgressConditionType = "IPAllocated" - IPAssigned EgressConditionType = "IPAssigned" + // IPAssigned means the Egress has been assigned to a Node. + // It is not applicable for Egresses with empty ExternalIPPool. + IPAssigned EgressConditionType = "IPAssigned" ) type EgressCondition struct { diff --git a/pkg/apis/crd/v1beta1/types.go b/pkg/apis/crd/v1beta1/types.go index e6e7c1a86e9..9c94d4ee4a3 100644 --- a/pkg/apis/crd/v1beta1/types.go +++ b/pkg/apis/crd/v1beta1/types.go @@ -843,8 +843,12 @@ type EgressStatus struct { type EgressConditionType string const ( + // IPAllocated means at least one IP has been allocated to the Egress from ExternalIPPool. + // It is not applicable for Egresses with empty ExternalIPPool. IPAllocated EgressConditionType = "IPAllocated" - IPAssigned EgressConditionType = "IPAssigned" + // IPAssigned means the Egress has been assigned to a Node. + // It is not applicable for Egresses with empty ExternalIPPool. + IPAssigned EgressConditionType = "IPAssigned" ) type EgressCondition struct { diff --git a/pkg/apis/crd/v1beta1/util.go b/pkg/apis/crd/v1beta1/util.go new file mode 100644 index 00000000000..09c49b696a0 --- /dev/null +++ b/pkg/apis/crd/v1beta1/util.go @@ -0,0 +1,24 @@ +// Copyright 2023 Antrea Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package v1beta1 + +func GetEgressCondition(conditions []EgressCondition, conditionType EgressConditionType) *EgressCondition { + for _, c := range conditions { + if c.Type == conditionType { + return &c + } + } + return nil +} diff --git a/pkg/controller/egress/controller.go b/pkg/controller/egress/controller.go index 2fa97bdfec7..5d40662baba 100644 --- a/pkg/controller/egress/controller.go +++ b/pkg/controller/egress/controller.go @@ -475,7 +475,7 @@ func (c *EgressController) updateEgressAllocatedCondition(egress *egressv1beta1. Type: egressv1beta1.IPAllocated, Status: v1.ConditionFalse, Reason: "AllocationError", - Message: fmt.Sprintf("Cannot allocate EgressIP from ExternalIPPool %s due to: %v", egress.Spec.ExternalIPPool, err), + Message: fmt.Sprintf("Cannot allocate EgressIP from ExternalIPPool: %v", err), LastTransitionTime: metav1.Now(), } } @@ -484,7 +484,7 @@ func (c *EgressController) updateEgressAllocatedCondition(egress *egressv1beta1. toUpdate := egress.DeepCopy() var updateErr, getErr error if err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - actualCondition := getIPAllocatedCondition(toUpdate) + actualCondition := egressv1beta1.GetEgressCondition(toUpdate.Status.Conditions, egressv1beta1.IPAllocated) if compareConditionIgnoringTimestamp(actualCondition, desiredCondition) { return nil } @@ -510,16 +510,6 @@ func (c *EgressController) updateEgressAllocatedCondition(egress *egressv1beta1. } } -// getIPAllocatedCondition gets the IPAllocated condition from an Egress -func getIPAllocatedCondition(egress *egressv1beta1.Egress) *egressv1beta1.EgressCondition { - for _, c := range egress.Status.Conditions { - if c.Type == egressv1beta1.IPAllocated { - return &c - } - } - return nil -} - // compareConditionIgnoringTimestamp compares two conditions ignoring the timestamp func compareConditionIgnoringTimestamp(condition1, condition2 *egressv1beta1.EgressCondition) bool { if condition1 == nil && condition2 == nil { diff --git a/pkg/controller/egress/controller_test.go b/pkg/controller/egress/controller_test.go index e7fba9335e3..4e8c6392e66 100644 --- a/pkg/controller/egress/controller_test.go +++ b/pkg/controller/egress/controller_test.go @@ -45,6 +45,7 @@ import ( "antrea.io/antrea/pkg/controller/egress/store" "antrea.io/antrea/pkg/controller/externalippool" "antrea.io/antrea/pkg/controller/grouping" + "antrea.io/antrea/pkg/util/k8s" ) var ( @@ -767,99 +768,108 @@ func checkExternalIPPoolUsed(t *testing.T, controller *egressController, poolNam assert.NoError(t, err) } -func TestEgressAllocatedStatus(t *testing.T) { +func TestUpdateEgressAllocatedCondition(t *testing.T) { tests := []struct { - name string - existingEgresses []*v1beta1.Egress - existingExternalIPPool *v1beta1.ExternalIPPool - inputEgress *v1beta1.Egress - expectedUpdate bool - expectedEgressConditionType v1beta1.EgressConditionType - expectedEgressConditionStatus v1.ConditionStatus - expectedEgressConditionReason string + name string + inputEgress *v1beta1.Egress + inputErr error + expectedStatus v1beta1.EgressStatus }{ { - name: "Manually assigned EgressIP", - existingExternalIPPool: newExternalIPPool("ipPoolA", "1.1.1.0/32", "", ""), + name: "allocating IP succeeds", inputEgress: &v1beta1.Egress{ ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, Spec: v1beta1.EgressSpec{ EgressIP: "1.1.1.1", - ExternalIPPool: "", + ExternalIPPool: "pool1", + }, + }, + expectedStatus: v1beta1.EgressStatus{ + Conditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, }, }, - expectedUpdate: false, }, { - name: "Available IP to be allocated", - existingExternalIPPool: newExternalIPPool("ipPoolA", "1.1.1.0/24", "", ""), + name: "allocating IP fails", inputEgress: &v1beta1.Egress{ ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, Spec: v1beta1.EgressSpec{ - EgressIP: "", - ExternalIPPool: "ipPoolA", + ExternalIPPool: "pool1", + }, + }, + inputErr: fmt.Errorf("no available IP"), + expectedStatus: v1beta1.EgressStatus{ + Conditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAllocated, Status: v1.ConditionFalse, Reason: "AllocationError", Message: "Cannot allocate EgressIP from ExternalIPPool: no available IP"}, }, }, - expectedUpdate: true, - expectedEgressConditionType: v1beta1.IPAllocated, - expectedEgressConditionStatus: v1.ConditionTrue, - expectedEgressConditionReason: "Allocated", }, { - name: "No available IP to be allocated", - existingExternalIPPool: newExternalIPPool("ipPoolA", "1.1.1.0/32", "", ""), - existingEgresses: []*v1beta1.Egress{ - { - ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, - Spec: v1beta1.EgressSpec{ - EgressIP: "", - ExternalIPPool: "ipPoolA", + name: "specifying IP fails", + inputEgress: &v1beta1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, + Spec: v1beta1.EgressSpec{ + EgressIP: "1.1.1.1", + ExternalIPPool: "pool1", + }, + }, + inputErr: fmt.Errorf("IP already used"), + expectedStatus: v1beta1.EgressStatus{ + Conditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAllocated, Status: v1.ConditionFalse, Reason: "AllocationError", Message: "Cannot allocate EgressIP from ExternalIPPool: IP already used"}, + }, + }, + }, + { + name: "updating condition succeeds", + inputEgress: &v1beta1.Egress{ + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, + Spec: v1beta1.EgressSpec{ + EgressIP: "1.1.1.1", + ExternalIPPool: "pool1", + }, + Status: v1beta1.EgressStatus{ + Conditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAllocated, Status: v1.ConditionFalse, Reason: "AllocationError", Message: "Cannot allocate EgressIP from ExternalIPPool: no available IP"}, }, }, }, + expectedStatus: v1beta1.EgressStatus{ + Conditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + { + name: "removing condition succeeds", inputEgress: &v1beta1.Egress{ - ObjectMeta: metav1.ObjectMeta{Name: "egressB", UID: "uidB"}, + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, Spec: v1beta1.EgressSpec{ - EgressIP: "", - ExternalIPPool: "ipPoolA", + EgressIP: "1.1.1.1", + ExternalIPPool: "", // ExternalIPPool is removed. + }, + Status: v1beta1.EgressStatus{ + Conditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully allocated"}, + {Type: v1beta1.IPAllocated, Status: v1.ConditionFalse, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, + }, + }, + expectedStatus: v1beta1.EgressStatus{ + Conditions: []v1beta1.EgressCondition{ // It should only delete IPAllocated condition. + {Type: v1beta1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully allocated"}, }, }, - expectedUpdate: true, - expectedEgressConditionType: v1beta1.IPAllocated, - expectedEgressConditionStatus: v1.ConditionFalse, - expectedEgressConditionReason: "AllocationError", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - stopCh := make(chan struct{}) - defer close(stopCh) - var fakeObjects []runtime.Object - fakeObjects = append(fakeObjects, tt.inputEgress, tt.existingExternalIPPool) - controller := newController(nil, fakeObjects) - controller.informerFactory.Start(stopCh) - controller.crdInformerFactory.Start(stopCh) - controller.informerFactory.WaitForCacheSync(stopCh) - controller.crdInformerFactory.WaitForCacheSync(stopCh) - go controller.externalIPAllocator.Run(stopCh) - require.True(t, cache.WaitForCacheSync(stopCh, controller.externalIPAllocator.HasSynced)) - controller.restoreIPAllocations(tt.existingEgresses) - controller.addEgress(tt.inputEgress) - controller.syncEgress(tt.inputEgress.Name) - eg, err := controller.crdClient.CrdV1beta1().Egresses().Get(context.TODO(), tt.inputEgress.Name, metav1.GetOptions{}) - assert.NoError(t, err) - - if tt.expectedUpdate { - assert.Eventually(t, func() bool { - eg, err = controller.crdClient.CrdV1beta1().Egresses().Get(context.TODO(), tt.inputEgress.Name, metav1.GetOptions{}) - if len(eg.Status.Conditions) == 0 { - return false - } - return assert.Equal(t, tt.expectedEgressConditionStatus, eg.Status.Conditions[0].Status) && assert.Equal(t, tt.expectedEgressConditionType, eg.Status.Conditions[0].Type) && assert.Equal(t, tt.expectedEgressConditionReason, eg.Status.Conditions[0].Reason) - }, time.Second, 50*time.Millisecond, "Egress status was not updated properly") - } else { - assert.Len(t, eg.Status.Conditions, 0) - } + controller := newController(nil, []runtime.Object{tt.inputEgress}) + controller.updateEgressAllocatedCondition(tt.inputEgress, tt.inputErr) + gotEgress, err := controller.crdClient.CrdV1beta1().Egresses().Get(context.TODO(), tt.inputEgress.Name, metav1.GetOptions{}) + require.NoError(t, err) + assert.True(t, k8s.SemanticIgnoringTime.DeepEqual(tt.expectedStatus, gotEgress.Status), "Expected:\n%v\ngot:\n%v", tt.expectedStatus, gotEgress.Status) }) } } diff --git a/pkg/util/k8s/semantic.go b/pkg/util/k8s/semantic.go new file mode 100644 index 00000000000..4c9d6ca7e79 --- /dev/null +++ b/pkg/util/k8s/semantic.go @@ -0,0 +1,26 @@ +// Copyright 2023 Antrea Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package k8s + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/conversion" +) + +var SemanticIgnoringTime = conversion.EqualitiesOrDie( + func(a, b metav1.Time) bool { + return true + }, +) diff --git a/test/e2e/egress_test.go b/test/e2e/egress_test.go index e9c01691779..7da9d309080 100644 --- a/test/e2e/egress_test.go +++ b/test/e2e/egress_test.go @@ -35,6 +35,7 @@ import ( "antrea.io/antrea/pkg/agent/config" "antrea.io/antrea/pkg/apis/crd/v1beta1" "antrea.io/antrea/pkg/features" + "antrea.io/antrea/pkg/util/k8s" ) const waitEgressRealizedTimeout = 3 * time.Second @@ -285,12 +286,13 @@ func testEgressClientIP(t *testing.T, data *TestData) { func testEgressCRUD(t *testing.T, data *TestData) { tests := []struct { - name string - ipRange v1beta1.IPRange - nodeSelector metav1.LabelSelector - expectedEgressIP string - expectedNodes sets.Set[string] - expectedTotal int + name string + ipRange v1beta1.IPRange + nodeSelector metav1.LabelSelector + expectedEgressIP string + expectedNodes sets.Set[string] + expectedTotal int + expectedConditions []v1beta1.EgressCondition }{ { name: "single matching Node", @@ -303,6 +305,10 @@ func testEgressCRUD(t *testing.T, data *TestData) { expectedEgressIP: "169.254.100.1", expectedNodes: sets.New[string](nodeName(0)), expectedTotal: 2, + expectedConditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode"}, + {Type: v1beta1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, }, { name: "single matching Node with IPv6 range", @@ -315,6 +321,10 @@ func testEgressCRUD(t *testing.T, data *TestData) { expectedEgressIP: "2021:1::aaa1", expectedNodes: sets.New[string](nodeName(0)), expectedTotal: 15, + expectedConditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode"}, + {Type: v1beta1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, }, { name: "two matching Nodes", @@ -331,6 +341,10 @@ func testEgressCRUD(t *testing.T, data *TestData) { expectedEgressIP: "169.254.101.10", expectedNodes: sets.New[string](nodeName(0), nodeName(1)), expectedTotal: 2, + expectedConditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAssigned, Status: v1.ConditionTrue, Reason: "Assigned", Message: "EgressIP is successfully assigned to EgressNode"}, + {Type: v1beta1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, }, { name: "no matching Node", @@ -343,6 +357,10 @@ func testEgressCRUD(t *testing.T, data *TestData) { expectedEgressIP: "169.254.102.1", expectedNodes: sets.New[string](), expectedTotal: 2, + expectedConditions: []v1beta1.EgressCondition{ + {Type: v1beta1.IPAssigned, Status: v1.ConditionFalse, Reason: "AssignmentError", Message: "Failed to assign the IP to EgressNode: no Node available"}, + {Type: v1beta1.IPAllocated, Status: v1.ConditionTrue, Reason: "Allocated", Message: "EgressIP is successfully allocated"}, + }, }, } for _, tt := range tests { @@ -367,6 +385,9 @@ func testEgressCRUD(t *testing.T, data *TestData) { if egress.Spec.EgressIP != tt.expectedEgressIP { return false, nil } + if !k8s.SemanticIgnoringTime.DeepEqual(tt.expectedConditions, egress.Status.Conditions) { + return false, nil + } if tt.expectedNodes.Len() == 0 { if egress.Status.EgressNode != "" { return false, fmt.Errorf("this Egress shouldn't be assigned to any Node")