VM with RunStrategyHalted now accepts manual stop request with gracep…

…eriod=0 Originally, RunStrategyHalted blocked all stop requests. Now, if a user needs to force stop a VM that has stalled/crashed during shutdown, they can issue a subsequent force stop command with a grace period of 0, which will be honored and send a new stop request. Stu suggested doing this with any request with a shorter grace period, but for now I have just done it with a grace period of 0. Signed-off-by: prnaraya <[email protected]>
cnchef · Jun 3, 2022 · a0531ea · a0531ea
1 parent abc9d0d
commit a0531ea
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 8 deletions.
diff --git a/pkg/virt-api/rest/subresource.go b/pkg/virt-api/rest/subresource.go
@@ -408,7 +408,7 @@ func (app *SubresourceAPIApp) RestartVMRequestHandler(request *restful.Request,
 				response.WriteHeader(http.StatusAccepted)
 				return
 			}
-			// set termincationGracePeriod and delete the VMI pod to trigger a forced restart
+			// set terminationGracePeriod and delete the VMI pod to trigger a forced restart
 			err = app.virtCli.CoreV1().Pods(namespace).Delete(context.Background(), vmiPodname, k8smetav1.DeleteOptions{GracePeriodSeconds: bodyStruct.GracePeriodSeconds})
 			if err != nil {
 				if !errors.IsNotFound(err) {
@@ -570,7 +570,7 @@ func (app *SubresourceAPIApp) StartVMRequestHandler(request *restful.Request, re
 }
 
 func (app *SubresourceAPIApp) StopVMRequestHandler(request *restful.Request, response *restful.Response) {
-	// RunStrategyHalted         -> doesn't make sense
+	// RunStrategyHalted         -> force stop if graceperiod=0, otherwise doesn't make sense
 	// RunStrategyManual         -> send stop request
 	// RunStrategyAlways         -> spec.running = false
 	// RunStrategyRerunOnFailure -> spec.running = false
@@ -615,6 +615,7 @@ func (app *SubresourceAPIApp) StopVMRequestHandler(request *restful.Request, res
 	patchType := types.MergePatchType
 	var patchErr error
 	if hasVMI && !vmi.IsFinal() && bodyStruct.GracePeriod != nil {
+
 		bodyString := getUpdateTerminatingSecondsGracePeriod(*bodyStruct.GracePeriod)
 		log.Log.Object(vmi).V(2).Infof("Patching VMI: %s", bodyString)
 		_, err = app.virtCli.VirtualMachineInstance(namespace).Patch(vmi.GetName(), patchType, []byte(bodyString), &k8smetav1.PatchOptions{DryRun: bodyStruct.DryRun})
@@ -630,8 +631,24 @@ func (app *SubresourceAPIApp) StopVMRequestHandler(request *restful.Request, res
 			writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf(vmNotRunning)), response)
 			return
 		}
-		writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf("%v does not support manual stop requests", v1.RunStrategyHalted)), response)
-		return
+		if vmi.Spec.TerminationGracePeriodSeconds == nil {
+			writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf("%v only supports manual stop requests with graceperiod=0", v1.RunStrategyHalted)), response)
+			return
+		}
+		if *vmi.Spec.TerminationGracePeriodSeconds != 0 {
+			writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf("%v only supports manual stop requests with graceperiod=0", v1.RunStrategyHalted)), response)
+			return
+		}
+		// same behavior as RunStrategyManual
+		patchType = types.JSONPatchType
+		bodyString, err := getChangeRequestJson(vm,
+			v1.VirtualMachineStateChangeRequest{Action: v1.StopRequest, UID: &vmi.UID})
+		if err != nil {
+			writeError(errors.NewInternalError(err), response)
+			return
+		}
+		log.Log.Object(vm).V(4).Infof(patchingVMStatusFmt, bodyString)
+		patchErr = app.statusUpdater.PatchStatus(vm, patchType, []byte(bodyString), &k8smetav1.PatchOptions{DryRun: bodyStruct.DryRun})
 	case v1.RunStrategyManual:
 		if !hasVMI || vmi.IsFinal() {
 			writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf(vmNotRunning)), response)

