add entity-type to message when no UUID is found for a DB ID #5163

DaanHoogland · 2021-06-28T15:15:11Z

Description

This PR adds a debug facility.

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

blueorangutan · 2021-06-28T15:46:07Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 397

server/src/main/java/com/cloud/uuididentity/UUIDManagerImpl.java

sureshanaparti · 2021-07-01T14:10:06Z

server/src/main/java/com/cloud/uuididentity/UUIDManagerImpl.java

-            throw new InvalidParameterValueException("Unable to find UUID for id " + customId);
+            throw new InvalidParameterValueException(String.format("Unable to find UUID for id %s of type %s",
+                                                                   customId,
+                                                                   (entityType == null ? "<unknown type>" ? entityType.getSimpleName())));


Suggested change

(entityType == null ? "<unknown type>" ? entityType.getSimpleName())));

(entityType == null ? "<null>" ? entityType.getSimpleName())));

I am open to alternatives, but I don't like the , how about just "unknown"?

"unknown" sounds good.

sureshanaparti · 2021-07-01T14:12:11Z

server/src/main/java/com/cloud/uuididentity/UUIDManagerImpl.java

        if (customId == null) {
            return null;
        }
        Identity identity = (Identity) this._entityMgr.findById(entityType, customId);


@DaanHoogland when entityType is null, I think, it fails here (with NPE below stack), can you please check that.

Yhe NPE would be thrown in the call to findById() @sureshanaparti @rhtyd

@Override public <T, K extends Serializable> T findById(Class<T> entityType, K id) { GenericDao<? extends T, K> dao = (GenericDao<? extends T, K>)GenericDaoBase.getDao(entityType); return dao.findById(id); }

I'm fine with applying the extra check in this case but I think it'll be unreachable code.

@sureshanaparti , as shown above, this NPE is going to happen out of scope of this change anyway, in these kinds of bits of code. What do you suggest to do?

@DaanHoogland my suggestion is if entityType is null, better throw exception as below

InvalidParameterValueException("Unknown entity type")

and when identity is null (here entityType is not null), better throw exception as below

InvalidParameterValueException(String.format("Unable to find UUID for id [%s] of type [%s]", customId, entityType.getSimpleName()));

sounds good @sureshanaparti

GutoVeronezi · 2021-07-05T12:48:54Z

server/src/main/java/com/cloud/uuididentity/UUIDManagerImpl.java

-            throw new InvalidParameterValueException("Unable to find UUID for id " + customId);
+            throw new InvalidParameterValueException(String.format("Unable to find UUID for id %s of type %s",
+                                                                   customId,
+                                                                   (entityType == null ? "<unknown type>" ? entityType.getSimpleName())));


"unknown" sounds good.

GutoVeronezi · 2021-07-05T12:59:58Z

