Merge pull request #242 from nunnatsa/node-drain-external

Support VMI eviction also for external infra clusters
kubernetes-sigs · Jun 26, 2023 · 44140e4 · 44140e4
2 parents 1b99674 + 87a8fb7
commit 44140e4
Show file tree

Hide file tree

Showing 15 changed files with 633 additions and 797 deletions.
diff --git a/api/v1alpha1/kubevirtcluster_types.go b/api/v1alpha1/kubevirtcluster_types.go
@@ -36,7 +36,8 @@ const ( //labels
 )
 
 const ( // annotations
-	VmiDeletionGraceTime = "capk.cluster.x-k8s.io/vmi-deletion-grace-time"
+	VmiDeletionGraceTime       = "capk.cluster.x-k8s.io/vmi-deletion-grace-time"
+	VmiDeletionGraceTimeEscape = "capk.cluster.x-k8s.io~1vmi-deletion-grace-time"
 )
 
 // KubevirtClusterSpec defines the desired state of KubevirtCluster.

diff --git a/clusterkubevirtadm/cmd/get/kubeconfig/kubeconfig_test.go b/clusterkubevirtadm/cmd/get/kubeconfig/kubeconfig_test.go
@@ -157,7 +157,7 @@ var _ = Describe("test kubeconfig function", func() {
 			Expect(found).ShouldNot(BeNil())
 			Expect(found.Secrets).ToNot(BeEmpty())
 
-			Eventually(doneUpdatingSa)
+			Eventually(doneUpdatingSa).Should(BeClosed())
 		})
 
 		It("should fail after 10 seconds if the secret was not created", func() {

diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -101,10 +101,6 @@ rules:
   verbs:
   - delete
   - get
-  - list
-  - patch
-  - update
-  - watch
 - apiGroups:
   - kubevirt.io
   resources:
@@ -113,10 +109,8 @@ rules:
   - create
   - delete
   - get
-  - list
   - patch
   - update
-  - watch
 - apiGroups:
   - rbac.authorization.k8s.io
   resources:

diff --git a/controllers/kubevirtmachine_controller.go b/controllers/kubevirtmachine_controller.go
@@ -64,8 +64,8 @@ type KubevirtMachineReconciler struct {
 // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=kubevirtmachines/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;machines,verbs=get;list;watch
 // +kubebuilder:rbac:groups="",resources=secrets;,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines;,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachineinstances;,verbs=get;list;watch
+// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines;,verbs=get;create;update;patch;delete
+// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachineinstances;,verbs=get;delete
 
 // Reconcile handles KubevirtMachine events.
 func (r *KubevirtMachineReconciler) Reconcile(goctx gocontext.Context, req ctrl.Request) (_ ctrl.Result, rerr error) {
@@ -289,6 +289,14 @@ func (r *KubevirtMachineReconciler) reconcileNormal(ctx *context.MachineContext)
 		return ctrl.Result{RequeueAfter: 20 * time.Second}, nil
 	}
 
+	retryDuration, err := externalMachine.DrainNodeIfNeeded(r.WorkloadCluster)
+	if err != nil {
+		return ctrl.Result{RequeueAfter: retryDuration}, errors.Wrap(err, "failed to drain node")
+	}
+	if retryDuration > 0 {
+		return ctrl.Result{RequeueAfter: retryDuration}, nil
+	}
+
 	if externalMachine.SupportsCheckingIsBootstrapped() && !conditions.IsTrue(ctx.KubevirtMachine, infrav1.BootstrapExecSucceededCondition) {
 		if !externalMachine.IsBootstrapped() {
 			ctx.Logger.Info("Waiting for underlying VM to bootstrap...")

diff --git a/controllers/kubevirtmachine_controller_test.go b/controllers/kubevirtmachine_controller_test.go
@@ -18,6 +18,7 @@ package controllers
 
 import (
 	gocontext "context"
+	"fmt"
 	"time"
 
 	"github.com/golang/mock/gomock"
@@ -395,6 +396,8 @@ var _ = Describe("reconcile a kubevirt machine", func() {
 		machineMock.EXPECT().Address().Return("1.1.1.1").AnyTimes()
 		machineMock.EXPECT().SupportsCheckingIsBootstrapped().Return(false).AnyTimes()
 		machineMock.EXPECT().GenerateProviderID().Return("abc", nil).AnyTimes()
+		machineMock.EXPECT().GenerateProviderID().Return("abc", nil).AnyTimes()
+		machineMock.EXPECT().DrainNodeIfNeeded(gomock.Any()).Return(time.Duration(0), nil).AnyTimes()
 		machineFactoryMock.EXPECT().NewMachine(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(machineMock, nil).Times(1)
 
 		infraClusterMock.EXPECT().GenerateInfraClusterClient(kubevirtMachine.Spec.InfraClusterSecretRef, kubevirtMachine.Namespace, machineContext.Context).Return(fakeClient, kubevirtMachine.Namespace, nil).Times(3)
@@ -898,6 +901,7 @@ var _ = Describe("reconcile a kubevirt machine", func() {
 				machineMock.EXPECT().Exists().Return(true).Times(1)
 				machineMock.EXPECT().Address().Return("1.1.1.1").Times(1)
 				machineMock.EXPECT().SupportsCheckingIsBootstrapped().Return(false).Times(1)
+				machineMock.EXPECT().DrainNodeIfNeeded(gomock.Any()).Return(time.Duration(0), nil)
 				machineFactoryMock.EXPECT().NewMachine(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(machineMock, nil).Times(1)
 
 				infraClusterMock.EXPECT().GenerateInfraClusterClient(kubevirtMachine.Spec.InfraClusterSecretRef, kubevirtMachine.Namespace, machineContext.Context).Return(fakeClient, kubevirtMachine.Namespace, nil)
@@ -943,6 +947,7 @@ var _ = Describe("reconcile a kubevirt machine", func() {
 				machineMock.EXPECT().GenerateProviderID().Return("abc", nil).AnyTimes()
 				machineMock.EXPECT().SupportsCheckingIsBootstrapped().Return(true)
 				machineMock.EXPECT().IsBootstrapped().Return(false)
+				machineMock.EXPECT().DrainNodeIfNeeded(gomock.Any()).Return(time.Duration(0), nil)
 
 				machineFactoryMock.EXPECT().NewMachine(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(machineMock, nil).Times(1)
 
@@ -992,6 +997,7 @@ var _ = Describe("reconcile a kubevirt machine", func() {
 				machineMock.EXPECT().GenerateProviderID().Return("abc", nil).Times(1)
 				machineMock.EXPECT().SupportsCheckingIsBootstrapped().Return(true)
 				machineMock.EXPECT().IsBootstrapped().Return(true)
+				machineMock.EXPECT().DrainNodeIfNeeded(gomock.Any()).Return(time.Duration(0), nil)
 
 				machineFactoryMock.EXPECT().NewMachine(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(machineMock, nil).Times(1)
 
@@ -1007,6 +1013,97 @@ var _ = Describe("reconcile a kubevirt machine", func() {
 				Expect(conditions[0].Type).To(Equal(infrav1.BootstrapExecSucceededCondition))
 				Expect(conditions[0].Status).To(Equal(corev1.ConditionTrue))
 			})
+
+			It("should requeue on node draining", func() {
+				vmiReadyCondition := kubevirtv1.VirtualMachineInstanceCondition{
+					Type:   kubevirtv1.VirtualMachineInstanceReady,
+					Status: corev1.ConditionTrue,
+				}
+				vmi.Status.Conditions = append(vmi.Status.Conditions, vmiReadyCondition)
+				vmi.Status.Interfaces = []kubevirtv1.VirtualMachineInstanceNetworkInterface{
+
+					{
+						IP: "1.1.1.1",
+					},
+				}
+				sshKeySecret.Data["pub"] = []byte("shell")
+
+				objects := []client.Object{
+					cluster,
+					kubevirtCluster,
+					machine,
+					kubevirtMachine,
+					bootstrapSecret,
+					bootstrapUserDataSecret,
+					sshKeySecret,
+					vm,
+					vmi,
+				}
+
+				const requeueDurationSeconds = 3
+				machineMock.EXPECT().IsTerminal().Return(false, "", nil).Times(1)
+				machineMock.EXPECT().Exists().Return(true).Times(1)
+				machineMock.EXPECT().IsReady().Return(true).Times(1)
+				machineMock.EXPECT().Address().Return("1.1.1.1").Times(1)
+				machineMock.EXPECT().DrainNodeIfNeeded(gomock.Any()).Return(time.Second*requeueDurationSeconds, nil).Times(1)
+
+				machineFactoryMock.EXPECT().NewMachine(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(machineMock, nil).Times(1)
+
+				setupClient(machineFactoryMock, objects)
+
+				infraClusterMock.EXPECT().GenerateInfraClusterClient(kubevirtMachine.Spec.InfraClusterSecretRef, kubevirtMachine.Namespace, machineContext.Context).Return(fakeClient, kubevirtMachine.Namespace, nil)
+
+				res, err := kubevirtMachineReconciler.reconcileNormal(machineContext)
+				Expect(err).ShouldNot(HaveOccurred())
+
+				Expect(res.RequeueAfter).To(Equal(time.Second * requeueDurationSeconds))
+			})
+
+			It("should requeue on node draining error + requeue duration", func() {
+				vmiReadyCondition := kubevirtv1.VirtualMachineInstanceCondition{
+					Type:   kubevirtv1.VirtualMachineInstanceReady,
+					Status: corev1.ConditionTrue,
+				}
+				vmi.Status.Conditions = append(vmi.Status.Conditions, vmiReadyCondition)
+				vmi.Status.Interfaces = []kubevirtv1.VirtualMachineInstanceNetworkInterface{
+
+					{
+						IP: "1.1.1.1",
+					},
+				}
+				sshKeySecret.Data["pub"] = []byte("shell")
+
+				objects := []client.Object{
+					cluster,
+					kubevirtCluster,
+					machine,
+					kubevirtMachine,
+					bootstrapSecret,
+					bootstrapUserDataSecret,
+					sshKeySecret,
+					vm,
+					vmi,
+				}
+
+				const requeueDurationSeconds = 3
+				machineMock.EXPECT().IsTerminal().Return(false, "", nil).Times(1)
+				machineMock.EXPECT().Exists().Return(true).Times(1)
+				machineMock.EXPECT().IsReady().Return(true).Times(1)
+				machineMock.EXPECT().Address().Return("1.1.1.1").Times(1)
+				machineMock.EXPECT().DrainNodeIfNeeded(gomock.Any()).Return(time.Second*requeueDurationSeconds, fmt.Errorf("mock error")).Times(1)
+
+				machineFactoryMock.EXPECT().NewMachine(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(machineMock, nil).Times(1)
+
+				setupClient(machineFactoryMock, objects)
+
+				infraClusterMock.EXPECT().GenerateInfraClusterClient(kubevirtMachine.Spec.InfraClusterSecretRef, kubevirtMachine.Namespace, machineContext.Context).Return(fakeClient, kubevirtMachine.Namespace, nil)
+
+				res, err := kubevirtMachineReconciler.reconcileNormal(machineContext)
+				Expect(err).Should(HaveOccurred())
+				Expect(errors.Unwrap(err).Error()).Should(ContainSubstring("failed to drain node: mock error"))
+
+				Expect(res.RequeueAfter).To(Equal(time.Second * requeueDurationSeconds))
+			})
 		})
 	})
 	It("should detect when a previous Ready KubeVirtMachine is no longer ready due to vmi ready condition being false", func() {