test(CNV-43603): add disk-uploader functional tests #549

codingben · 2024-11-12T14:39:16Z

What this PR does / why we need it:

Deploy a new Cirros VM to export from,
and run disk-uploader as Task to execute
extraction process and upload it to the
Quay container registry.

Release note:

None

kubevirt-bot · 2024-11-12T14:39:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign 0xfelix for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-11-12T14:39:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: codingben
Once this PR has been reviewed and has the lgtm label, please assign 0xfelix for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codingben · 2024-11-12T14:39:42Z

/cc @ksimon1 @akrejcir

codingben · 2024-11-12T14:40:09Z

@ksimon1 @0xFelix Can you please take a look?

0xFelix · 2024-11-12T16:04:29Z

test/disk_uploader_test.go

+				ExportSourceKind: "vm",
+				ExportSourceName: vmName,
+				VolumeName:       dvName,
+				ImageDestination: "quay.io/boukhano/e2e-example-vm", // TODO: Use quay.io/kubevirt/e2e-example-vm


Is there a registry local to the test cluster, that can be used?

I had a discussion with @ksimon1 about it to keep using an external registry that allows you also to test it on Kubernetes cluster. As you know, only OpenShift has built-in container registry.

test/disk_uploader_test.go

0xFelix · 2024-11-12T16:06:56Z

test/disk_uploader_test.go

+			"accessKeyId": []byte(""), // TODO: Get value from OCP CI
+			"secretKey":   []byte(""), // TODO: Get value from OCP CI


Please don't add any credentials to this repo! Can we do this differently with a local registry?

There will be no credentials in the repo, since the OCP ci has a secret vault, which can be used for populating the secrets in test

There will be no secrets in this repository. Both accessKeyId and secretKey values will be set from the environment variables (aka Secrets in the OCP CI).

@ksimon1 How do you think we'll store the registry credenetials when running in developer mode, locally in the developer machine? (not in a CI)

0xFelix · 2024-11-12T16:07:53Z

test/disk_uploader_test.go

+			CreateTaskRun().
+			ExpectSuccess().
+			ExpectLogs(config.GetAllExpectedLogs()...).
+			ExpectResults(nil)


Query the repository after to verify disk was uploaded?

Yep good idea, need to check it.

Are you going to add a check?

Yes. We may want to delete the repository after completing running the tests. If we won't delete it, and re-run the functional tests, it may be false check.

codingben · 2024-11-13T14:02:07Z

@0xFelix @ksimon1 This revision adds the usage of libvmi and libdv.

CI or developer locally expected to set REGISTRY_IMAGE_DESTINATION, REGISTRY_ACCESS_KEY_ID and REGISTRY_SECRET_KEY environment variables.

codingben · 2024-11-13T14:04:25Z

@ksimon1 @0xFelix Currently there is a single test case: Extracts disk from VM and upload to container registry. Do you think we should also test scenarios of extracting it from VMSnapshot or PVC, or it's enough to have only VM? I don't want to end up re-testing and over-testing Export API.

0xFelix · 2024-11-14T09:37:11Z

test/disk_uploader_test.go

+func createAndWaitForVM(client kubevirtcliv1.KubevirtClient, namespace, name string, timeout time.Duration) {
+	dataVolume := libdv.NewDataVolume(
+		libdv.WithRegistryURLSource("docker://quay.io/kubevirt/cirros-container-disk-demo"),
+		libdv.WithForceBindAnnotation(),


Should not be required, if the VM was started at least once.

VM can't be exported if started and it's running. It should be stopped.

VM is created as stopped by default.

test/disk_uploader_test.go

Deploy a new Cirros VM to export from, and run disk-uploader as Task to execute extraction process and upload it to the Quay container registry. Signed-off-by: Ben Oukhanov <[email protected]>

codingben · 2024-11-14T13:17:47Z

@ksimon1 @0xFelix This revision will make e2e tests work, as tested locally it works now and image was pushed to the Quay container registry [1].

Can you please review?

[1] https://quay.io/repository/boukhano/e2e-tests-example-vm

0xFelix · 2024-11-15T09:05:57Z

automation/e2e-deploy-resources.sh

@@ -40,7 +40,7 @@ kubectl apply -f "https://github.com/kubevirt/kubevirt/releases/download/${KUBEV

 kubectl apply -f "https://github.com/kubevirt/kubevirt/releases/download/${KUBEVIRT_VERSION}/kubevirt-cr.yaml"

-kubectl patch kubevirt kubevirt -n kubevirt --type merge -p '{"spec":{"configuration":{"developerConfiguration":{"featureGates": ["DataVolumes"]}}}}'
+kubectl patch kubevirt kubevirt -n kubevirt --type merge -p '{"spec":{"configuration":{"developerConfiguration":{"featureGates": ["DataVolumes", "VMExport"]}}}}'


DataVolumes FG can be dropped I guess.

Yep just want to get confirmation from @ksimon1 about it, so this PR won't do any other changes except adding new tests.

0xFelix · 2024-11-15T09:06:34Z

automation/e2e-source.sh

@@ -3,3 +3,6 @@
 export DEV_MODE="${DEV_MODE:-false}"
 export STORAGE_CLASS="${STORAGE_CLASS:-}"
 export NUM_NODES=${NUM_NODES:-2}
+export REGISTRY_IMAGE_DESTINATION="quay.io/kubevirt/e2e-tests-example-vm"


Is there absolutely no way to use an ephemeral registry? Why do we have to store test artifacts on a live registry? (quay.io)

I replied here earlier. The issue is that there is no built-in registry in Kubernetes cluster (only on OpenShift).

So we have 3 options:

1). Deploy a new pod that will use registry image to push there
2). Use OCP's built-in container registry
3). Use an external container registry

