Update vm idler (codeready-toolchain#647)

Author: rajivnathan

controllers/idler/idler_controller.go

@@ -13,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/dynamic"
+	"k8s.io/client-go/rest"
 	"k8s.io/client-go/scale"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -38,7 +39,8 @@ import (
 )
 
 const (
-	restartThreshold = 50
+	restartThreshold    = 50
+	vmSubresourceURLFmt = "/apis/subresources.kubevirt.io/%s"
 )
 
 var SupportedScaleResources = map[schema.GroupVersionKind]schema.GroupVersionResource{
@@ -63,6 +65,7 @@ type Reconciler struct {
 	Client              client.Client
 	Scheme              *runtime.Scheme
 	AllNamespacesClient client.Client
+	RestClient          rest.Interface
 	ScalesClient        scale.ScalesGetter
 	DynamicClient       dynamic.Interface
 	GetHostCluster      cluster.GetHostClusterFunc
@@ -79,6 +82,10 @@ type Reconciler struct {
 //+kubebuilder:rbac:groups=batch,resources=jobs,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines;virtualmachineinstances,verbs=get;list;watch;create;update;patch;delete
 
+// needed to stop the VMs - we need to make a PUT request for the "stop" subresource. Kubernetes internally classifies these as either create or update
+// based on the state of the existing object.
+//+kubebuilder:rbac:groups=subresources.kubevirt.io,resources=virtualmachines/stop,verbs=create;update
+
 // Reconcile reads that state of the cluster for an Idler object and makes changes based on the state read
 // and what is in the Idler.Spec
 // Note:
@@ -338,7 +345,7 @@ func (r *Reconciler) getUserEmailsFromMURs(ctx context.Context, hostCluster *clu
 
 // scaleControllerToZero checks if the object has an owner controller (Deployment, ReplicaSet, etc)
 // and scales the owner down to zero and returns "true".
-// Otherwise returns "false".
+// Otherwise, returns "false".
 // It also returns the parent controller type and name or empty strings if there is no parent controller.
 func (r *Reconciler) scaleControllerToZero(ctx context.Context, meta metav1.ObjectMeta) (string, string, bool, error) {
 	log.FromContext(ctx).Info("Scaling controller to zero", "name", meta.Name)
@@ -609,9 +616,14 @@ func (r *Reconciler) stopVirtualMachine(ctx context.Context, namespace string, o
 		return "", "", false, err
 	}
 
-	// patch the virtualmachine resource by setting spec.running to false in order to stop the VM
-	patch := []byte(`{"spec":{"running":false}}`)
-	_, err = r.DynamicClient.Resource(vmGVR).Namespace(namespace).Patch(ctx, vm.GetName(), types.MergePatchType, patch, metav1.PatchOptions{})
+	err = r.RestClient.Put().
+		AbsPath(fmt.Sprintf(vmSubresourceURLFmt, "v1")).
+		Namespace(vm.GetNamespace()).
+		Resource("virtualmachines").
+		Name(vm.GetName()).
+		SubResource("stop").
+		Do(ctx).
+		Error()
 	if err != nil {
 		return "", "", false, err
 	}

controllers/idler/idler_controller_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/codeready-toolchain/toolchain-common/pkg/cluster"
 	"github.com/codeready-toolchain/toolchain-common/pkg/condition"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test"
+	"gopkg.in/h2non/gock.v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 
@@ -46,6 +47,7 @@ const (
 	RestartCountWithinThresholdContainer2 = 24
 	RestartCountOverThreshold             = 52
 	TestIdlerTimeOutSeconds               = 540
+	apiEndpoint                           = "https://api.openshift.com:6443"
 )
 
 func TestReconcile(t *testing.T) {
@@ -217,9 +219,9 @@ func TestEnsureIdling(t *testing.T) {
 				StatefulSetScaledUp(podsTooEarlyToKill.statefulSet).
 				StatefulSetScaledUp(noise.statefulSet).
 				StatefulSetScaledUp(podsCrashLoopingWithinThreshold.statefulSet).
-				VMRunning(podsRunningForTooLong.virtualmachine).
-				VMRunning(podsTooEarlyToKill.virtualmachine).
-				VMRunning(noise.virtualmachine)
+				VMRunning(podsRunningForTooLong.vmStopCallCounter).
+				VMRunning(podsTooEarlyToKill.vmStopCallCounter).
+				VMRunning(noise.vmStopCallCounter)
 
 			// after golang 1.22 upgrade can use slices.Concat(podsTooEarlyToKill.allPods, podsRunningForTooLong.allPods, podsCrashLooping.allPods, podsCrashLoopingWithinThreshold.allPods)
 			// Tracked pods
@@ -273,9 +275,9 @@ func TestEnsureIdling(t *testing.T) {
 					StatefulSetScaledUp(podsTooEarlyToKill.statefulSet).
 					StatefulSetScaledUp(noise.statefulSet).
 					StatefulSetScaledUp(podsCrashLoopingWithinThreshold.statefulSet).
-					VMStopped(podsRunningForTooLong.virtualmachine).
-					VMRunning(podsTooEarlyToKill.virtualmachine).
-					VMRunning(noise.virtualmachine)
+					VMStopped(podsRunningForTooLong.vmStopCallCounter).
+					VMRunning(podsTooEarlyToKill.vmStopCallCounter).
+					VMRunning(noise.vmStopCallCounter)
 
 				// Only tracks pods that have not been deleted
 				memberoperatortest.AssertThatIdler(t, idler.Name, cl).
@@ -624,6 +626,9 @@ func TestEnsureIdlingFailed(t *testing.T) {
 			assertCanNotUpdateObject := func(inaccessible runtime.Object, errMsg string) {
 				// given
 				reconciler, req, cl, allCl, dynamicCl := prepareReconcileWithPodsRunningTooLong(t, idler)
+				gock.Off()
+				// mock stop call
+				mockStopVMCalls(".*", ".*", http.StatusInternalServerError)
 
 				update := allCl.MockUpdate
 				defer func() { allCl.MockUpdate = update }()
@@ -661,7 +666,7 @@ func TestEnsureIdlingFailed(t *testing.T) {
 			assertCanNotUpdateObject(&appsv1.StatefulSet{}, "can't update statefulset")
 			assertCanNotUpdateObject(&openshiftappsv1.DeploymentConfig{}, "can't update deploymentconfig")
 			assertCanNotUpdateObject(&corev1.ReplicationController{}, "can't update replicationcontroller")
-			assertCanNotUpdateObject(vm, "can't patch virtualmachine")
+			assertCanNotUpdateObject(vm, "an error on the server (\"\") has prevented the request from succeeding (put virtualmachines.authentication.k8s.io alex-stage-virtualmachine)")
 		})
 
 		t.Run("can't delete payloads", func(t *testing.T) {
@@ -1173,6 +1178,7 @@ type payloads struct {
 	replicationController  *corev1.ReplicationController
 	job                    *batchv1.Job
 	virtualmachine         *unstructured.Unstructured
+	vmStopCallCounter      *int
 	virtualmachineinstance *unstructured.Unstructured
 }
 
@@ -1319,6 +1325,9 @@ func preparePayloads(t *testing.T, r *Reconciler, namespace, namePrefix string,
 	_, err = r.DynamicClient.Resource(vmGVR).Namespace(namespace).Create(context.TODO(), vm, metav1.CreateOptions{})
 	require.NoError(t, err)
 
+	// mock stop call
+	stopCallCounter := mockStopVMCalls(namespace, vm.GetName(), http.StatusAccepted)
+
 	// VirtualMachineInstance
 	vmstartTime := metav1.NewTime(startTimes.vmStartTime)
 	vmi := &unstructured.Unstructured{}
@@ -1379,10 +1388,30 @@ func preparePayloads(t *testing.T, r *Reconciler, namespace, namePrefix string,
 		replicationController:  standaloneRC,
 		job:                    job,
 		virtualmachine:         vm,
+		vmStopCallCounter:      stopCallCounter,
 		virtualmachineinstance: vmi,
 	}
 }
 
+func mockStopVMCalls(namespace, name string, reply int) *int {
+	expPath := fmt.Sprintf("/apis/subresources.kubevirt.io/v1/namespaces/%s/virtualmachines/%s/stop", namespace, name)
+	stopCallCounter := new(int)
+	gock.New(apiEndpoint).
+		Put(expPath).
+		Persist().
+		AddMatcher(func(request *http.Request, request2 *gock.Request) (bool, error) {
+			// the matcher function is called before checking the path,
+			// so we need to verify that it's really the same VM
+			if request.URL.Path == expPath {
+				*stopCallCounter++
+			}
+			return true, nil
+		}).
+		Reply(reply).
+		BodyString("")
+	return stopCallCounter
+}
+
 func preparePayloadsSinglePod(t *testing.T, r *Reconciler, namespace, namePrefix string, startTime time.Time, conditions ...corev1.PodCondition) payloads {
 	sTime := metav1.NewTime(startTime)
 
@@ -1563,10 +1592,18 @@ func prepareReconcile(t *testing.T, name string, getHostClusterFunc func(fakeCli
 		return true, obj, nil
 	})
 
+	restClient, err := test.NewRESTClient("dummy-token", apiEndpoint)
+	require.NoError(t, err)
+	restClient.Client.Transport = gock.DefaultTransport
+	t.Cleanup(func() {
+		gock.OffAll()
+	})
+
 	r := &Reconciler{
 		Client:              fakeClient,
 		AllNamespacesClient: allNamespacesClient,
 		DynamicClient:       dynamicClient,
+		RestClient:          restClient,
 		ScalesClient:        &scalesClient,
 		Scheme:              s,
 		GetHostCluster:      getHostClusterFunc(fakeClient),

main.go

@@ -207,6 +207,12 @@ func main() {
 		os.Exit(1)
 	}
 
+	restClient, err := newRestClient(cfg)
+	if err != nil {
+		setupLog.Error(err, "unable to create scales client")
+		os.Exit(1)
+	}
+
 	dynamicClient, err := dynamic.NewForConfig(cfg)
 	if err != nil {
 		setupLog.Error(err, "unable to create dynamic client")
@@ -245,6 +251,7 @@ func main() {
 		Client:              mgr.GetClient(),
 		ScalesClient:        scalesClient,
 		DynamicClient:       dynamicClient,
+		RestClient:          restClient,
 		GetHostCluster:      cluster.GetHostCluster,
 		Namespace:           namespace,
 	}).SetupWithManager(mgr, allNamespacesCluster); err != nil {
@@ -323,6 +330,15 @@ func main() {
 
 }
 
+func newRestClient(config *rest.Config) (*rest.RESTClient, error) {
+	httpClient, err := rest.HTTPClientFor(config)
+	if err != nil {
+		return nil, err
+	}
+
+	return rest.RESTClientForConfigAndClient(config, httpClient)
+}
+
 func newScalesClient(config *rest.Config) (scale.ScalesGetter, error) {
 	c, err := kubernetes.NewForConfig(config)
 	if err != nil {

test/idler_assertion.go

@@ -17,15 +17,11 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	fakedynamic "k8s.io/client-go/dynamic/fake"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-var vmGVR = schema.GroupVersionResource{Group: "kubevirt.io", Version: "v1", Resource: "virtualmachines"}
-
 type IdlerAssertion struct {
 	idler          *toolchainv1alpha1.Idler
 	client         client.Client
@@ -275,20 +271,12 @@ func (a *IdleablePayloadAssertion) StatefulSetScaledUp(statefulSet *appsv1.State
 	return a
 }
 
-func (a *IdleablePayloadAssertion) VMRunning(vm *unstructured.Unstructured) *IdleablePayloadAssertion {
-	return a.vmRunning(vm, true)
-}
-
-func (a *IdleablePayloadAssertion) VMStopped(vm *unstructured.Unstructured) *IdleablePayloadAssertion {
-	return a.vmRunning(vm, false)
+func (a *IdleablePayloadAssertion) VMRunning(vmStopCallCounter *int) *IdleablePayloadAssertion {
+	assert.Empty(a.t, *vmStopCallCounter)
+	return a
 }
 
-func (a *IdleablePayloadAssertion) vmRunning(vm *unstructured.Unstructured, running bool) *IdleablePayloadAssertion {
-	vm, err := a.dynamicClient.Resource(vmGVR).Namespace(vm.GetNamespace()).Get(context.TODO(), vm.GetName(), metav1.GetOptions{})
-	require.NoError(a.t, err)
-	val, found, err := unstructured.NestedBool(vm.Object, "spec", "running")
-	require.NoError(a.t, err)
-	assert.True(a.t, found)
-	assert.Equal(a.t, running, val)
+func (a *IdleablePayloadAssertion) VMStopped(vmStopCallCounter *int) *IdleablePayloadAssertion {
+	assert.NotEmpty(a.t, *vmStopCallCounter)
 	return a
 }