diff --git a/pkg/virt-api/rest/subresource_test.go b/pkg/virt-api/rest/subresource_test.go
@@ -1431,7 +1431,7 @@ var _ = Describe("VirtualMachineInstance Subresources", func() {
 
 			statusErr := ExpectStatusErrorWithCode(recorder, http.StatusConflict)
 			// check the msg string that would be presented to virtctl output
-			Expect(statusErr.Error()).To(ContainSubstring("Halted does not support manual stop requests"))
+			Expect(statusErr.Error()).To(ContainSubstring("Halted only supports manual stop requests with graceperiod=0"))
 		})
 
 		It("should fail on VM with RunStrategyHalted", func() {
@@ -1445,9 +1445,24 @@ var _ = Describe("VirtualMachineInstance Subresources", func() {
 
 			statusErr := ExpectStatusErrorWithCode(recorder, http.StatusConflict)
 			// check the msg string that would be presented to virtctl output
-			Expect(statusErr.Error()).To(ContainSubstring("Halted does not support manual stop requests"))
+			Expect(statusErr.Error()).To(ContainSubstring("Halted only supports manual stop requests with graceperiod=0"))
 		})
 
+		It("should not fail on VM with RunStrategyHalted and graceperiod=0", func() {
+			vm := newVirtualMachineWithRunStrategy(v1.RunStrategyHalted)
+			vmi := newVirtualMachineInstanceInPhase(v1.Running)
+			vmi.Spec.TerminationGracePeriodSeconds = &gracePeriodZero
+
+			vmClient.EXPECT().Get(vm.Name, &k8smetav1.GetOptions{}).Return(vm, nil)
+			vmiClient.EXPECT().Get(vm.Name, &k8smetav1.GetOptions{}).Return(vmi, nil)
+
+			vmClient.EXPECT().PatchStatus(vm.Name, types.JSONPatchType, gomock.Any(), &k8smetav1.PatchOptions{}).Return(vm, nil)
+
+			app.StopVMRequestHandler(request, response)
+
+			//Expect(response.Error()).ToNot(HaveOccurred())
+			Expect(response.StatusCode()).To(Equal(http.StatusAccepted))
+		})
 		DescribeTable("should not fail on VM with RunStrategy", func(runStrategy v1.VirtualMachineRunStrategy) {
 			vm := newVirtualMachineWithRunStrategy(runStrategy)
 			vmi := newVirtualMachineInstanceInPhase(v1.Running)

diff --git a/pkg/virtctl/vm/vm.go b/pkg/virtctl/vm/vm.go
@@ -464,7 +464,7 @@ func (o *Command) Run(args []string) error {
 			if gracePeriodIsSet(gracePeriod) {
 				err = virtClient.VirtualMachine(namespace).ForceStop(vmiName, &v1.StopOptions{GracePeriod: &gracePeriod, DryRun: dryRunOption})
 				if err != nil {
-					return fmt.Errorf("Error force stoping VirtualMachine, %v", err)
+					return fmt.Errorf("Error force stopping VirtualMachine, %v", err)
 				}
 			} else if !gracePeriodIsSet(gracePeriod) {
 				return fmt.Errorf("Can not force stop without gracePeriod")

diff --git a/tests/vm_test.go b/tests/vm_test.go
@@ -709,7 +709,7 @@ var _ = Describe("[rfe_id:1177][crit:medium][vendor:[email protected]][level:com
 			Expect(vmi.Status.VirtualMachineRevisionName).To(Equal(expectedVMRevisionName))
 			oldVMRevisionName := expectedVMRevisionName
 
-			By("Stoping the VM")
+			By("Stopping the VM")
 			newVM = stopVM(newVM)
 
 			By("Updating the VM template spec")