@0xFelix @ksimon1 What do you prefer?

0xFelix · 2024-11-15T09:07:54Z

test/disk_uploader_test.go

+		imageDestination := os.Getenv("REGISTRY_IMAGE_DESTINATION")
+		Expect(imageDestination).ToNot(BeEmpty())
+
+		registryCredentials, err := createRegistryCredentials(f.CoreV1Client, f.DeployNamespace)
+		Expect(err).ToNot(HaveOccurred())
+
+		cirrosVm, err := createCirrosVM(f.KubevirtClient, f.DeployNamespace)
+		Expect(err).ToNot(HaveOccurred())
+
+		_, err = vm.WaitForVM(f.KubevirtClient, f.DeployNamespace, cirrosVm.Name, "", constants.Timeouts.WaitForVMStart.Duration, false)
+		Expect(err).ToNot(HaveOccurred())


Move this to BeforeEach?

0xFelix · 2024-11-15T09:08:12Z

test/disk_uploader_test.go

+			CreateTaskRun().
+			ExpectSuccess().
+			ExpectLogs(config.GetAllExpectedLogs()...).
+			ExpectResults(nil)


Are you going to add a check?

0xFelix · 2024-11-15T09:09:28Z

test/disk_uploader_test.go

+
+func createCirrosVM(client kubevirtcliv1.KubevirtClient, namespace string) (*v1.VirtualMachine, error) {
+	dataVolume := libdv.NewDataVolume(
+		libdv.WithRegistryURLSource("docker://quay.io/kubevirt/cirros-container-disk-demo"),


Please do not use libdv. It lives under tests/libdv which might be subject to change. Only libvmi can be considered stable.

@0xFelix Don't you think that this is an issue that such important libs for external projects tests living under kubevirt/kubevirt, especially under tests/libdv?

codingben · 2024-11-15T10:14:50Z

@ksimon1 @0xFelix Currently there is a single test case: Extracts disk from VM and upload to container registry. Do you think we should also test scenarios of extracting it from VMSnapshot or PVC, or it's enough to have only VM? I don't want to end up re-testing and over-testing Export API.

@ksimon1 @0xFelix What is your opinion about it?

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Nov 12, 2024

kubevirt-bot requested a review from 0xFelix November 12, 2024 14:39

kubevirt-bot requested a review from geetikakay November 12, 2024 14:39

openshift-ci bot requested a review from lyarwood November 12, 2024 14:39

kubevirt-bot added the size/L label Nov 12, 2024

codingben marked this pull request as draft November 12, 2024 14:39

kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 12, 2024

openshift-ci bot requested review from akrejcir and ksimon1 November 12, 2024 14:39

0xFelix reviewed Nov 12, 2024

View reviewed changes

codingben force-pushed the CNV-43603 branch from eb6a2ad to e24fe1a Compare November 13, 2024 14:00

kubevirt-bot added size/XXL and removed size/L labels Nov 13, 2024

codingben requested a review from 0xFelix November 13, 2024 14:06

0xFelix reviewed Nov 14, 2024

View reviewed changes

test/disk_uploader_test.go Outdated Show resolved Hide resolved

test(CNV-43603): add disk-uploader functional tests

ef903df

Deploy a new Cirros VM to export from, and run disk-uploader as Task to execute extraction process and upload it to the Quay container registry. Signed-off-by: Ben Oukhanov <[email protected]>

codingben force-pushed the CNV-43603 branch from e24fe1a to ef903df Compare November 14, 2024 13:15

codingben requested a review from 0xFelix November 14, 2024 13:17

0xFelix reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(CNV-43603): add disk-uploader functional tests #549

test(CNV-43603): add disk-uploader functional tests #549

codingben commented Nov 12, 2024

kubevirt-bot commented Nov 12, 2024

openshift-ci bot commented Nov 12, 2024

codingben commented Nov 12, 2024

codingben commented Nov 12, 2024

0xFelix Nov 12, 2024

codingben Nov 13, 2024

0xFelix Nov 12, 2024

ksimon1 Nov 13, 2024

codingben Nov 13, 2024

codingben Nov 13, 2024

0xFelix Nov 12, 2024

codingben Nov 13, 2024

0xFelix Nov 15, 2024

codingben Nov 15, 2024

codingben commented Nov 13, 2024 •

edited

Loading

codingben commented Nov 13, 2024 •

edited

Loading

0xFelix Nov 14, 2024

codingben Nov 14, 2024

codingben Nov 14, 2024 •

edited

Loading

codingben commented Nov 14, 2024

0xFelix Nov 15, 2024

codingben Nov 15, 2024

0xFelix Nov 15, 2024

codingben Nov 15, 2024

0xFelix Nov 15, 2024

0xFelix Nov 15, 2024

0xFelix Nov 15, 2024

codingben Nov 15, 2024

codingben commented Nov 15, 2024

		"accessKeyId": []byte(""), // TODO: Get value from OCP CI
		"secretKey": []byte(""), // TODO: Get value from OCP CI

test(CNV-43603): add disk-uploader functional tests #549

Are you sure you want to change the base?

test(CNV-43603): add disk-uploader functional tests #549

Conversation

codingben commented Nov 12, 2024

kubevirt-bot commented Nov 12, 2024

openshift-ci bot commented Nov 12, 2024

codingben commented Nov 12, 2024

codingben commented Nov 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben commented Nov 13, 2024 • edited Loading

codingben commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

codingben commented Nov 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codingben commented Nov 15, 2024

codingben commented Nov 13, 2024 •

edited

Loading

codingben commented Nov 13, 2024 •

edited

Loading

codingben Nov 14, 2024 •

edited

Loading