fix: update status with up-to-date agent image

pkg/controller/ippool/common.go

@@ -191,6 +191,14 @@ func NewIPPoolBuilder(namespace, name string) *IPPoolBuilder {
 	}
 }
 
+func (b *IPPoolBuilder) Annotation(key, value string) *IPPoolBuilder {
+	if b.ipPool.Annotations == nil {
+		b.ipPool.Annotations = make(map[string]string)
+	}
+	b.ipPool.Annotations[key] = value
+	return b
+}
+
 func (b *IPPoolBuilder) NetworkName(networkName string) *IPPoolBuilder {
 	b.ipPool.Spec.NetworkName = networkName
 	return b

pkg/controller/ippool/controller.go

@@ -29,7 +29,8 @@ import (
 const (
 	controllerName = "vm-dhcp-ippool-controller"
 
-	multusNetworksAnnotationKey = "k8s.v1.cni.cncf.io/networks"
+	multusNetworksAnnotationKey         = "k8s.v1.cni.cncf.io/networks"
+	holdIPPoolAgentUpgradeAnnotationKey = "network.harvesterhci.io/hold-ippool-agent-upgrade"
 
 	ipPoolNamespaceLabelKey  = network.GroupName + "/ippool-namespace"
 	ipPoolNameLabelKey       = network.GroupName + "/ippool-name"
@@ -254,6 +255,8 @@ func (h *Handler) OnRemove(key string, ipPool *networkv1.IPPool) (*networkv1.IPP
 	return ipPool, nil
 }
 
+// DeployAgent reconciles ipPool and ensures there's an agent pod for it. The
+// returned status reports whether an agent pod is registered.
 func (h *Handler) DeployAgent(ipPool *networkv1.IPPool, status networkv1.IPPoolStatus) (networkv1.IPPoolStatus, error) {
 	logrus.Debugf("(ippool.DeployAgent) deploy agent for ippool %s/%s", ipPool.Namespace, ipPool.Name)
 
@@ -281,6 +284,7 @@ func (h *Handler) DeployAgent(ipPool *networkv1.IPPool, status networkv1.IPPoolS
 	}
 
 	if ipPool.Status.AgentPodRef != nil {
+		status.AgentPodRef.Image = h.getAgentImage(ipPool)
 		pod, err := h.podCache.Get(ipPool.Status.AgentPodRef.Namespace, ipPool.Status.AgentPodRef.Name)
 		if err != nil {
 			if !apierrors.IsNotFound(err) {
@@ -289,11 +293,15 @@ func (h *Handler) DeployAgent(ipPool *networkv1.IPPool, status networkv1.IPPoolS
 
 			logrus.Warningf("(ippool.DeployAgent) agent pod %s missing, redeploying", ipPool.Status.AgentPodRef.Name)
 		} else {
-			if pod.GetUID() == ipPool.Status.AgentPodRef.UID {
-				return status, nil
+			if pod.DeletionTimestamp != nil {
+				return status, fmt.Errorf("agent pod %s marked for deletion", ipPool.Status.AgentPodRef.Name)
 			}
 
-			return status, fmt.Errorf("agent pod %s uid mismatch, waiting for removal then redeploy", ipPool.Status.AgentPodRef.Name)
+			if pod.GetUID() != ipPool.Status.AgentPodRef.UID {
+				return status, fmt.Errorf("agent pod %s uid mismatch", ipPool.Status.AgentPodRef.Name)
+			}
+
+			return status, nil
 		}
 	}
 
@@ -325,6 +333,11 @@ func (h *Handler) DeployAgent(ipPool *networkv1.IPPool, status networkv1.IPPoolS
 	return status, nil
 }
 
+// BuildCache reconciles ipPool and initializes the IPAM and MAC caches for it.
+// The source information comes from both ipPool's spec and status. Since
+// IPPool objects are deemed source of truths, BuildCache honors the state and
+// use it to load up internal caches. The returned status reports whether both
+// caches are fully initialized.
 func (h *Handler) BuildCache(ipPool *networkv1.IPPool, status networkv1.IPPoolStatus) (networkv1.IPPoolStatus, error) {
 	logrus.Debugf("(ippool.BuildCache) build ipam for ippool %s/%s", ipPool.Namespace, ipPool.Name)
 
@@ -380,6 +393,10 @@ func (h *Handler) BuildCache(ipPool *networkv1.IPPool, status networkv1.IPPoolSt
 	return status, nil
 }
 
+// MonitorAgent reconciles ipPool and keeps an eye on the agent pod. If the
+// running agent pod does not match to the one record in ipPool's status,
+// MonitorAgent tries to delete it. The returned status reports whether the
+// agent pod is ready.
 func (h *Handler) MonitorAgent(ipPool *networkv1.IPPool, status networkv1.IPPoolStatus) (networkv1.IPPoolStatus, error) {
 	logrus.Debugf("(ippool.MonitorAgent) monitor agent for ippool %s/%s", ipPool.Namespace, ipPool.Name)
 
@@ -401,11 +418,19 @@ func (h *Handler) MonitorAgent(ipPool *networkv1.IPPool, status networkv1.IPPool
 	}
 
 	if agentPod.GetUID() != ipPool.Status.AgentPodRef.UID || agentPod.Spec.Containers[0].Image != ipPool.Status.AgentPodRef.Image {
-		return status, h.podClient.Delete(agentPod.Namespace, agentPod.Name, &metav1.DeleteOptions{})
+		if agentPod.DeletionTimestamp != nil {
+			return status, fmt.Errorf("agent pod %s marked for deletion", agentPod.Name)
+		}
+
+		if err := h.podClient.Delete(agentPod.Namespace, agentPod.Name, &metav1.DeleteOptions{}); err != nil {
+			return status, err
+		}
+
+		return status, fmt.Errorf("agent pod %s obsolete and purged", agentPod.Name)
 	}
 
 	if !isPodReady(agentPod) {
-		return status, fmt.Errorf("agent %s for ippool %s/%s is not ready", agentPod.Name, ipPool.Namespace, ipPool.Name)
+		return status, fmt.Errorf("agent pod %s not ready", agentPod.Name)
 	}
 
 	return status, nil
@@ -420,6 +445,14 @@ func isPodReady(pod *corev1.Pod) bool {
 	return false
 }
 
+func (h *Handler) getAgentImage(ipPool *networkv1.IPPool) string {
+	_, ok := ipPool.Annotations[holdIPPoolAgentUpgradeAnnotationKey]
+	if ok {
+		return ipPool.Status.AgentPodRef.Image
+	}
+	return h.agentImage.String()
+}
+
 func (h *Handler) cleanup(ipPool *networkv1.IPPool) error {
 	if ipPool.Status.AgentPodRef == nil {
 		return nil

pkg/controller/ippool/controller_test.go

@@ -506,6 +506,20 @@ func TestHandler_DeployAgent(t *testing.T) {
 			AgentPodRef(testPodNamespace, testPodName, testImage, "").Build()
 		givenNAD := newTestNetworkAttachmentDefinitionBuilder().
 			Label(clusterNetworkLabelKey, testClusterNetwork).Build()
+		givenPod, _ := prepareAgentPod(
+			NewIPPoolBuilder(testIPPoolNamespace, testIPPoolName).
+				ServerIP(testServerIP).
+				CIDR(testCIDR).
+				NetworkName(testNetworkName).Build(),
+			false,
+			testPodNamespace,
+			testClusterNetwork,
+			testServiceAccountName,
+			&config.Image{
+				Repository: testImageRepository,
+				Tag:        testImageTag,
+			},
+		)
 
 		expectedStatus := newTestIPPoolStatusBuilder().
 			AgentPodRef(testPodNamespace, testPodName, testImageNew, "").Build()
@@ -521,6 +535,66 @@ func TestHandler_DeployAgent(t *testing.T) {
 		assert.Nil(t, err, "mock resource should add into fake controller tracker")
 
 		k8sclientset := k8sfake.NewSimpleClientset()
+		err = k8sclientset.Tracker().Add(givenPod)
+		assert.Nil(t, err, "mock resource should add into fake controller tracker")
+
+		handler := Handler{
+			agentNamespace: testPodNamespace,
+			agentImage: &config.Image{
+				Repository: testImageRepository,
+				Tag:        testImageTagNew,
+			},
+			agentServiceAccountName: testServiceAccountName,
+			nadCache:                fakeclient.NetworkAttachmentDefinitionCache(clientset.K8sCniCncfIoV1().NetworkAttachmentDefinitions),
+			podClient:               fakeclient.PodClient(k8sclientset.CoreV1().Pods),
+			podCache:                fakeclient.PodCache(k8sclientset.CoreV1().Pods),
+		}
+
+		status, err := handler.DeployAgent(givenIPPool, givenIPPool.Status)
+		assert.Nil(t, err)
+		assert.Equal(t, expectedStatus, status)
+	})
+
+	t.Run("agent pod upgrade held back", func(t *testing.T) {
+		givenIPPool := newTestIPPoolBuilder().
+			Annotation(holdIPPoolAgentUpgradeAnnotationKey, "true").
+			ServerIP(testServerIP).
+			CIDR(testCIDR).
+			NetworkName(testNetworkName).
+			AgentPodRef(testPodNamespace, testPodName, testImage, "").Build()
+		givenNAD := newTestNetworkAttachmentDefinitionBuilder().
+			Label(clusterNetworkLabelKey, testClusterNetwork).Build()
+		givenPod, _ := prepareAgentPod(
+			NewIPPoolBuilder(testIPPoolNamespace, testIPPoolName).
+				ServerIP(testServerIP).
+				CIDR(testCIDR).
+				NetworkName(testNetworkName).Build(),
+			false,
+			testPodNamespace,
+			testClusterNetwork,
+			testServiceAccountName,
+			&config.Image{
+				Repository: testImageRepository,
+				Tag:        testImageTag,
+			},
+		)
+
+		expectedStatus := newTestIPPoolStatusBuilder().
+			AgentPodRef(testPodNamespace, testPodName, testImage, "").Build()
+
+		nadGVR := schema.GroupVersionResource{
+			Group:    "k8s.cni.cncf.io",
+			Version:  "v1",
+			Resource: "network-attachment-definitions",
+		}
+
+		clientset := fake.NewSimpleClientset()
+		err := clientset.Tracker().Create(nadGVR, givenNAD, givenNAD.Namespace)
+		assert.Nil(t, err, "mock resource should add into fake controller tracker")
+
+		k8sclientset := k8sfake.NewSimpleClientset()
+		err = k8sclientset.Tracker().Add(givenPod)
+		assert.Nil(t, err, "mock resource should add into fake controller tracker")
 
 		handler := Handler{
 			agentNamespace: testPodNamespace,
@@ -589,7 +663,7 @@ func TestHandler_DeployAgent(t *testing.T) {
 		}
 
 		_, err = handler.DeployAgent(givenIPPool, givenIPPool.Status)
-		assert.Equal(t, fmt.Sprintf("agent pod %s uid mismatch, waiting for removal then redeploy", testPodName), err.Error())
+		assert.Equal(t, fmt.Sprintf("agent pod %s uid mismatch", testPodName), err.Error())
 	})
 }
 
@@ -736,7 +810,7 @@ func TestHandler_MonitorAgent(t *testing.T) {
 		}
 
 		_, err = handler.MonitorAgent(givenIPPool, givenIPPool.Status)
-		assert.Equal(t, fmt.Sprintf("agent %s for ippool %s/%s is not ready", testPodName, testIPPoolNamespace, testIPPoolName), err.Error())
+		assert.Equal(t, fmt.Sprintf("agent pod %s not ready", testPodName), err.Error())
 	})
 
 	t.Run("agent pod ready", func(t *testing.T) {
@@ -805,10 +879,9 @@ func TestHandler_MonitorAgent(t *testing.T) {
 		}
 
 		_, err = handler.MonitorAgent(givenIPPool, givenIPPool.Status)
-		assert.Nil(t, err)
+		assert.Equal(t, fmt.Sprintf("agent pod %s obsolete and purged", testPodName), err.Error())
 
 		_, err = handler.podClient.Get(testPodNamespace, testPodName, metav1.GetOptions{})
-		assert.NotNil(t, err)
 		assert.Equal(t, fmt.Sprintf("pods \"%s\" not found", testPodName), err.Error())
 	})
 }