Prepare CDI testing for the upcoming non-CSI lane #2730

alromeros · 2023-05-26T18:09:21Z

What this PR does / why we need it:

With the recent inclusion of populators in CDI, we plan to include a new testing lane without CSI support, so we are able to test the non-populator flow. Since all our current lanes have default storage classes with CSI drivers, we need to tweak some of our already existing tests so they work with the new lane.

This PR updates our functional tests so we specify valid storage classes in tests that use the default one or skip invalid cases. It also introduces some changes in the cluster-up scripts to allow having a non-CSI default storage class.

Special notes for your reviewer: This will take some time since I'm running the tests locally with local storage class to detect failures.

Release note:

Update CDI to allow the use of a non-CSI testing lane

kubevirt-bot · 2023-05-26T18:09:26Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

alromeros · 2023-05-26T18:09:45Z

/test all

mhenriks · 2023-05-31T13:40:42Z

/test pull-containerized-data-importer-e2e-hpp-latest

alromeros · 2023-05-31T15:37:08Z

/test pull-containerized-data-importer-e2e-hpp-latest
/test pull-containerized-data-importer-e2e-hpp-previous

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2023-06-01T09:30:16Z

/test pull-containerized-data-importer-e2e-hpp-latest
/test pull-containerized-data-importer-e2e-hpp-previous

mhenriks · 2023-06-01T23:18:59Z

/test pull-containerized-data-importer-non-csi-hpp

mhenriks · 2023-06-01T23:22:22Z

@alromeros looks like non-csi-hpp.sh has to be executable

ls -l automation
total 60
-rwxr-xr-x. 1 mhenriks mhenriks  686 Apr 16 20:15 ceph-gc.sh
-rwxr-xr-x. 1 mhenriks mhenriks  825 May 23 16:40 ceph.sh
-rwxr-xr-x. 1 mhenriks mhenriks  847 May 23 16:40 ceph-wffc.sh
-rwxr-xr-x. 1 mhenriks mhenriks  798 May 23 16:40 destructive.sh
-rwxr-xr-x. 1 mhenriks mhenriks   37 Mar 19  2021 git-askpass.sh
-rwxr-xr-x. 1 mhenriks mhenriks  792 May 23 16:40 istio.sh
-rwxr-xr-x. 1 mhenriks mhenriks  758 May 23 16:40 latest-hpp.sh
-rwxr-xr-x. 1 mhenriks mhenriks  794 May 23 16:40 nfs.sh
-rw-r--r--. 1 mhenriks mhenriks  782 Jun  1 19:21 non-csi-hpp.sh
-rwxr-xr-x. 1 mhenriks mhenriks  276 Feb 14 14:53 postsubmit-main.sh
-rwxr-xr-x. 1 mhenriks mhenriks  789 May 23 16:40 previous-hpp.sh
-rwxrwxr-x. 1 mhenriks mhenriks  524 Aug  9  2021 prow_periodic_push.sh
-rwxr-xr-x. 1 mhenriks mhenriks 2175 Feb 14 14:53 release.sh
-rwxr-xr-x. 1 mhenriks mhenriks 3891 May 23 16:40 test.sh
-rwxr-xr-x. 1 mhenriks mhenriks  784 May 23 16:40 upgrade.sh

awels · 2023-06-02T12:27:34Z

tests/datavolume_test.go

+			// your changes to the latest version and try again" error in "test_id:5912" when using a
+			// default storage class without accessMode.
+			// The real cause should bee somewhere else but this fixes it for the moment.
+			storageProfile, err = f.CdiClient.CdiV1beta1().StorageProfiles().Get(context.TODO(), name, metav1.GetOptions{})


So I am assuming the update is what is causing the object is modified error. This is because something is modifying the object in between the get and update. The proper way to solve this is to use an Eventually, something like this:

Eventually(func() error storageProfile, err = f.CdiClient.CdiV1beta1().StorageProfiles().Get(context.TODO(), name, metav1.GetOptions{}) Expect(err).ToNot(HaveOccurred()) storageProfile.Spec = spec return client.Update(context.TODO(), storageProfile) }, 15*time.Second, time.Second).Should(BeNil())

The get still can't fail (if it does, that is really bad) but the update can fail due to the modified error, we simply retry.

awels · 2023-06-02T12:34:23Z

tests/datavolume_test.go

