add entity-type to message when no UUID is found for a DB ID by DaanHoogland · Pull Request #5163 · apache/cloudstack

DaanHoogland · Jun 28, 2021

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 · Jun 28, 2021

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

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

sureshanaparti · Jul 1, 2021

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 · Jul 1, 2021

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

@@ -119,7 +119,9 @@ public <T> String getUuid(Class<T> entityType, Long customId) {
        }
        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 · Jul 5, 2021

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 · Jul 5, 2021

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

yadvr · Jul 15, 2021

@blueorangutan package

blueorangutan · Jul 15, 2021

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

blueorangutan · Jul 15, 2021

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

blueorangutan · Jul 16, 2021

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

blueorangutan · Jul 16, 2021

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

blueorangutan · Jul 16, 2021

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

blueorangutan · Jul 16, 2021

Trillian Build Failed (tid-1294)

blueorangutan · Jul 17, 2021

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 · Jul 19, 2021

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 · Jul 19, 2021

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

sureshanaparti · Jul 19, 2021

@blueorangutan package

blueorangutan · Jul 19, 2021

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

blueorangutan · Jul 19, 2021

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

sureshanaparti · Jul 19, 2021

@blueorangutan test

blueorangutan · Jul 19, 2021

Trillian Build Failed (tid-1316)

blueorangutan · Jul 20, 2021

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

sureshanaparti · Jul 20, 2021

@blueorangutan package

blueorangutan · Jul 20, 2021

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

blueorangutan · Jul 20, 2021

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

sureshanaparti · Jul 20, 2021

@blueorangutan test

blueorangutan · Jul 20, 2021

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

blueorangutan · Jul 20, 2021

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 · Jul 20, 2021

@blueorangutan package

blueorangutan · Jul 20, 2021

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

blueorangutan · Jul 21, 2021

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

nvazquez · Jul 21, 2021

@blueorangutan test

blueorangutan · Jul 21, 2021

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

blueorangutan · Jul 21, 2021

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 · Jul 21, 2021

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

yadvr 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

yadvr approved these changes Jul 15, 2021

View reviewed changes

yadvr closed this Jul 15, 2021

yadvr 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())));

Search code, repositories, users, issues, pull requests...

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!

yadvr 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

Uh oh!

blueorangutan commented Jul 20, 2021

sureshanaparti Jul 19, 2021 •

edited

Loading