Show network name in exception message

[ravening]

Description

Display network name in exception message instead of network object

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)

Screenshots (if appropriate):

How Has This Been Tested?

[DaanHoogland]

this might yield an NPE hiding the original problem, no?

[GabrielBrascher]

I see no problem in adding an if checking for a non-null name. However, a network can only be created with a name.

If the network is a non-null Object then I don't see how the name would be null. Unless manual queries have been executed on DB (messing with networks) or there are some bugs on the network removal/update process.

[ravening]

@DaanHoogland @GabrielBrascher most of the time the network name will be nonnull but there is no harm adding an extra if check. I will modify the code

[DaanHoogland]

I am talking about the model returning a network in the first place. the namer being null will probably just print "<null>", @ravening @GabrielBrascher

[ravening]

@DaanHoogland we are iterating over the list of networks which got populated earlier and it contains only valid existing networks. so networkmodel shouldnt return null

[DaanHoogland]

@DaanHoogland we are iterating over the list of networks which got populated earlier and it contains only valid existing networks. so networkmodel shouldnt return null

Maybe so @ravening, i trust your judgement on that. But it didn't happen in this method, so there is no telling what future generations will make of it.

[Pearl1594]

@ravening How about just adding the network name to the NetworkVO toString(), so this remains the same, and we get to see the network's name along with other details like networkId, traffic type, etc?

@Pearl1594 should we all those detail in exception message? It makes sense to add entire networkVO object tostring in log message

It actually depends, we may not want to show the network Id (i.e, DB id) to users, so maybe replace id with name in the toString()

[ravening]

@ravening How about just adding the network name to the NetworkVO toString(), so this remains the same, and we get to see the network's name along with other details like networkId, traffic type, etc?

@Pearl1594 should we all those detail in exception message? It makes sense to add entire networkVO object tostring in log message

It actually depends, we may not want to show the network Id (i.e, DB id) to users, so maybe replace id with name in the toString()

That will involve extra code changes which I want to avoid

[Pearl1594]

@ravening while it does make sense to show the network name, we do not ensure uniqueness in network names in CloudStack - so would it lead to ambiguity just showing the name?

[ravening]

@ravening while it does make sense to show the network name, we do not ensure uniqueness in network names in CloudStack - so would it lead to ambiguity just showing the name?

@Pearl1594 this exception is thrown when we are trying to create second VM with same in same network. So I would assume, user will know in which network he is creating the VM. Also even if the network name is not unique, it's better than displaying VO object right which won't make sense anyway

[GabrielBrascher]

Thanks for the PR @ravening.

I am +1 on adding the network's name on exception. I did not add a looks good for this PR in respect to Daan's comment.

[GabrielBrascher]

I see no problem in adding an if checking for a non-null name. However, a network can only be created with a name.

If the network is a non-null Object then I don't see how the name would be null. Unless manual queries have been executed on DB (messing with networks) or there are some bugs on the network removal/update process.

[ravening]

Thanks for the PR @ravening.

I am +1 on adding the network's name on exception. I did not add a looks good for this PR in respect to Daan's comment.

@GabrielBrascher Daan was mentioning about networkmodel retuning a null rather than network name being null. So i think this should be fine and no need for extra if check

[GabrielBrascher]

@rhtyd suggested implementing NetworkVO toString(). The toString() sounds like a good plan.

Either way, I am OK with both approaches. Code LGTM.

[davidjumani]

LGTM!

[yadvr]

@ravening can you fix the conflict?

[ravening]

@ravening can you fix the conflict?

@rhtyd done

[yadvr]

@blueorangutan package

[blueorangutan]

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

[blueorangutan]

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2296

[DaanHoogland]

@blueorangutan test

[blueorangutan]

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

[blueorangutan]

Trillian test result (tid-3097)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 31757 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4396-t3097-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_privategw_acl.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_rvpc_privategw_static_routes	Failure	869.32	test_privategw_acl.py

[weizhouapache]

@rhtyd @ravening
build failed due to a missing ")" at the end of line.

[weizhouapache]

                 // * verify that there are no duplicates
                 if (hostNames.contains(hostName)) {
                     throw new InvalidParameterValueException("The vm with hostName " + hostName + " already exists in the network domain: " + ntwkDomain.getKey() + "; network="
-                            + (_networkModel.getNetwork(ntwkId) != null) ? _networkModel.getNetwork(ntwkId).getName()) : "<unknown>";
+                            + (_networkModel.getNetwork(ntwkId) != null) ? _networkModel.getNetwork(ntwkId).getName() : "<unknown>");
                 }
             }
         }

[weizhouapache]

good it has been fixed by @rhtyd