Fix smoke tests due to change in behavior of restore VM #10583
Conversation
Pearl1594
requested review from
rohityadavcloud,
shwstppr,
DaanHoogland,
vishesh92 and
weizhouapache
boring-cyborg
bot
added
component:integration-test
Python
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 4.19 #10583 +/- ##
===========================================
Coverage 15.16% 15.16%
- Complexity 11327 11349 +22
===========================================
Files 5414 5415 +1
Lines 474814 476042 +1228
Branches 57912 58391 +479
===========================================
+ Hits 72004 72197 +193
- Misses 394758 395783 +1025
- Partials 8052 8062 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I wonder if this change in behaviour requires documentation. |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12822 |
|
@blueorangutan test |
|
@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-12760)
|
|
@blueorangutan test matrix |
|
@Pearl1594 a [SL] Trillian-Jenkins matrix job (EL8 mgmt + EL8 KVM, Ubuntu22 mgmt + Ubuntu22 KVM, EL8 mgmt + VMware 7.0u3, EL9 mgmt + XCP-ng 8.2 ) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-12776)
|
|
[SF] Trillian test result (tid-12775)
|
|
[SF] Trillian test result (tid-12774)
|
|
[SF] Trillian test result (tid-12777)
|
weizhouapache
changed the title
| account_network.update(self.apiclient, name=account_network.name + ts) | ||
| account_network.delete(self.apiclient) | ||
| self.cleanup.remove(account_network) | ||
| virtual_machine.start(self.apiclient) |
There was a problem hiding this comment.
@Pearl1594 I'm not completely aware of the new behaviour but idea of the test was to do a bunch of operations related to VM, network, volume etc and then check if the events for them have resourceid an resourcetype associated for them. Can we update the test in a way that similar check can be done? With the current change it is checking only for VM action event.
There was a problem hiding this comment.
I see your point and the point of such tests, but we are not checking for specific events anyway, just listing them. If the test is error prone, not specific and sensitive to environmental issues, I'd rather we simplify it and add more specific checks. Would you agree?
There was a problem hiding this comment.
@shwstppr I removed the delete resource operations because it did not matter in terms of the events being generated. As the test doesn't check the type of event being generated for a resource, but rather just checking if events for a resource are generated. And the cleanup is happening via the teardown process.
Regarding the start operation, it was noticed that if a VM is restored - it loses its root disk, hence the test was failing on vmware during the deletion of the vm.
There was a problem hiding this comment.
@Pearl1594 Your change is correct in itself but I feel now we are verifying lesser set of actions. I would prefer if we can have only the change that fixes restore VM.
Like if earlier we were verifying 20 events, now we might only be doing 5 events.
There was a problem hiding this comment.
I've reverted the change, to keep only the relevant change to what this PR is attempting to fix. Thanks @shwstppr
| account_network.update(self.apiclient, name=account_network.name + ts) | ||
| account_network.delete(self.apiclient) | ||
| self.cleanup.remove(account_network) | ||
| virtual_machine.start(self.apiclient) |
There was a problem hiding this comment.
I see your point and the point of such tests, but we are not checking for specific events anyway, just listing them. If the test is error prone, not specific and sensitive to environmental issues, I'd rather we simplify it and add more specific checks. Would you agree?
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
@blueorangutan package |
|
@Pearl1594 a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12983 |
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12984 |
|
@blueorangutan test |
|
@Pearl1594 a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-12909)
|
Description
This PR fixes a regression introduced due to change in behavior with restore VM operation. Since restoreVM results in root volume deletion, if the VM is cleaned up without starting no Root disk is found.
Caused by: #8800 (#8800 (comment))
Test failures also noticed on 4.19.2 health check: #9315 (comment)
Fixes failure in:
test_events_resource.py
test_network_permissions.py
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?