Skip to content

Commit

Permalink
Combined separate tests with duplicate logic
Browse files Browse the repository at this point in the history 
to reduce code repition and redundancy.

Signed-off-by: prnaraya <[email protected]>
  • Loading branch information
@prnaraya
prnaraya committed Jun 3, 2022
1 parent e97f729 commit 0105779
Show file tree
Hide file tree
Showing 2 changed files with 55 additions and 53 deletions.
40 changes: 21 additions & 19 deletions pkg/virt-api/rest/subresource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,17 @@ func getUpdateTerminatingSecondsGracePeriod(gracePeriod int64) string {
return fmt.Sprintf("{\"spec\":{\"terminationGracePeriodSeconds\": %d }}", gracePeriod)
}

func (app *SubresourceAPIApp) patchVMStatusStopped(vmi *v1.VirtualMachineInstance, vm *v1.VirtualMachine, response *restful.Response, bodyStruct *v1.StopOptions) (error, error) {
bodyString, err := getChangeRequestJson(vm,
v1.VirtualMachineStateChangeRequest{Action: v1.StopRequest, UID: &vmi.UID})
if err != nil {
writeError(errors.NewInternalError(err), response)
return nil, err
}
log.Log.Object(vm).V(4).Infof(patchingVMStatusFmt, bodyString)
return app.statusUpdater.PatchStatus(vm, types.JSONPatchType, []byte(bodyString), &k8smetav1.PatchOptions{DryRun: bodyStruct.DryRun}), nil
}

func (app *SubresourceAPIApp) MigrateVMRequestHandler(request *restful.Request, response *restful.Response) {
name := request.PathParameter("name")
namespace := request.PathParameter("namespace")
Expand Down Expand Up @@ -570,7 +581,7 @@ func (app *SubresourceAPIApp) StartVMRequestHandler(request *restful.Request, re
}

func (app *SubresourceAPIApp) StopVMRequestHandler(request *restful.Request, response *restful.Response) {
// RunStrategyHalted -> force stop if graceperiod=0, otherwise doesn't make sense
// RunStrategyHalted -> force stop if graceperiod in request is shorter than before, otherwise doesn't make sense
// RunStrategyManual -> send stop request
// RunStrategyAlways -> spec.running = false
// RunStrategyRerunOnFailure -> spec.running = false
Expand Down Expand Up @@ -612,9 +623,14 @@ func (app *SubresourceAPIApp) StopVMRequestHandler(request *restful.Request, res
return
}

var oldGracePeriodSeconds int64
patchType := types.MergePatchType
var patchErr error
if hasVMI && !vmi.IsFinal() && bodyStruct.GracePeriod != nil {
// used for stopping a VM with RunStrategyHalted
if vmi.Spec.TerminationGracePeriodSeconds != nil {
oldGracePeriodSeconds = *vmi.Spec.TerminationGracePeriodSeconds
}

bodyString := getUpdateTerminatingSecondsGracePeriod(*bodyStruct.GracePeriod)
log.Log.Object(vmi).V(2).Infof("Patching VMI: %s", bodyString)
Expand All @@ -631,40 +647,26 @@ func (app *SubresourceAPIApp) StopVMRequestHandler(request *restful.Request, res
writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf(vmNotRunning)), response)
return
}
if bodyStruct.GracePeriod == nil {
writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf("%v only supports manual stop requests with graceperiod=0", v1.RunStrategyHalted)), response)
return
}
if *bodyStruct.GracePeriod >= *vmi.Spec.TerminationGracePeriodSeconds {
writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf("%v only supports manual stop requests with graceperiod=0", v1.RunStrategyHalted)), response)
if bodyStruct.GracePeriod == nil || (vmi.Spec.TerminationGracePeriodSeconds != nil && *bodyStruct.GracePeriod >= oldGracePeriodSeconds) {
writeError(errors.NewConflict(v1.Resource("virtualmachine"), name, fmt.Errorf("%v only supports manual stop requests with a shorter graceperiod", v1.RunStrategyHalted)), response)
return
}
// same behavior as RunStrategyManual
patchType = types.JSONPatchType
bodyString, err := getChangeRequestJson(vm,
v1.VirtualMachineStateChangeRequest{Action: v1.StopRequest, UID: &vmi.UID})
patchErr, err = app.patchVMStatusStopped(vmi, vm, response, bodyStruct)
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)
return
}
// pass the buck and ask virt-controller to stop the VM. this way the
// VM will retain RunStrategy = manual
patchType = types.JSONPatchType
bodyString, err := getChangeRequestJson(vm,
v1.VirtualMachineStateChangeRequest{Action: v1.StopRequest, UID: &vmi.UID})
patchErr, err = app.patchVMStatusStopped(vmi, vm, response, bodyStruct)
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.RunStrategyRerunOnFailure, v1.RunStrategyAlways, v1.RunStrategyOnce:
bodyString := getRunningJson(vm, false)
log.Log.Object(vm).V(4).Infof(patchingVMFmt, bodyString)
Expand Down
68 changes: 34 additions & 34 deletions pkg/virt-api/rest/subresource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,12 @@ import (
"strings"
"sync"

"k8s.io/utils/pointer"

"github.com/golang/mock/gomock"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/testing"
"k8s.io/utils/pointer"

"github.com/emicklei/go-restful"
. "github.com/onsi/ginkgo/v2"
Expand Down Expand Up @@ -1431,52 +1430,53 @@ 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 only supports manual stop requests with graceperiod=0"))
})