@@ -2444,6 +2454,10 @@ var _ = Describe("[vendor:cnv-qe@redhat.com][level:component]DataVolume tests",
 		}

 		table.DescribeTable("import DV with AccessModes, PVC is pending until", func(scName string, scFunc func(string), dvFunc func(string) *cdiv1.DataVolume) {
+			if utils.IsDefaultSCNoProvisioner() {
+				Skip("Default storage class is no provisioner. The new storage class won't work")


I would say Default storage class has no provisioner. The new storage class won't work

Yeah that sounds better, I'll change it

mhenriks · 2023-06-02T13:12:28Z

/test pull-containerized-data-importer-non-csi-hpp

This commit modifies several scripts to allow the usage of classic HPP as the default SC in tests. This allows us to test our non-populator flow with a non-csi provisioner. Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros · 2023-06-02T14:41:20Z

/test pull-containerized-data-importer-non-csi-hpp

mhenriks · 2023-06-02T23:23:29Z

/retest-required

mhenriks · 2023-06-02T23:23:37Z

/test pull-containerized-data-importer-non-csi-hpp

alromeros · 2023-06-03T07:12:14Z

/test pull-containerized-data-importer-non-csi-hpp
/test pull-containerized-data-importer-e2e-upg

mhenriks · 2023-06-03T20:22:16Z

/test pull-containerized-data-importer-non-csi-hpp
/test pull-containerized-data-importer-e2e-upg

alromeros · 2023-06-04T02:33:54Z

/test pull-containerized-data-importer-non-csi-hpp

ShellyKa13 · 2023-06-04T13:30:03Z

/lgtm

mhenriks · 2023-06-05T14:36:39Z

/approve

kubevirt-bot · 2023-06-05T14:36:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mhenriks]

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

alromeros · 2023-06-05T18:19:55Z

/test pull-containerized-data-importer-e2e-istio

alromeros · 2023-06-05T21:24:52Z

/test pull-containerized-data-importer-e2e-ceph

* Update functional tests to skip incompatible default storage classes Signed-off-by: Alvaro Romero <alromero@redhat.com> * Enable the use of non-csi HPP in testing lanes This commit modifies several scripts to allow the usage of classic HPP as the default SC in tests. This allows us to test our non-populator flow with a non-csi provisioner. Signed-off-by: Alvaro Romero <alromero@redhat.com> --------- Signed-off-by: Alvaro Romero <alromero@redhat.com>

* Enable empty schedule in DataImportCron (#2711) Allow disabling DataImportCron schedule and support external trigger Signed-off-by: Ido Aharon <iaharon@redhat.com> * expand upon #2721 (#2731) Need to replace requeue bool with requeue duration Signed-off-by: Michael Henriksen <mhenriks@redhat.com> * Add clone from snapshot functionalities to clone-populator (#2724) * Add clone from snapshot functionalities to the clone populator Signed-off-by: Alvaro Romero <alromero@redhat.com> * Update clone populator unit tests to cover clone from snapshot capabilities Signed-off-by: Alvaro Romero <alromero@redhat.com> * Fix storage class assignation in temp-source claim for host-assisted clone from snapshot This commit also includes other minor and styling-related fixes Signed-off-by: Alvaro Romero <alromero@redhat.com> --------- Signed-off-by: Alvaro Romero <alromero@redhat.com> * Prepare CDI testing for the upcoming non-CSI lane (#2730) * Update functional tests to skip incompatible default storage classes Signed-off-by: Alvaro Romero <alromero@redhat.com> * Enable the use of non-csi HPP in testing lanes This commit modifies several scripts to allow the usage of classic HPP as the default SC in tests. This allows us to test our non-populator flow with a non-csi provisioner. Signed-off-by: Alvaro Romero <alromero@redhat.com> --------- Signed-off-by: Alvaro Romero <alromero@redhat.com> * Allow snapshots as format for DataImportCron created sources (#2700) * StorageProfile API for declaring format of resulting cron disk images Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Integrate recommended format in dataimportcron controller Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Take snapclass existence into consideration when populating cloneStrategy and sourceFormat Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> --------- Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> * Remove leader election test (#2745) Now that we are using the standard k8s leases from the controller runtime library, there is no need to test our implementation as it is no longer in use. This will save some testing time and random failures. Signed-off-by: Alexander Wels <awels@redhat.com> * Integration of Data volume using CDI populators (#2722) * move cleanup out of dv deletion It seemed off to call cleanup in the prepare function just because we don't call cleanup unless the dv is deleting. Instead we check in the clenup function itself if it should be done: in this 2 specific cases in case of deletion and in case the dv succeeded. The cleanup will be used in future commit also for population cleanup which we also want to happen not only on deletion. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Use populator if csi storage class exists Add new datavolume phase PendingPopulation to indicate wffc when using populators, this new phase will be used in kubevirt in order to know that there is no need for dummy pod to pass wffc phase and that the population will occur once creating the vm. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Update population targetPVC with pvc prime annotations The annotations will be used to update dv that uses the populators. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Adjust UT with new behavior Signed-off-by: Shelly Kagan <skagan@redhat.com> * updates after review Signed-off-by: Shelly Kagan <skagan@redhat.com> * Fix import populator report progress The import pod should be taken from pvcprime Signed-off-by: Shelly Kagan <skagan@redhat.com> * Prevent requeue upload dv when failing to find progress report pod Signed-off-by: Shelly Kagan <skagan@redhat.com> * Remove size inflation in populators The populators are handling existing PVCs. The PVC already has a defined requested size, inflating the PVC' with fsoverhead will only be on the PVC' spec and will not reflect on the target PVC, this seems undesired. Instead if the populators is using by PVC that the datavolume controller created the inflation will happen there if needed. Signed-off-by: Shelly Kagan <skagan@redhat.com> * Adjust functional tests to handle dvs using populators Signed-off-by: Shelly Kagan <skagan@redhat.com> * Fix clone test Signed-off-by: Shelly Kagan <skagan@redhat.com> * add shouldUpdateProgress variable to know if need to update progress Signed-off-by: Shelly Kagan <skagan@redhat.com> * Change update of annotation from denied list to allowed list Instead if checking if the annotation on pvcPrime is not desired go over desired list and if the annotation exists add it. Signed-off-by: Shelly Kagan <skagan@redhat.com> * fix removing annotations from pv when rebinding Signed-off-by: Shelly Kagan <skagan@redhat.com> * More fixes and UT Signed-off-by: Shelly Kagan <skagan@redhat.com> * a bit more updates and UTs Signed-off-by: Shelly Kagan <skagan@redhat.com> --------- Signed-off-by: Shelly Kagan <skagan@redhat.com> * Run bazelisk run //robots/cmd/uploader:uploader -- -workspace /home/prow/go/src/github.com/kubevirt/project-infra/../containerized-data-importer/WORKSPACE -dry-run=false (#2751) Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> * Allow dynamic linked build for non bazel build (#2753) The current script always passes the static ldflag to the compiler which will result in a static binary. We would like to be able to build dynamic libraries instead. cdi-containerimage-server has to be static because we are copying it into the context of a container disk container which is most likely based on a scratch container and has no libraries for us to use. Signed-off-by: Alexander Wels <awels@redhat.com> * Disable DV GC by default (#2754) * Disable DV GC by default DataVolume garbage collection is a nice feature, but unfortunately it violates fundamental principle of Kubernetes. CR should not be auto-deleted when it completes its role (Job with TTLSecondsAfter- Finished is an exception), and once CR was created we can assume it is there until explicitly deleted. In addition, CR should keep idempotency, so the same CR manifest can be applied multiple times, as long as it is a valid update (e.g. DataVolume validation webhook does not allow updating the spec). When GC is enabled, some systems (e.g GitOps / ArgoCD) may require a workaround (DV annotation deleteAfterCompletion = "false") to prevent GC and function correctly. On the next kubevirt-bot Bump kubevirtci PR (with bump-cdi), it will fail on all kubevirtci lanes with tests referring DVs, as the tests IsDataVolumeGC() looks at CDIConfig Spec.DataVolumeTTLSeconds and assumes default is enabled. This should be fixed there. Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Fix test waiting for PVC deletion with UID Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Fix clone test assuming DV was GCed Signed-off-by: Arnon Gilboa <agilboa@redhat.com> * Fix DIC controller DV/PVC deletion when snapshot is ready Signed-off-by: Arnon Gilboa <agilboa@redhat.com> --------- Signed-off-by: Arnon Gilboa <agilboa@redhat.com> --------- Signed-off-by: Ido Aharon <iaharon@redhat.com> Signed-off-by: Michael Henriksen <mhenriks@redhat.com> Signed-off-by: Alvaro Romero <alromero@redhat.com> Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com> Signed-off-by: Alexander Wels <awels@redhat.com> Signed-off-by: Shelly Kagan <skagan@redhat.com> Signed-off-by: kubevirt-bot <kubevirtbot@redhat.com> Signed-off-by: Arnon Gilboa <agilboa@redhat.com> Co-authored-by: Ido Aharon <iaharon@redhat.com> Co-authored-by: Michael Henriksen <mhenriks@redhat.com> Co-authored-by: alromeros <alromero@redhat.com> Co-authored-by: akalenyu <akalenyu@redhat.com> Co-authored-by: Shelly Kagan <skagan@redhat.com> Co-authored-by: kubevirt-bot <kubevirtbot@redhat.com> Co-authored-by: Arnon Gilboa <agilboa@redhat.com>

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note-none Denotes a PR that doesn't merit a release note. labels May 26, 2023

kubevirt-bot requested review from awels and mhenriks May 26, 2023 18:09

kubevirt-bot added the size/M label May 26, 2023

alromeros changed the title ~~[WIP] Fix functional tests to work with incompatible default storage classes~~ [WIP] Update functional tests to avoid failures with incompatible default storage classes May 26, 2023

alromeros force-pushed the fix-local-sc-tests branch 6 times, most recently from f8b6fcd to e5fc72b Compare May 31, 2023 12:57

alromeros force-pushed the fix-local-sc-tests branch from e5fc72b to 9eba18e Compare May 31, 2023 13:43

Update functional tests to skip incompatible default storage classes

e7aa9c0

Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros force-pushed the fix-local-sc-tests branch from e6ebe87 to df2b6d7 Compare June 1, 2023 09:29

alromeros changed the title ~~[WIP] Update functional tests to avoid failures with incompatible default storage classes~~ [WIP] Prepare CDI testing for the upcoming non-CSI lane Jun 1, 2023

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 1, 2023

alromeros force-pushed the fix-local-sc-tests branch from df2b6d7 to cb1c64f Compare June 1, 2023 12:40

kubevirt-bot added size/L and removed size/M labels Jun 1, 2023

alromeros changed the title ~~[WIP] Prepare CDI testing for the upcoming non-CSI lane~~ Prepare CDI testing for the upcoming non-CSI lane Jun 1, 2023

alromeros marked this pull request as ready for review June 1, 2023 12:41

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2023

kubevirt-bot requested a review from aglitke June 1, 2023 12:41

alromeros force-pushed the fix-local-sc-tests branch 2 times, most recently from 8281407 to c371cde Compare June 1, 2023 17:51

alromeros force-pushed the fix-local-sc-tests branch 2 times, most recently from 9f68e28 to ee08525 Compare June 2, 2023 11:24

awels reviewed Jun 2, 2023

View reviewed changes

Enable the use of non-csi HPP in testing lanes

bd9e52c

This commit modifies several scripts to allow the usage of classic HPP as the default SC in tests. This allows us to test our non-populator flow with a non-csi provisioner. Signed-off-by: Alvaro Romero <alromero@redhat.com>

alromeros force-pushed the fix-local-sc-tests branch from ee08525 to bd9e52c Compare June 2, 2023 13:15

kubevirt-bot assigned ShellyKa13 Jun 4, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2023

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2023

kubevirt-bot merged commit fab858e into kubevirt:main Jun 6, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare CDI testing for the upcoming non-CSI lane #2730

Prepare CDI testing for the upcoming non-CSI lane #2730

alromeros commented May 26, 2023 •

edited

kubevirt-bot commented May 26, 2023

alromeros commented May 26, 2023

mhenriks commented May 31, 2023

alromeros commented May 31, 2023

alromeros commented Jun 1, 2023

mhenriks commented Jun 1, 2023

mhenriks commented Jun 1, 2023

awels Jun 2, 2023

awels Jun 2, 2023

alromeros Jun 2, 2023

mhenriks commented Jun 2, 2023

alromeros commented Jun 2, 2023

mhenriks commented Jun 2, 2023

mhenriks commented Jun 2, 2023

alromeros commented Jun 3, 2023

mhenriks commented Jun 3, 2023

alromeros commented Jun 4, 2023

ShellyKa13 commented Jun 4, 2023

mhenriks commented Jun 5, 2023

kubevirt-bot commented Jun 5, 2023

alromeros commented Jun 5, 2023

alromeros commented Jun 5, 2023

Prepare CDI testing for the upcoming non-CSI lane #2730

Prepare CDI testing for the upcoming non-CSI lane #2730

Conversation

alromeros commented May 26, 2023 • edited

kubevirt-bot commented May 26, 2023

alromeros commented May 26, 2023

mhenriks commented May 31, 2023

alromeros commented May 31, 2023

alromeros commented Jun 1, 2023

mhenriks commented Jun 1, 2023

mhenriks commented Jun 1, 2023

awels Jun 2, 2023

Choose a reason for hiding this comment

awels Jun 2, 2023

Choose a reason for hiding this comment

alromeros Jun 2, 2023

Choose a reason for hiding this comment

mhenriks commented Jun 2, 2023

alromeros commented Jun 2, 2023

mhenriks commented Jun 2, 2023

mhenriks commented Jun 2, 2023

alromeros commented Jun 3, 2023

mhenriks commented Jun 3, 2023

alromeros commented Jun 4, 2023

ShellyKa13 commented Jun 4, 2023

mhenriks commented Jun 5, 2023

kubevirt-bot commented Jun 5, 2023

alromeros commented Jun 5, 2023

alromeros commented Jun 5, 2023

alromeros commented May 26, 2023 •

edited