Adding UT coverage for VM validation in VRG #2052
Conversation
bd07b83 to
7942c6a
Compare
There was a problem hiding this comment.
Why do we need to keep the kubvirt crd in ramen source? This is 5835 lines file. We can install this from kubevirt release for the tests.
There was a problem hiding this comment.
I tried out other options:
-
Downloading VirtualMachine crd yaml raw file in runtime from the testEnv: kubevirt doesn't provide CRDs. I tried some URLs to download the CRD, but they are not reliable.
-
Applying kubevirt-operator and kubevirt-cr, expecting it to install VirtualMachine CRD, eventually. But the kubevirt-operator.yaml defines deployments that won’t run in envtest, because there's no controller manager or scheduler. The operator won't function, so it never installs the CRDs like VirtualMachine.
-
Instead of downloading CRDs from unreliable URLs, we can generate the necessary CRDs locally during the test setup. But we will need to clone the kubevirt repo, and generate manifests there.. but that also doesn't look like a better solution. right?
Any comments on the approaches I tried? or any other suggestions?
| vrgTestCase.cleanupVRG() | ||
| }) | ||
|
|
||
| It("VM do not have the protection label, but is in protected vms list of secondary vrg", func() { |
There was a problem hiding this comment.
Why do we need to test VM? can we use deployment for testing the same thing? We should not have vm specific code in ramen, so we don't need vm specific tests.
There was a problem hiding this comment.
Hi @nirs, these tests are added specifically to test VM-recipe. The virtualmachine instances gets validated by the VRG controller, in-order to simulate the scenarios we need to create VM instances in testenv.
b1f7564 to
39c2d56
Compare
There was a problem hiding this comment.
The minimized version looks nice. Please add a comment explaining that this is minimized version of the original crd, and add a link to source you took it from.
| @@ -35,7 +35,7 @@ require ( | |||
| k8s.io/component-base v0.31.1 | |||
| k8s.io/kube-openapi v0.0.0-20241105132330-32ad38e42d3f | |||
| k8s.io/utils v0.0.0-20241104100929-3ea5e8cea738 | |||
| kubevirt.io/api v1.5.0 | |||
| kubevirt.io/api v1.5.1 | |||
There was a problem hiding this comment.
Upgrading to 1.5.1 is not related to the test change, this should be done separately, and use latest version:
https://github.com/kubevirt/kubevirt/releases/tag/v1.5.2
Note that in drenv we are still testing kubevirt 1.3. We need to upgrade it to 1.5.2 as well.
| @@ -1072,6 +1076,426 @@ var _ = Describe("VolumeReplicationGroupVolRepController", func() { | |||
| }) | |||
| }) | |||
|
|
|||
| var vrgTestCase *vrgTest | |||
| var vrgTestCase1 *vrgTest | |||
There was a problem hiding this comment.
These names do not describe the purpose of the variable. It would be better to define them at the context scope so we use the same short variable name for the entire context.
Looking at the new code vrgTestCase1 is not used in this context.
| Expect(condition.Message).To(Equal("VolumeReplicationGroup is not in the admin namespace: " + | ||
| "VolumeReplicationGroup is not allowed to protect namespaces")) | ||
|
|
||
| vrgTestCase.cleanupVRG() |
There was a problem hiding this comment.
Is this called if an assertion failed before the test?
for example
39c2d56#diff-af799cf0d298b89e62ed4257879ffdf53d91e60f4f4871f01bc3811c61366209R1113
Cleanup code must be called from cleanup handler so it is always called, even on failures.
| }, vrgtimeout, vrginterval).Should(BeTrue(), "while waiting for VRG TRUE condition on secondary %s", | ||
| vrgTestCase.vrgName) | ||
|
|
||
| deleteVM(vm, vmNamespacedName) |
There was a problem hiding this comment.
Other tests do not delete the vm - should they?
| Expect(condition.Status).To(Equal(metav1.ConditionFalse)) | ||
| Expect(condition.Reason).To(Equal("ClusterDataConflictSecondary")) | ||
|
|
||
| deleteVM(vm1, vmNamespacedName1) |
There was a problem hiding this comment.
This will not be called if the assetions before fail, but we don't call this later in a cleanup.
de5c64f to
f19469d
Compare
Signed-off-by: Abhijeet Shakya <abhijeetshakya21@gmail.com>
UT coverage for VM-DR epic:
Writing unit tests and validations of VRG status for different combination of VMs(with and without protection label) and their states.