[release-1.2] VMI controller - move unit tests to fake client (VirtualMachineInstanceInterface) #11863
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit 54034ae)
Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit 88d0e48) Signed-off-by: fossedihelm <ffossemo@redhat.com>
Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit 669d5f9)
Add the virtclientset to be used when vmi client is interrogated. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit e3e8373)
Register resources to the fakeclient during their addition to the informers. In this way wa can interact directly with the fakeclient that will store the changes that will be applied to the resources. Created `addPod`, `addDataVolumePVC`, `addDataVolume` convenience functions. These will be used in subsequent commits. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit c3022aa)
Created `addPod`, `addDataVolumePVC`, `addDataVolume` convenience functions allow us to register the resoruces to the fakeclient. Let's use them instead of specific informer additions. This allows us to remove some fake prepend reactor, since the resources are correctly registered (or not), in the client. Also, the podFeeder and dataVolumeFeeder have been removed since they art not used anymore. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit 383c1bc)
The mockinterfaces need to be instructed to what expect before the controller execution, while using the fakeclient allow us to check resources after the controller execution. Convert the `shouldExpect` functions, which prepare the mockinterface to `expect` ones, which check the fakeclient. In particular: - shouldExpectMatchingPodCreation -> expectMatchingPodCreation - shouldExpectPodCreation -> expectPodExists - shouldExpectPodDeletion -> expectPodDoesNotExist - dropped shouldExpectMultiplePodDeletions - shouldExpectHotplugPod -> expectHotplugPod N.B. shouldExpectPodDeletion was also moved under a specific `DescribeTable` which requires it. This will be address in the future. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit 8b4e43b)
The mockinterfaces need to be instructed to what expect before the controller execution, while using the fakeclient allow us to check resources after the controller execution. Convert the `shouldExpect` functions, which prepare the mockinterface to `expect` ones, which check the fakeclient. In particular: - shouldExpectVirtualMachineSchedulingState -> expectVMISchedulingState - shouldExpectVirtualMachineScheduledState -> expectVMIScheduledState - shouldExpectVirtualMachineFailedState -> expectVMIFailedState - shouldExpectVirtualMachineDataVolumesReadyCondition -> expectVMIDataVolumeReadyCondition - added expectVMIWithMatcherConditions generic function - added expectVMIBeInPhase generic function - added expectVMITimestamp generic function - dropped shouldExpectVirtualMachineDataVolumesReadyConditionWithMessage since it was used only once and replaced using generic expectVMIWithMatcherConditions Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit 179b6da)
There are two tests that want to check the existence(or not) of specific network annotations on the pod. Currently, those tests are not checking correctly the annotations, since they are looking at the `pod` variable that is defined under the test scope itself. What we want to check is that the `updatedPod` (IOW: in the fakeclient) has the correct annotations. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit e573666) Signed-off-by: fossedihelm <ffossemo@redhat.com>
`NewPendingVirtualMachine` function returns a vmi in
pending status. This vmi is used in the tests and it
is registered in the fakeclient and the informers.
Both activePods and volumeStatus are defined as `json:<name,omitempty>`.
This means that the in real clusters it is not possible that
there will be a Pod with empty activePods (because the marshall will
convert it to nil). However, currently, we are storing a vmi with
empty (non-nil) activepods in the vmi informer.
Since the pod in the vmi informer is used by the vmi controller
to build the patch operations, the empty map causes a failure in the
`test` patch operation in the test `should add active pods to status if VMI is in running state`.
In other words, the resource stored in the vmi fakeclient is different to
what we put in the vmi informer because in the fakeclient there is
a nil value, while in the vmi informer an empty map.
The empty map will result in this patch operation:
`{ "op": "test", "path": "/status/activePods", "value": {} }`
that fails because it should be:
`{ "op": "test", "path": "/status/activePods", "value": null }`
This is a clear example of how the fakeclient help us to create
tests closer to reality, which results in an increasing of
stability.
Even if no failures was observed around the VolumeStatus,
the same logic can be applied, so here we are going to
align its behavior too.
Signed-off-by: fossedihelm <ffossemo@redhat.com>
(cherry picked from commit dabac9f)
With `Actions()` fakeclient function we can check how many actions has been performed. This is useful when we want to check that nothing is happened. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit a912feb)
There are two tests that checks the vmi phase and volumestatus. Regarding the phase, currently, those tests are not performing the right check, since they are looking at the `vmi` variable that is defined under the test scope itself. What we want to check is that the `updatedVmi` (IOW: in the fakeclient) has the correct phase. Regarding the volumestatus, we can switch checking the updated vmi from the fakeclient instead of expexting the right patch operation. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit e66a79e)
It is not used anymore. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit ad6e016)
Both invtsc and hyperv tests expect a pod creation when topoloty hints is going to be set, but actually this pod creation does not occur (it can occur in the subsequent reconcile cycle). Remove also `runController` function since it does not add anything. Signed-off-by: fossedihelm <ffossemo@redhat.com> (cherry picked from commit 6645dcd)
kubevirt-bot
added
release-note-none
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acardace 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:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
approved
|
/retest-required |
jean-edouard
approved these changes
May 15, 2024
|
@fossedihelm: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does
This is a manual cherry-pick of #11751
Release note