It("should fail on VM with RunStrategyHalted", func() {
vm := newVirtualMachineWithRunStrategy(v1.RunStrategyHalted)
vmi := newVirtualMachineInstanceInPhase(v1.Running)

vmClient.EXPECT().Get(vm.Name, &k8smetav1.GetOptions{}).Return(vm, nil)
vmiClient.EXPECT().Get(vm.Name, &k8smetav1.GetOptions{}).Return(vmi, nil)

app.StopVMRequestHandler(request, response)

statusErr := ExpectStatusErrorWithCode(recorder, http.StatusConflict)
// check the msg string that would be presented to virtctl output
Expect(statusErr.Error()).To(ContainSubstring("Halted only supports manual stop requests with graceperiod=0"))
Expect(statusErr.Error()).To(ContainSubstring("Halted only supports manual stop requests with a shorter graceperiod"))
})

It("should not fail on VM with RunStrategyHalted and a shorter grace period", func() {
DescribeTable("for VM with RunStrategyHalted, should", func(terminationGracePeriod *int64, graceperiod *int64, shouldFail bool) {
vm := newVirtualMachineWithRunStrategy(v1.RunStrategyHalted)
vmi := newVirtualMachineInstanceInPhase(v1.Running)

var terminationGracePeriodSeconds int64 = 1800
vmi.Spec.TerminationGracePeriodSeconds = &terminationGracePeriodSeconds
vmi.Spec.TerminationGracePeriodSeconds = terminationGracePeriod

gracePeriodShorter := int64(600)
stopOptions := &v1.StopOptions{GracePeriod: &gracePeriodShorter}
stopOptions := &v1.StopOptions{GracePeriod: graceperiod}

bytesRepresentation, _ := json.Marshal(stopOptions)
bytesRepresentation, err := json.Marshal(stopOptions)
Expect(err).ToNot(HaveOccurred())
request.Request.Body = io.NopCloser(bytes.NewReader(bytesRepresentation))

vmClient.EXPECT().Get(vm.Name, &k8smetav1.GetOptions{}).Return(vm, nil)
vmiClient.EXPECT().Get(vm.Name, &k8smetav1.GetOptions{}).Return(vmi, nil)

vmiClient.EXPECT().Patch(vmi.Name, types.MergePatchType, gomock.Any(), gomock.Any()).DoAndReturn(
func(name string, patchType types.PatchType, body interface{}, opts *k8smetav1.PatchOptions) (interface{}, interface{}) {
//check that dryRun option has been propagated to patch request
Expect(opts.DryRun).To(BeEquivalentTo(stopOptions.DryRun))
return vm, nil
})
vmClient.EXPECT().PatchStatus(vm.Name, types.JSONPatchType, gomock.Any(), &k8smetav1.PatchOptions{}).Return(vm, nil)

if graceperiod != nil {
vmiClient.EXPECT().Patch(vmi.Name, types.MergePatchType, gomock.Any(), gomock.Any()).DoAndReturn(
func(name string, patchType types.PatchType, body interface{}, opts *k8smetav1.PatchOptions) (interface{}, interface{}) {
//check that dryRun option has been propagated to patch request
Expect(opts.DryRun).To(BeEquivalentTo(stopOptions.DryRun))
return vm, nil
})
}
if !shouldFail {
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))
})
if shouldFail {
statusErr := ExpectStatusErrorWithCode(recorder, http.StatusConflict)
// check the msg string that would be presented to virtctl output
Expect(statusErr.Error()).To(ContainSubstring("Halted only supports manual stop requests with a shorter graceperiod"))
} else {
Expect(response.Error()).ToNot(HaveOccurred())
Expect(response.StatusCode()).To(Equal(http.StatusAccepted))
}
},
Entry("fail with nil graceperiod", pointer.Int64(int64(1800)), nil, true),
Entry("fail with equal graceperiod", pointer.Int64(int64(1800)), pointer.Int64(int64(1800)), true),
Entry("fail with greater graceperiod", pointer.Int64(int64(1800)), pointer.Int64(int64(2400)), true),
Entry("not fail with non-nil graceperiod and nil termination graceperiod", nil, pointer.Int64(int64(1800)), false),
Entry("not fail with shorter graceperiod and non-nil termination graceperiod", pointer.Int64(int64(1800)), pointer.Int64(int64(800)), false),
)

DescribeTable("should not fail on VM with RunStrategy", func(runStrategy v1.VirtualMachineRunStrategy) {
vm := newVirtualMachineWithRunStrategy(runStrategy)
vmi := newVirtualMachineInstanceInPhase(v1.Running)
Expand Down

0 comments on commit 0105779

Please sign in to comment.