Skip to content

Commit

Permalink
Addressed code review comments.
Browse files Browse the repository at this point in the history 
Fixed flaky test and cleaned up the functional tests a little

Signed-off-by: Alexander Wels <[email protected]>
  • Loading branch information
@awels
awels committed Aug 4, 2022
1 parent 70df80a commit 1bc7fbd
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 69 deletions.
12 changes: 4 additions & 8 deletions pkg/virt-controller/watch/export/export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -445,12 +445,8 @@ func (ctrl *VMExportController) handleIsVmSnapshot(vmExport *exportv1.VirtualMac
func (ctrl *VMExportController) handlePodSucceededOrFailed(vmExport *exportv1.VirtualMachineExport, pod *corev1.Pod) error {
if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed {
// The server died or completed, delete the pod.
ctrl.Recorder.Eventf(vmExport, corev1.EventTypeWarning, exporterPodFailedOrCompletedEvent, "Exporter pod %s/%s succeeded or failed", pod.Namespace, pod.Name)
if err := ctrl.Client.CoreV1().Pods(vmExport.Namespace).Delete(context.Background(), pod.Name, metav1.DeleteOptions{}); err != nil {
return err
} else {
return nil
}
ctrl.Recorder.Eventf(vmExport, corev1.EventTypeWarning, exporterPodFailedOrCompletedEvent, "Exporter pod %s/%s is in phase %s", pod.Namespace, pod.Name, &pod.Status.Phase)
return ctrl.Client.CoreV1().Pods(vmExport.Namespace).Delete(context.Background(), pod.Name, metav1.DeleteOptions{})
}
return nil
}
Expand Down Expand Up @@ -1199,11 +1195,11 @@ func buildPemFromCert(cert *x509.Certificate) string {
}

func (ctrl *VMExportController) matchesOrWildCard(hostName, compare string) bool {
wildCard := fmt.Sprintf("*.%s", getDomainFromHost(hostName, ctrl.KubevirtNamespace))
wildCard := fmt.Sprintf("*.%s", getDomainFromHost(hostName))
return hostName == compare || wildCard == compare
}

func getDomainFromHost(host, namespace string) string {
func getDomainFromHost(host string) string {
if index := strings.Index(host, "."); index != -1 {
return host[index+1:]
}
Expand Down
17 changes: 13 additions & 4 deletions pkg/virt-controller/watch/snapshot/restore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -732,16 +732,22 @@ func (ctrl *VMRestoreController) createRestorePVC(
volumeRestore *snapshotv1.VolumeRestore,
sourceVmName, sourceVmNamespace string,
) error {
if volumeBackup.VolumeSnapshotName == nil {
if volumeBackup == nil || volumeBackup.VolumeSnapshotName == nil {
log.Log.Errorf("VolumeSnapshot name missing %+v", volumeBackup)
return fmt.Errorf("missing VolumeSnapshot name")
}

if vmRestore == nil {
return fmt.Errorf("missing vmRestore")
}
volumeSnapshot, err := ctrl.VolumeSnapshotProvider.GetVolumeSnapshot(vmRestore.Namespace, *volumeBackup.VolumeSnapshotName)
if err != nil {
return err
}

if volumeRestore == nil {
return fmt.Errorf("missing volumeRestore")
}
pvc := CreateRestorePVCDef(vmRestore.Name, volumeRestore.PersistentVolumeClaimName, volumeSnapshot, volumeBackup, sourceVmName, sourceVmNamespace)
target.Own(pvc)

Expand All @@ -754,6 +760,10 @@ func (ctrl *VMRestoreController) createRestorePVC(
}

func CreateRestorePVCDef(vmRestoreName, restorePVCName string, volumeSnapshot *vsv1.VolumeSnapshot, volumeBackup *snapshotv1.VolumeBackup, sourceVmName, sourceVmNamespace string) *corev1.PersistentVolumeClaim {
if volumeBackup == nil || volumeBackup.VolumeSnapshotName == nil {
log.Log.Errorf("VolumeSnapshot name missing %+v", volumeBackup)
return nil
}
sourcePVC := volumeBackup.PersistentVolumeClaim.DeepCopy()
pvc := &corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -764,11 +774,10 @@ func CreateRestorePVCDef(vmRestoreName, restorePVCName string, volumeSnapshot *v
Spec: sourcePVC.Spec,
}

if volumeBackup.VolumeSnapshotName == nil {
log.Log.Errorf("VolumeSnapshot name missing %+v", volumeBackup)
if volumeSnapshot == nil {
log.Log.Errorf("VolumeSnapshot missing %+v", volumeSnapshot)
return nil
}

if volumeSnapshot.Status != nil && volumeSnapshot.Status.RestoreSize != nil {
restorePVCSize, ok := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
// Update restore pvc size to be the maximum between the source PVC and the restore size
Expand Down
104 changes: 47 additions & 57 deletions tests/storage/export.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

corev1 "k8s.io/api/core/v1"
k8sv1 "k8s.io/api/core/v1"
networkingv1 "k8s.io/api/networking/v1"
storagev1 "k8s.io/api/storage/v1"
Expand All @@ -41,15 +40,13 @@ import (
"k8s.io/apimachinery/pkg/util/rand"
"k8s.io/utils/pointer"

v1 "kubevirt.io/api/core/v1"
virtv1 "kubevirt.io/api/core/v1"
exportv1 "kubevirt.io/api/export/v1alpha1"
snapshotv1 "kubevirt.io/api/snapshot/v1alpha1"
"kubevirt.io/client-go/kubecli"
"kubevirt.io/client-go/log"
cdiv1 "kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1"

"kubevirt.io/kubevirt/pkg/certificates/triple/cert"
k6ttypes "kubevirt.io/kubevirt/pkg/util/types"
"kubevirt.io/kubevirt/pkg/virt-operator/resource/generate/components"

Expand All @@ -61,6 +58,7 @@ import (
"kubevirt.io/kubevirt/tests/util"

routev1 "github.com/openshift/api/route/v1"

certutil "kubevirt.io/kubevirt/pkg/certificates/triple/cert"
)

Expand Down Expand Up @@ -984,7 +982,7 @@ var _ = SIGDescribe("Export", func() {
ingress := createIngress(tlsSecretName)
vmExport := createRunningPVCExport(sc, k8sv1.PersistentVolumeFilesystem)
Expect(vmExport.Status.Links.External.Cert).To(Equal(testCert))
certs, err := cert.ParseCertsPEM([]byte(vmExport.Status.Links.External.Cert))
certs, err := certutil.ParseCertsPEM([]byte(vmExport.Status.Links.External.Cert))
Expect(err).ToNot(HaveOccurred())
Expect(certs).To(HaveLen(1))
prefix := fmt.Sprintf("%s-%s", components.VirtExportProxyServiceName, flags.KubeVirtInstallNamespace)
Expand All @@ -1010,7 +1008,7 @@ var _ = SIGDescribe("Export", func() {
Skip("Not on openshift")
}
vmExport := createRunningPVCExport(sc, k8sv1.PersistentVolumeFilesystem)
certs, err := cert.ParseCertsPEM([]byte(vmExport.Status.Links.External.Cert))
certs, err := certutil.ParseCertsPEM([]byte(vmExport.Status.Links.External.Cert))
Expect(err).ToNot(HaveOccurred())
Expect(certs).To(HaveLen(1))
route := getExportRoute()
Expand All @@ -1027,7 +1025,7 @@ var _ = SIGDescribe("Export", func() {
})
})

waitForDisksComplete := func(vm *v1.VirtualMachine) {
waitForDisksComplete := func(vm *virtv1.VirtualMachine) {
for _, volume := range vm.Spec.Template.Spec.Volumes {
if volume.DataVolume != nil {
Eventually(func() cdiv1.DataVolumePhase {
Expand All @@ -1041,22 +1039,22 @@ var _ = SIGDescribe("Export", func() {
}
}

createVM := func(vm *v1.VirtualMachine) *v1.VirtualMachine {
createVM := func(vm *virtv1.VirtualMachine) *virtv1.VirtualMachine {
vm, err := virtClient.VirtualMachine(vm.Namespace).Create(vm)
Expect(err).ToNot(HaveOccurred())
waitForDisksComplete(vm)
return vm
}

newSnapshot := func(vm *v1.VirtualMachine) *snapshotv1.VirtualMachineSnapshot {
newSnapshot := func(vm *virtv1.VirtualMachine) *snapshotv1.VirtualMachineSnapshot {
apiGroup := "kubevirt.io"
return &snapshotv1.VirtualMachineSnapshot{
ObjectMeta: metav1.ObjectMeta{
Name: "snapshot-" + vm.Name,
Namespace: vm.Namespace,
},
Spec: snapshotv1.VirtualMachineSnapshotSpec{
Source: corev1.TypedLocalObjectReference{
Source: k8sv1.TypedLocalObjectReference{
APIGroup: &apiGroup,
Kind: "VirtualMachine",
Name: vm.Name,
Expand Down Expand Up @@ -1103,64 +1101,56 @@ var _ = SIGDescribe("Export", func() {
return snapshot
}

verifyMultiLinksInternal := func(vmExport *exportv1.VirtualMachineExport, link1Format exportv1.ExportVolumeFormat, link1Url string, link2Format exportv1.ExportVolumeFormat, link2Url string, link3Format exportv1.ExportVolumeFormat, link3Url string, link4Format exportv1.ExportVolumeFormat, link4Url string) {
verifyLinksInternal := func(vmExport *exportv1.VirtualMachineExport, expectedVolumeFormats ...exportv1.VirtualMachineExportVolumeFormat) {
Expect(vmExport.Status).ToNot(BeNil())
Expect(vmExport.Status.Links).ToNot(BeNil())
Expect(vmExport.Status.Links.Internal).NotTo(BeNil())
Expect(vmExport.Status.Links.Internal.Cert).NotTo(BeEmpty())
Expect(vmExport.Status.Links.Internal.Volumes).To(HaveLen(2))
Expect(vmExport.Status.Links.Internal.Volumes[0].Formats).To(HaveLen(2))
Expect(vmExport.Status.Links.Internal.Volumes[0].Formats).To(ContainElements(exportv1.VirtualMachineExportVolumeFormat{
Format: link1Format,
Url: link1Url,
}, exportv1.VirtualMachineExportVolumeFormat{
Format: link2Format,
Url: link2Url,
}))
Expect(vmExport.Status.Links.Internal.Volumes[1].Formats).To(HaveLen(2))
Expect(vmExport.Status.Links.Internal.Volumes[1].Formats).To(ContainElements(exportv1.VirtualMachineExportVolumeFormat{
Format: link3Format,
Url: link3Url,
}, exportv1.VirtualMachineExportVolumeFormat{
Format: link4Format,
Url: link4Url,
}))
Expect(vmExport.Status.Links.Internal.Volumes).To(HaveLen(len(expectedVolumeFormats) / 2))
for _, expectedVolume := range expectedVolumeFormats {
found := false
for _, volume := range vmExport.Status.Links.Internal.Volumes {
Expect(volume.Formats).To(HaveLen(2))
for _, format := range volume.Formats {
if format == expectedVolume {
found = true
}
}
}
Expect(found).To(BeTrue())
}
}

verifyMultiKubevirtInternal := func(vmExport *exportv1.VirtualMachineExport, exportName, namespace, volumeName1, volumeName2 string) {
verifyMultiLinksInternal(vmExport,
exportv1.KubeVirtRaw,
fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName1),
exportv1.KubeVirtGz,
fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img.gz", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName1),
exportv1.KubeVirtRaw,
fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName2),
exportv1.KubeVirtGz,
fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img.gz", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName2))
}

verifyLinksInternal := func(vmExport *exportv1.VirtualMachineExport, link1Format exportv1.ExportVolumeFormat, link1Url string, link2Format exportv1.ExportVolumeFormat, link2Url string) {
Expect(vmExport.Status).ToNot(BeNil())
Expect(vmExport.Status.Links).ToNot(BeNil())
Expect(vmExport.Status.Links.Internal).NotTo(BeNil())
Expect(vmExport.Status.Links.Internal.Cert).NotTo(BeEmpty())
Expect(vmExport.Status.Links.Internal.Volumes).To(HaveLen(1))
Expect(vmExport.Status.Links.Internal.Volumes[0].Formats).To(HaveLen(2))
Expect(vmExport.Status.Links.Internal.Volumes[0].Formats).To(ContainElements(exportv1.VirtualMachineExportVolumeFormat{
Format: link1Format,
Url: link1Url,
}, exportv1.VirtualMachineExportVolumeFormat{
Format: link2Format,
Url: link2Url,
}))
verifyLinksInternal(vmExport,
exportv1.VirtualMachineExportVolumeFormat{
Format: exportv1.KubeVirtRaw,
Url: fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName1),
},
exportv1.VirtualMachineExportVolumeFormat{
Format: exportv1.KubeVirtGz,
Url: fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img.gz", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName1),
},
exportv1.VirtualMachineExportVolumeFormat{
Format: exportv1.KubeVirtRaw,
Url: fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName2),
},
exportv1.VirtualMachineExportVolumeFormat{
Format: exportv1.KubeVirtGz,
Url: fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img.gz", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName2),
})
}

verifyKubevirtInternal := func(vmExport *exportv1.VirtualMachineExport, exportName, namespace, volumeName string) {
verifyLinksInternal(vmExport,
exportv1.KubeVirtRaw,
fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName),
exportv1.KubeVirtGz,
fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img.gz", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName))
exportv1.VirtualMachineExportVolumeFormat{
Format: exportv1.KubeVirtRaw,
Url: fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName),
},
exportv1.VirtualMachineExportVolumeFormat{
Format: exportv1.KubeVirtGz,
Url: fmt.Sprintf("https://%s.%s.svc/volumes/%s/disk.img.gz", fmt.Sprintf("%s-%s", exportPrefix, exportName), namespace, volumeName),
})
}

It("should create export from VMSnapshot", func() {
Expand Down Expand Up @@ -1207,7 +1197,7 @@ var _ = SIGDescribe("Export", func() {
if !exists {
Skip("Skip test when Filesystem storage is not present")
}
blankDv := libstorage.NewRandomBlankDataVolume(util.NamespaceTestDefault, sc, "64Mi", corev1.ReadWriteOnce, k8sv1.PersistentVolumeFilesystem)
blankDv := libstorage.NewRandomBlankDataVolume(util.NamespaceTestDefault, sc, "64Mi", k8sv1.ReadWriteOnce, k8sv1.PersistentVolumeFilesystem)
vm := tests.NewRandomVMWithDataVolumeAndUserDataInStorageClass(
cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskCirros),
util.NamespaceTestDefault,
Expand Down

0 comments on commit 1bc7fbd

Please sign in to comment.