Skip to content

Commit

Permalink
Fix review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Shelly Kagan <[email protected]>
  • Loading branch information
@ShellyKa13
ShellyKa13 committed Oct 2, 2022
1 parent 83717fa commit 197ab3a
Show file tree
Hide file tree
Showing 6 changed files with 97 additions and 76 deletions.
16 changes: 10 additions & 6 deletions pkg/virtctl/memorydump/memorydump.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,15 @@ func generatePVC(size *resource.Quantity, claimName, namespace, storageClass, ac
pvc.Spec.StorageClassName = &storageClass
}

if accessMode == string(k8sv1.ReadOnlyMany) {
return nil, fmt.Errorf("cannot dump memory to a readonly pvc, use either ReadWriteOnce or ReadWriteMany if supported")
} else if accessMode != "" && accessMode != string(k8sv1.ReadWriteOnce) && accessMode != string(k8sv1.ReadWriteMany) {
return nil, fmt.Errorf("invalid access mode, use either ReadWriteOnce or ReadWriteMany if supported")
}
if accessMode != "" {
if accessMode == string(k8sv1.ReadOnlyMany) {
return nil, fmt.Errorf("cannot dump memory to a readonly pvc, use either ReadWriteOnce or ReadWriteMany if supported")
}
// TODO: fix when issue: https://github.com/kubevirt/containerized-data-importer/issues/2365 is done
if accessMode != string(k8sv1.ReadWriteOnce) && accessMode != string(k8sv1.ReadWriteMany) {
return nil, fmt.Errorf("invalid access mode, use either ReadWriteOnce or ReadWriteMany if supported")
}

pvc.Spec.AccessModes = []k8sv1.PersistentVolumeAccessMode{k8sv1.PersistentVolumeAccessMode(accessMode)}
} else {
pvc.Spec.AccessModes = []k8sv1.PersistentVolumeAccessMode{k8sv1.ReadWriteOnce}
Expand Down Expand Up @@ -304,7 +307,8 @@ func downloadMemoryDump(namespace, vmName string, virtClient kubecli.KubevirtCli
claimName, err := WaitMemoryDumpComplete(virtClient, namespace, vmName, processingWaitInterval, processingWaitTotal)
if err != nil {
return err
} else if claimName == "" {
}
if claimName == "" {
return fmt.Errorf("claim name not on vm memory dump request")
}
exportSource := k8sv1.TypedLocalObjectReference{
Expand Down
18 changes: 9 additions & 9 deletions pkg/virtctl/memorydump/memorydump_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ var _ = Describe("MemoryDump", func() {
kubecli.MockKubevirtClientInstance.EXPECT().CoreV1().Return(coreClient.CoreV1()).AnyTimes()
})

expectGetCDIConfig := func() {
handleGetCDIConfig := func() {
kubecli.MockKubevirtClientInstance.EXPECT().CdiClient().Return(cdiClient).AnyTimes()
}

Expand Down Expand Up @@ -185,7 +185,7 @@ var _ = Describe("MemoryDump", func() {
})
}

expectPVCGet := func(pvc *k8sv1.PersistentVolumeClaim) {
handlePVCGet := func(pvc *k8sv1.PersistentVolumeClaim) {
coreClient.Fake.PrependReactor("get", "persistentvolumeclaims", func(action testing.Action) (bool, runtime.Object, error) {
get, ok := action.(testing.GetAction)
Expect(ok).To(BeTrue())
Expand Down Expand Up @@ -247,7 +247,7 @@ var _ = Describe("MemoryDump", func() {

It("should fail call memory dump subresource with create-claim and existing pvc", func() {
expectGetVMNoAssociatedMemoryDump()
expectPVCGet(pvcSpec())
handlePVCGet(pvcSpec())
commandAndArgs := []string{"memory-dump", "get", "testvm", claimNameFlag, createClaimFlag}
cmd := clientcmd.NewVirtctlCommand(commandAndArgs...)
res := cmd.Execute()
Expand All @@ -267,7 +267,7 @@ var _ = Describe("MemoryDump", func() {
})

DescribeTable("should fail call memory dump subresource with invalid access mode", func(accessMode, expectedErr string) {
expectGetCDIConfig()
handleGetCDIConfig()
expectGetVMNoAssociatedMemoryDump()
expectGetVMI()
commandAndArgs := []string{"memory-dump", "get", "testvm", claimNameFlag, createClaimFlag, fmt.Sprintf("--access-mode=%s", accessMode)}
Expand All @@ -281,7 +281,7 @@ var _ = Describe("MemoryDump", func() {
)

DescribeTable("should create pvc for memory dump and call subresource", func(storageclass, accessMode string) {
expectGetCDIConfig()
handleGetCDIConfig()
expectGetVMNoAssociatedMemoryDump()
expectGetVMI()
expectPVCCreate(claimName, storageclass, accessMode)
Expand Down Expand Up @@ -376,8 +376,8 @@ var _ = Describe("MemoryDump", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtGz),
},
}, secretName)
utils.ExpectSecretGet(coreClient, secretName)
utils.ExpectVMExportCreate(vmExportClient, vmexport)
utils.HandleSecretGet(coreClient, secretName)
utils.HandleVMExportCreate(vmExportClient, vmexport)

commandAndArgs := []string{"memory-dump", "get", "testvm", outputFileFlag}
cmd := clientcmd.NewVirtctlCommand(commandAndArgs...)
Expand All @@ -393,8 +393,8 @@ var _ = Describe("MemoryDump", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtGz),
},
}, secretName)
utils.ExpectSecretGet(coreClient, secretName)
utils.ExpectVMExportCreate(vmExportClient, vmexport)
utils.HandleSecretGet(coreClient, secretName)
utils.HandleVMExportCreate(vmExportClient, vmexport)

commandAndArgs := []string{"memory-dump", "download", "testvm", outputFileFlag}
cmd := clientcmd.NewVirtctlCommand(commandAndArgs...)
Expand Down
10 changes: 4 additions & 6 deletions pkg/virtctl/utils/test_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func VMExportSpec(name, namespace, kind, resourceName, secretName string) *expor
return vmexport
}

func ExpectVMExportGet(client *kubevirtfake.Clientset, vme *exportv1.VirtualMachineExport, vmexportName string) {
func HandleVMExportGet(client *kubevirtfake.Clientset, vme *exportv1.VirtualMachineExport, vmexportName string) {
client.Fake.PrependReactor("get", "virtualmachineexports", func(action testing.Action) (bool, runtime.Object, error) {
get, ok := action.(testing.GetAction)
Expect(ok).To(BeTrue())
Expand All @@ -76,7 +76,7 @@ func ExpectVMExportGet(client *kubevirtfake.Clientset, vme *exportv1.VirtualMach
})
}

func ExpectVMExportCreate(client *kubevirtfake.Clientset, vme *exportv1.VirtualMachineExport) {
func HandleVMExportCreate(client *kubevirtfake.Clientset, vme *exportv1.VirtualMachineExport) {
client.Fake.PrependReactor("create", "virtualmachineexports", func(action testing.Action) (handled bool, obj runtime.Object, err error) {
create, ok := action.(testing.CreateAction)
Expect(ok).To(BeTrue())
Expand All @@ -88,12 +88,12 @@ func ExpectVMExportCreate(client *kubevirtfake.Clientset, vme *exportv1.VirtualM
}

Expect(ok).To(BeTrue())
ExpectVMExportGet(client, vme, vme.Name)
HandleVMExportGet(client, vme, vme.Name)
return true, vme, nil
})
}

func ExpectSecretGet(k8sClient *fakek8sclient.Clientset, secretName string) {
func HandleSecretGet(k8sClient *fakek8sclient.Clientset, secretName string) {
secret := &v1.Secret{
ObjectMeta: k8smetav1.ObjectMeta{
Name: secretName,
Expand All @@ -106,13 +106,11 @@ func ExpectSecretGet(k8sClient *fakek8sclient.Clientset, secretName string) {
}

k8sClient.Fake.PrependReactor("get", "secrets", func(action testing.Action) (handled bool, obj runtime.Object, err error) {
fmt.Println("GETTTTTTTTTTTTT secret ")
get, ok := action.(testing.GetAction)
Expect(ok).To(BeTrue())
Expect(get.GetNamespace()).To(Equal(k8smetav1.NamespaceDefault))
Expect(get.GetName()).To(Equal(secretName))
if secret == nil {
fmt.Println("secret not found")
return true, nil, errors.NewNotFound(v1.Resource("Secret"), secretName)
}
return true, secret, nil
Expand Down
44 changes: 22 additions & 22 deletions pkg/virtctl/vmexport/vmexport_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ var _ = Describe("vmexport", func() {
It("VirtualMachineExport already exists when using 'create'", func() {
testInit(http.StatusOK)
vmexport := utils.VMExportSpec(vmexportName, metav1.NamespaceDefault, "pvc", "test-pvc", secretName)
utils.ExpectVMExportGet(vmExportClient, vmexport, vmexportName)
utils.HandleVMExportGet(vmExportClient, vmexport, vmexportName)
cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, CREATE, vmexportName, setflag(PVC_FLAG, "test-pvc"))
err := cmd()
Expect(err).To(HaveOccurred())
Expand Down Expand Up @@ -125,7 +125,7 @@ var _ = Describe("vmexport", func() {
testInit(http.StatusOK)
vme := utils.VMExportSpec(vmexportName, metav1.NamespaceDefault, "pvc", "test-pvc", secretName)
vme.Status = utils.GetVMEStatus(nil, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"), setflag(VOLUME_FLAG, volumeName))
err := cmd()
Expand All @@ -147,7 +147,7 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtRaw),
},
}, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"), setflag(VOLUME_FLAG, volumeName))
err := cmd()
Expand All @@ -169,7 +169,7 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtRaw),
},
}, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"))
err := cmd()
Expand All @@ -182,7 +182,7 @@ var _ = Describe("vmexport", func() {
testInit(http.StatusOK)
vme := utils.VMExportSpec(vmexportName, metav1.NamespaceDefault, "pvc", "test-pvc", secretName)
vme.Status = utils.GetVMEStatus([]exportv1.VirtualMachineExportVolume{{Name: volumeName}}, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"), setflag(VOLUME_FLAG, volumeName))
err := cmd()
Expand All @@ -200,7 +200,7 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.Dir),
},
}, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"), setflag(VOLUME_FLAG, volumeName))
err := cmd()
Expand All @@ -218,7 +218,7 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtRaw),
},
}, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)
// Adding a new reactor so the client returns a nil secret
kubeClient.Fake.PrependReactor("create", "secrets", func(action testing.Action) (handled bool, obj runtime.Object, err error) { return false, nil, nil })

Expand All @@ -238,8 +238,8 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtRaw),
},
}, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.ExpectSecretGet(kubeClient, secretName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleSecretGet(kubeClient, secretName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"), setflag(VOLUME_FLAG, volumeName))
err := cmd()
Expand Down Expand Up @@ -287,15 +287,15 @@ var _ = Describe("vmexport", func() {

// Create tests
It("VirtualMachineExport is created succesfully", func() {
utils.ExpectVMExportCreate(vmExportClient, nil)
utils.HandleVMExportCreate(vmExportClient, nil)
cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, CREATE, vmexportName, setflag(PVC_FLAG, "test-pvc"))
err := cmd()
Expect(err).ToNot(HaveOccurred())
})

// Delete tests
It("VirtualMachineExport is deleted succesfully", func() {
expectVMExportDelete(vmExportClient, vmexportName)
handleVMExportDelete(vmExportClient, vmexportName)
cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DELETE, vmexportName)
err := cmd()
Expect(err).ToNot(HaveOccurred())
Expand All @@ -317,8 +317,8 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtGz),
},
}, secretName)
utils.ExpectSecretGet(kubeClient, secretName)
utils.ExpectVMExportGet(vmExportClient, vmexport, vmexportName)
utils.HandleSecretGet(kubeClient, secretName)
utils.HandleVMExportGet(vmExportClient, vmexport, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(VOLUME_FLAG, volumeName), setflag(OUTPUT_FLAG, "test-pvc"), INSECURE_FLAG)
err := cmd()
Expand All @@ -333,8 +333,8 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtGz),
},
}, secretName)
utils.ExpectSecretGet(kubeClient, secretName)
utils.ExpectVMExportCreate(vmExportClient, vmexport)
utils.HandleSecretGet(kubeClient, secretName)
utils.HandleVMExportCreate(vmExportClient, vmexport)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(PVC_FLAG, "test-pvc"), setflag(VOLUME_FLAG, volumeName), setflag(OUTPUT_FLAG, "test-pvc"), INSECURE_FLAG)
err := cmd()
Expand All @@ -349,8 +349,8 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtRaw),
},
}, secretName)
utils.ExpectSecretGet(kubeClient, secretName)
utils.ExpectVMExportGet(vmExportClient, vmexport, vmexportName)
utils.HandleSecretGet(kubeClient, secretName)
utils.HandleVMExportGet(vmExportClient, vmexport, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(VOLUME_FLAG, volumeName), setflag(OUTPUT_FLAG, "test-pvc"), INSECURE_FLAG)
err := cmd()
Expand All @@ -366,8 +366,8 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtRaw),
},
}, secretName)
utils.ExpectSecretGet(kubeClient, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleSecretGet(kubeClient, secretName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"), setflag(VOLUME_FLAG, volumeName))
err := cmd()
Expand All @@ -383,8 +383,8 @@ var _ = Describe("vmexport", func() {
Formats: utils.GetExportVolumeFormat(server.URL, exportv1.KubeVirtRaw),
},
}, secretName)
utils.ExpectSecretGet(kubeClient, secretName)
utils.ExpectVMExportGet(vmExportClient, vme, vmexportName)
utils.HandleSecretGet(kubeClient, secretName)
utils.HandleVMExportGet(vmExportClient, vme, vmexportName)

cmd := clientcmd.NewRepeatableVirtctlCommand(commandName, DOWNLOAD, vmexportName, setflag(OUTPUT_FLAG, "disk.img"))
err := cmd()
Expand Down Expand Up @@ -465,7 +465,7 @@ var _ = Describe("vmexport", func() {
})
})

