diff --git a/pkg/agent/controller/egress/egress_controller.go b/pkg/agent/controller/egress/egress_controller.go index c98c55d8f7e..7e17697a8ac 100644 --- a/pkg/agent/controller/egress/egress_controller.go +++ b/pkg/agent/controller/egress/egress_controller.go @@ -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) @@ -647,14 +647,16 @@ 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. Therefore, we only set IPAssigned + // condition to false when there is an error. + if scheduleErr != nil { desiredStatus.Conditions = []crdv1b1.EgressCondition{ { Type: crdv1b1.IPAssigned, Status: v1.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), }, } } @@ -668,12 +670,14 @@ func (c *EgressController) updateEgressStatus(egress *crdv1b1.Egress, egressIP s if compareEgressStatus(toUpdate.Status, *desiredStatus) { return nil } + 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 +721,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 +746,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 +785,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) } @@ -1071,16 +1078,8 @@ func compareEgressStatus(currentStatus, desiredStatus crdv1b1.EgressStatus) bool if currentStatus.EgressIP != desiredStatus.EgressIP || currentStatus.EgressNode != desiredStatus.EgressNode { return false } - getIPAssignedCondition := func(conditions []crdv1b1.EgressCondition) *crdv1b1.EgressCondition { - for _, c := range conditions { - if c.Type == crdv1b1.IPAssigned { - return &c - } - } - return nil - } - 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..97347dd4c78 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) }) } } @@ -1205,11 +1343,10 @@ func checkQueueItemExistence(t *testing.T, queue workqueue.RateLimitingInterface func TestCompareEgressStatus(t *testing.T) { newIPAssignedCondition := func(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: crdv1b1.IPAssigned, + Status: c, + Message: message, + Reason: reason, } } tests := []struct { @@ -1302,17 +1439,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")