api,server: fix VM.CREATE events on vm deploy without start#7421
api,server: fix VM.CREATE events on vm deploy without start#7421yadvr merged 2 commits intoapache:4.18from
Conversation
Fixes apache#6697 Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Codecov Report
@@ Coverage Diff @@
## 4.18 #7421 +/- ##
============================================
- Coverage 12.69% 12.69% -0.01%
+ Complexity 8661 8656 -5
============================================
Files 2717 2717
Lines 256165 256163 -2
Branches 39928 39927 -1
============================================
- Hits 32526 32512 -14
- Misses 219504 219517 +13
+ Partials 4135 4134 -1
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Kudos, SonarCloud Quality Gate passed! |
|
@blueorangutan package |
|
@shwstppr a 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: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 5895 |
|
|
||
| @Override | ||
| @ActionEvent(eventType = EventTypes.EVENT_VM_CREATE, eventDescription = "starting Vm", async = true) | ||
| @ActionEvent(eventType = EventTypes.EVENT_VM_CREATE, eventDescription = "deploying Vm", async = true) |
There was a problem hiding this comment.
@shwstppr , it seems to me this should not yield an event for EVENT_VM_CREATE but for EVENT_VM_START. The EVENT_VM_CREATEshould already have been yielded on_userVmService.createVirtualMachine(this)`.
There was a problem hiding this comment.
@DaanHoogland As of now, when deploying a new VM, CloudStack doesn't publishes an EVENT_VM_START event and EVENT_VM_CREATE is not published during _userVmService.createVirtualMachine(this).
I feel this is apt behaviour because _userVmService.createVirtualMachine(this) will just create a DB record and actual deployment could fail in many scenarios.
Also, for a BaseAsyncCmd event with Scheduled state is published only when execute method of the API command class is executed, https://github.com/apache/cloudstack/blob/main/server/src/main/java/com/cloud/api/ApiServer.java#L732-L734. Therefore, even if we refactor EVENT_VM_CREATE publishing on userVmService.createVirtualMachine and EVENT_VM_START on userVmService.startVirtualMachine, without major refactor ApiServer order of events would look like,
EVENT_VM_CREATE Started
EVENT_VM_CREATE Completed
EVENT_VM_CREATE Scheduled
EVENT_VM_STARTED Started
EVENT_VM_STARTED Completed
There was a problem hiding this comment.
That first point you make makes sense, but what is the order of events now?
There was a problem hiding this comment.
I think the 'deploy' would make sense for both newly deployed and started.
There was a problem hiding this comment.
@DaanHoogland updated PR desc with tests cc @rohityadavcloud
|
Should we do a VM.DEPLOY? (deploy could mean both start or create) |
DaanHoogland
left a comment
There was a problem hiding this comment.
LGTM, though I think we may have to revisit what event are generated when at some later point (usage refactor)
@rohityadavcloud will it introduce any issues with usage or existing integrations while consuming events? Maybe it can be discussed/changed in a major release or as part of usage refactor |
|
If the event is still EventTypes.EVENT_VM_CREATE and we're only changing the description, I don't think so @shwstppr but perhaps we should test? |
|
@rohityadavcloud Sorry didn't get you completely. Also, the same event type would be published on the event bus |
|
@shwstppr my bad, no I didn't mean the event type but only the description. |
|
|
Description
Fixes #6697
Allows the server to generate started and completed events for VM.CREATE event type when VM is deployed with startvm=false.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
test-no-start.mp4
How Has This Been Tested?
Without fix (tested deplovm with and without start. VM.CREATE is not published as completed when startvm=false):
With fix (VM.CREATE as completed is published even with startvm=false)