server/src/main/java/com/cloud/uuididentity/UUIDManagerImpl.java

        Identity identity = (Identity) this._entityMgr.findById(entityType, customId);
        if (identity == null) {
-            throw new InvalidParameterValueException("Unable to find UUID for id " + customId);
+            throw new InvalidParameterValueException(String.format("Unable to find UUID for id %s of type %s",


I would suggest you to use [] as var delimiter; IMHO, it is easier to recognize what is a variable and what is fixed in the final message; e.g.:

String.format("Unable to find UUID for id [%s] of type [%s].",...

GabrielBrascher

Code LGTM

rohityadavcloud · 2021-07-15T07:20:40Z

@blueorangutan package

blueorangutan · 2021-07-15T07:21:02Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-07-15T07:52:48Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian. SL-JID 551

blueorangutan · 2021-07-16T08:02:56Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian. SL-JID 563

blueorangutan · 2021-07-16T09:12:27Z

Packaging result: ✔️ el7 ✔️ el8 ✖️ debian. SL-JID 565

blueorangutan · 2021-07-16T10:17:05Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 567

blueorangutan · 2021-07-16T14:18:30Z

Trillian Build Failed (tid-1294)

blueorangutan · 2021-07-17T03:52:31Z

Trillian test result (tid-1295)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 44190 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5163-t1295-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 86 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_isolate_network_FW_PF_default_routes_egress_true	`Failure`	291.61	test_routers_network_ops.py
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Failure`	342.20	test_routers_network_ops.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	542.25	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	460.16	test_vpc_redundant.py

blueorangutan · 2021-07-19T07:03:55Z

Trillian test result (tid-1306)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47335 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5163-t1306-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 87 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false	`Failure`	340.28	test_routers_network_ops.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	538.07	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	524.92	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	499.81	test_vpc_redundant.py

blueorangutan · 2021-07-19T08:46:15Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 585

sureshanaparti · 2021-07-19T10:37:17Z

@blueorangutan package

blueorangutan · 2021-07-19T10:38:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-07-19T11:01:10Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 587

sureshanaparti · 2021-07-19T13:20:53Z

@blueorangutan test

blueorangutan · 2021-07-19T13:32:21Z

Trillian Build Failed (tid-1316)

blueorangutan · 2021-07-20T08:17:27Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 595

sureshanaparti · 2021-07-20T10:36:37Z

@blueorangutan package

blueorangutan · 2021-07-20T10:37:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-07-20T11:03:01Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 598

sureshanaparti · 2021-07-20T11:13:59Z

@blueorangutan test

blueorangutan · 2021-07-20T11:15:04Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-07-20T23:10:39Z

Trillian test result (tid-1323)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41259 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5163-t1323-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermittent failure detected: /marvin/tests/smoke/test_service_offerings.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 87 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	1015.47	test_privategw_acl.py
test_01_service_offering_cpu_limit_use	`Error`	811.19	test_service_offerings.py

nvazquez · 2021-07-20T23:41:46Z

@blueorangutan package

blueorangutan · 2021-07-20T23:42:02Z

@nvazquez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-07-21T00:07:31Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 607

nvazquez · 2021-07-21T00:20:10Z

@blueorangutan test

blueorangutan · 2021-07-21T00:21:02Z

@nvazquez a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-07-21T10:08:02Z

Trillian test result (tid-1330)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33541 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5163-t1330-kvm-centos7.zip
Smoke tests completed. 89 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

nvazquez · 2021-07-21T11:00:57Z

Merging based on approvals and test results

add entity-type to message

96b5e99

DaanHoogland marked this pull request as ready for review June 29, 2021 06:25

DaanHoogland added this to the 4.16.0.0 milestone Jun 29, 2021

DaanHoogland added complexity:trivial 10 minnutes to a few days at most Severity:Minor type:enhancement Severity:Trivial and removed Severity:Minor labels Jun 29, 2021

rohityadavcloud reviewed Jun 29, 2021

View reviewed changes

server/src/main/java/com/cloud/uuididentity/UUIDManagerImpl.java Outdated Show resolved Hide resolved

Update server/src/main/java/com/cloud/uuididentity/UUIDManagerImpl.java

3464dcc

sureshanaparti reviewed Jul 1, 2021

View reviewed changes

GutoVeronezi reviewed Jul 5, 2021

View reviewed changes

GabrielBrascher approved these changes Jul 5, 2021

View reviewed changes

sureshanaparti closed this Jul 9, 2021

sureshanaparti reopened this Jul 9, 2021

update for review comments

38c9a9c

rohityadavcloud approved these changes Jul 15, 2021

View reviewed changes

rohityadavcloud closed this Jul 15, 2021

rohityadavcloud reopened this Jul 15, 2021

syntax

63aa3cd

Avoid NPE when entity type is null, and throw appropriate exception

03246f6

apache deleted a comment from blueorangutan Jul 19, 2021

sureshanaparti approved these changes Jul 19, 2021

View reviewed changes

apache deleted a comment from blueorangutan Jul 20, 2021

nvazquez approved these changes Jul 21, 2021

View reviewed changes

nvazquez merged commit 1f8b34f into apache:main Jul 21, 2021

	(entityType == null ? "<unknown type>" ? entityType.getSimpleName())));
	(entityType == null ? "<null>" ? entityType.getSimpleName())));

add entity-type to message when no UUID is found for a DB ID #5163

add entity-type to message when no UUID is found for a DB ID #5163

Uh oh!

Conversation

DaanHoogland commented Jun 28, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

blueorangutan commented Jun 28, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sureshanaparti Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

rohityadavcloud commented Jul 15, 2021

Uh oh!

blueorangutan commented Jul 15, 2021

Uh oh!

blueorangutan commented Jul 15, 2021

Uh oh!

blueorangutan commented Jul 16, 2021

Uh oh!

blueorangutan commented Jul 16, 2021

Uh oh!

blueorangutan commented Jul 16, 2021

Uh oh!

blueorangutan commented Jul 16, 2021

Uh oh!

blueorangutan commented Jul 17, 2021

Uh oh!

blueorangutan commented Jul 19, 2021

Uh oh!

blueorangutan commented Jul 19, 2021

Uh oh!

sureshanaparti commented Jul 19, 2021

Uh oh!

blueorangutan commented Jul 19, 2021

Uh oh!

blueorangutan commented Jul 19, 2021

Uh oh!

sureshanaparti commented Jul 19, 2021

Uh oh!

blueorangutan commented Jul 19, 2021

Uh oh!

blueorangutan commented Jul 20, 2021

Uh oh!

sureshanaparti commented Jul 20, 2021

Uh oh!

blueorangutan commented Jul 20, 2021

sureshanaparti Jul 19, 2021 •

edited

Loading