Fix event UUIDS missing on event bus

[ProjectMoon]

The fixing of CLOUDSTACK-8816 introduced a regression that removed the first class entities in the event bus description property. This is because everything was changed to use the Class as a key... Everything but the populateFirstClassEntities method in ActionEventUtils.

This commit tries to load the class key instead of the String key, which re-enables the populateFirstClassEntities method.

Likely this was not caught because of the trace exception handling... maybe some better logging/unit tests would be good for this PR.

[ProjectMoon]

To test, remote debug an in-memory event bus with a command like disable or enable static NAT. Check event description property before and after. If this commit is applied, it will have the UUIDs of the first class entities involved in the API call.

I think I will write a unit test for this to make sure this method is working. What file should I put it in?

[DaanHoogland]

@ProjectMoon server/test/com/cloud/event/ActionEventUtilsTest.java

and if you can maybe an integration test?

[ProjectMoon]

Integration tests use Marvin, right?

[DaanHoogland]

Yes, preferably but not necessarily. you can create your own if you want, based on scripts and cloudmonkey for instance or a similar tool.

[ProjectMoon]

Currently unable to build the branch due to #1110. What's the status of that request being merged in?

[DaanHoogland]

It is missing a review and LGTM, (with proper motivation)

[ProjectMoon]

For now I will cherry-pick that commit and use it locally to build my tests. It will not become part of this pull request of course.

[ProjectMoon]

The commit has been updated with a unit test. I also rebased this branch to latest master since my apparent accidental review of #1110 got the syntax error fix into the master branch.

[karuturi]

Hi @ProjectMoon Thanks for taking this up. If possible, can you put in a sample event before and after the fix?
I will test this tomorrow with rabbitmq

[ProjectMoon]

Example with the fix:

{
    eventDateTime=2015-11-24 16:57:58 +0000,
    status=Completed,
    description=Successfully completed enabling static nat. Ip Id: 4,
    event=STATICNAT.ENABLE,
    entityuuid=null,
    entity=null,
    account=7622afee-8ec9-11e5-9cba-54ee754a4435,
    IpAddress=4073023a-1734-4af9-b4e7-79999b39d176,
    VirtualMachine=7ea2776f-d8e7-42da-8be1-d93d161b613a,
    user=7622d527-8ec9-11e5-9cba-54ee754a4435
}

Without the fix:

{
    eventDateTime=2015-11-24 17:01:38 +0000,
    status=Completed,
    description=Successfully completed enabling static nat. Ip Id: 4,
    event=STATICNAT.ENABLE,
    entityuuid=null,
    entity=null,
    account=7622afee-8ec9-11e5-9cba-54ee754a4435,
    user=7622d527-8ec9-11e5-9cba-54ee754a4435
}

The first class entities found in the case of static NAT operations are VM ID and IP ID. This is the eventDescription property of the event sent over the event bus.

[karuturi]

manually tested it with rabbitmq integration

Here are the events before and after the fix

Before:
{"eventDateTime":"2015-11-25 11:39:27 +0530","status":"Completed","description":"Successfully completed enabling static nat. Ip Id: 4","event":"STATICNAT.ENABLE","account":"bd73dc2e-35c0-11e5-b094-d4ae52cb9af0","user":"bd7ea748-35c0-11e5-b094-d4ae52cb9af0"}

After:
{"eventDateTime":"2015-11-25 12:03:40 +0530","status":"Completed","description":"Successfully completed enabling static nat. Ip Id: 4","event":"STATICNAT.ENABLE","account":"bd73dc2e-35c0-11e5-b094-d4ae52cb9af0","IpAddress":"0d1e8fd3-e48d-43d7-b2c0-f76ba0cbcf5a","VirtualMachine":"aa9d4cfe-f42a-4b27-acd5-c5ded84ae668","user":"bd7ea748-35c0-11e5-b094-d4ae52cb9af0"}

👍

[karuturi]

@remibergsma @DaanHoogland can you take a look at this please? Its a good fix to have in 4.6.1
I didnt run the test suite. But, manually tested this fix.

[DaanHoogland]

@karuturi I don't think there is a test for this in the suites we use. I'll them anyway to at least see about regression in some other places.

[DaanHoogland]

@ProjectMoon there was one failure in the network tests for the password server. I am rerunning that one to make sure.
1111.network.results.txt
1111.vpc.results.txt

[DaanHoogland]

Check the password file in the Router VM ... === TestName: test_isolate_network_password_server | Status : SUCCESS ===
ok

----------------------------------------------------------------------
Ran 1 test in 367.955s

OK

So it was a fluke

[DaanHoogland]

LGTM

[karuturi]

I just realised the target branch should be 4.6 meaning the PR should be for 4.6 and fwd-merged to master. Can you take care of that please @ProjectMoon

[ProjectMoon]

Well, the target branch for this is master so it will be put in 4.7-SNAPSHOT, but it's also intended to be cherry-picked back into 4.6 (which I obviously do not have the access to do).

I can create a second pull request on the 4.6 branch as well if you want. What is the proper procedure for this?

[karuturi]

No cherry-picking. The PR should be only for 4.6 which will be fwd-merged to master(and hence will be available in 4.7 and up)
I wish it was as easy as editing this PR and changing the target branch. But, github doesnt provide that and a new PR is required. Since the commit ids are same, when the 4.6 PR is merged both will be marked merged. (or you could close this now itself and just reference this PR in the new PR)
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Release+principles+for+Apache+CloudStack+4.6+and+up

[ProjectMoon]

Since the branches are different, I will create a new branch off of 4.6, cherry-pick my commit to it, and then submit that as a new PR, which can then be forward merged.

[ProjectMoon]

Closed in favor of #1127.