func expectVMExportDelete(client *kubevirtfake.Clientset, name string) {
func handleVMExportDelete(client *kubevirtfake.Clientset, name string) {
client.Fake.PrependReactor("delete", "virtualmachineexports", func(action testing.Action) (handled bool, obj runtime.Object, err error) {
delete, ok := action.(testing.DeleteAction)
Expect(ok).To(BeTrue())
Expand Down
20 changes: 16 additions & 4 deletions tests/storage/export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"context"
"crypto/x509"
"encoding/pem"
goerrors "errors"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -1731,10 +1732,9 @@ var _ = SIGDescribe("Export", func() {
})

AfterEach(func() {
_, err := os.Stat(outputFile)
Expect(err).ToNot(HaveOccurred())
err = os.Remove(outputFile)
Expect(err).ToNot(HaveOccurred())
if err := os.Remove(outputFile); err != nil && !goerrors.Is(err, os.ErrNotExist) {
Fail(err.Error())
}
})

It("Download succeeds creating and downloading a vmexport using PVC source", func() {
Expand All @@ -1753,6 +1753,8 @@ var _ = SIGDescribe("Export", func() {

err = virtctlCmd()
Expect(err).ToNot(HaveOccurred())
_, err := os.Stat(outputFile)
Expect(err).ToNot(HaveOccurred())
})

It("Download succeeds creating and downloading a vmexport using Snapshot source", func() {
Expand Down Expand Up @@ -1791,6 +1793,8 @@ var _ = SIGDescribe("Export", func() {

err = virtctlCmd()
Expect(err).ToNot(HaveOccurred())
_, err := os.Stat(outputFile)
Expect(err).ToNot(HaveOccurred())
})

It("Download succeeds creating and downloading a vmexport using VM source", func() {
Expand Down Expand Up @@ -1827,6 +1831,8 @@ var _ = SIGDescribe("Export", func() {

err = virtctlCmd()
Expect(err).ToNot(HaveOccurred())
_, err := os.Stat(outputFile)
Expect(err).ToNot(HaveOccurred())
})

It("Download succeeds with an already existing vmexport", func() {
Expand All @@ -1844,6 +1850,8 @@ var _ = SIGDescribe("Export", func() {

err = virtctlCmd()
Expect(err).ToNot(HaveOccurred())
_, err := os.Stat(outputFile)
Expect(err).ToNot(HaveOccurred())
})

It("Download succeeds with a vmexport without user-defined TokenSecretRef", func() {
Expand All @@ -1869,6 +1877,8 @@ var _ = SIGDescribe("Export", func() {

err = virtctlCmd()
Expect(err).ToNot(HaveOccurred())
_, err := os.Stat(outputFile)
Expect(err).ToNot(HaveOccurred())
})

It("Download succeeds and keeps the vmexport after finishing the download", func() {
Expand All @@ -1887,6 +1897,8 @@ var _ = SIGDescribe("Export", func() {
err = virtctlCmd()
Expect(err).ToNot(HaveOccurred())
checkForReadyExport(vmeName)
_, err := os.Stat(outputFile)
Expect(err).ToNot(HaveOccurred())
})
})
})
Expand Down
Loading

0 comments on commit 197ab3a

Please sign in to comment.