CLOUDSTACK-9781:ACS records ID in events tables instead of UUID. #1940
Conversation
|
@blueorangutan package |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-484 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-832)
|
|
@jayantpatil1234 can you please have a look on the volumes and snapshots failures please. |
|
@borisstoyanov I have done required fix and now all test cases are passing, please have a look. |
|
Thanks @jayantpatil1234, will kick the tests again |
|
@borisstoyanov a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-541 |
|
@blueorangutan test |
|
@borisstoyanov a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-901)
|
|
@blueorangutan test cases which are failing, not related to my code changes. Could you restart test suites once again. |
|
Thanks @jayantpatil1234 those failures are known issues that we're looking forward to address soon. No need to rerun. |
|
LGTM for Testing. Checked the Events Tab for the ID it is referring to UUID.
|
|
Code LGTM |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
Though I am fully supporting the use of UUIDs, this is a behavioral change. We need to have this behind a setting and mark the former behaviour as deprecated so people already using this for billing are not hurt.
|
@DaanHoogland Current approach is not consistent between uuid and id and pr is to make it consistent. This change does not impact the usage events. Can you please provide the possible reasons for using events for billing rather than usage events? Also even if some external system is parsing the event text to get the id and call our api based on that, it should work even with this change as our apis treat both id and uuid in the same way |
|
@yvsubhash there are no usage events, these are just copies from the event table. As for APIs treating id and uuid the same way, I would not trust that throughout the system, even when some API might. |
|
@DaanHoogland usage events are a mostly a subset of events. However we are inserting in both the tables separately. So text inserted into events should not affect events. The ID to UUID conversion happens in param processor code before the api actually gets invoked. So I dont see a reason why it would not work for some of the apis |
|
ok @yvsubhash thanks for the explanation |
|
@DaanHoogland would you like to close your review |
| CallContext.current().setEventDetails( | ||
| "Template Id: " + getEntityId() + ((getSnapshotId() == null) ? " from volume Id: " + getVolumeId() : " from snapshot Id: " + getSnapshotId())); | ||
| "Template Id: " + getEntityUuid() + ((getSnapshotId() == null) ? " from volume Id: " + this._uuidMgr.getUuid(Volume.class,getVolumeId()) : " from snapshot Id: " + this._uuidMgr.getUuid(VirtualMachineTemplate.class,getSnapshotId()))); | ||
| VirtualMachineTemplate template = null; |
There was a problem hiding this comment.
Should be Snapshot.class, can you also please check if other entity types match correctly?
| @Override | ||
| public String getEventDescription() { | ||
| return "creating snapshot from vm snapshot : " + getVMSnapshotId(); | ||
| return "creating snapshot from vm snapshot : " + this._uuidMgr.getUuid(Snapshot.class,getVMSnapshotId()); |
There was a problem hiding this comment.
Can you please check this VMSnapshot.class or Snapshot.class
| public void execute() { | ||
| s_logger.info("CreateSnapshotFromVMSnapshotCmd with vm snapshot id:" + getVMSnapshotId() + " and snapshot id:" + getEntityId() + " starts:" + System.currentTimeMillis()); | ||
| CallContext.current().setEventDetails("Vm Snapshot Id: "+ getVMSnapshotId()); | ||
| CallContext.current().setEventDetails("Vm Snapshot Id: "+ this._uuidMgr.getUuid(Snapshot.class,getVMSnapshotId())); |
|
@jayantpatil1234 Can you please look at the comments and resolve the conflicts? |
|
Can you provide some feedback here @syed. |
|
Ping @syed, let's re-discuss this? |
|
Ping @syed, if you don't reply we'll have to consider other's review. Let's discuss? Thanks. |
|
PINGGG @syed |
|
It's also a good idea to start a conversation on dev/user ML about this change. This PR has been blocked for months now, if @syed does not reply by end of this month I'm okay to merge this based on other reviewer's comments and test results on the PR. Thoughts - @DaanHoogland @rafaelweingartner @borisstoyanov ? |
|
In my opinion we can proceed with the merge. @syed inquiries were already addressed. |
|
@blueorangutan package |
|
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
|
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1982 |
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-2554)
|
|
@blueorangutan test |
|
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
Trillian test result (tid-2562)
|
|
The only new issue/failure I see is |
|
I have replied you guys at #2607. This error happens if there are some other testes that change the tags of the primary storage I use. |
|
Okay @rafaelweingartner. Let me diagnose that and get back to you soon. |
|
Merging this based on code reviews and test results. |
CLOUDSTACK-9781:ACS records ID in events tables instead of UUID.ISSUE ===== Wrong presentation of volume id in ACS events. While creating a snapshot, only volume ID is mentioned in the events. For example, Scheduled async job for creating snapshot for volume Id:270". On looking into the notification, user is not able to identify the volume. So modified event description with UUID. Please see screenshots. **Before modification:**  **After modification:**  * pr/1940: CLOUDSTACK-9781:ACS records ID in events tables instead of UUID. Signed-off-by: Pierre-Luc Dion <[email protected]>
Cca custom 4.10jdk8 tmpl fix Fixes problems in apache#1940 See merge request !15
Mudança na mensagem de erro quando é criado um template a partir de volume Closes apache#1940 See merge request scclouds/scclouds!1164
13 participants
ISSUE
Wrong presentation of volume id in ACS events.
While creating a snapshot, only volume ID is mentioned in the events. For example, “Scheduled async job for creating snapshot for volume Id:270". On looking into the notification, user is not able to identify the volume. So modified event description with UUID.
Please see screenshots.
Before modification:
After modification: