[
https://issues.apache.org/jira/browse/CLOUDSTACK-9379?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15415999#comment-15415999
]
ASF GitHub Bot commented on CLOUDSTACK-9379:
--------------------------------------------
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1542
@nvazquez long time we don’t do this ;)
First of all, your PR explanation is great. The code is very well
documented and explained and with test cases (very good ones). Congratulations.
I really like your work.
I have some suggestions for you, though.
I think that the method “enableNestedVirtualization” could return a
Boolean. I see no need to transform the Boolean in a String there; It seems
better to use Boolean.toString(bool) at line 376. Moreover, when reading the
method “enableNestedVirtualization”, I felt like it was going to enable
something on the VM, which is not the case. This looks more a “should”, “can”,
“has” type of method. I mean, it is a method that checks if something has
to/should/can be done; in this case, the enabling of nested virtualization.
Therefore, I think names such as “canUseNestedVirtualization”, “should
enableNestedVirtualization”, “hasToEnableNestedVirtualization” seem more
appropriate.
I also think that the code has room for improvements. First, to reduce the
cyclomatic complexity, you can invert the first if, which become something like
this:
```
if (globalNestedV == null || globalNestedVPerVM == null) {
return false;
}
```
Then, the other conditional can be further improved. It is a bit
complicated. Something like this would have the same result:
```
if (globalNVPVM) {
return (localNestedV == null && globalNV) ||
BooleanUtils.toBoolean(localNestedV);
}
return globalNV;
```
On method “configNestedVirtualization”, I would just suggest using the word
“configure” instead of “config”. At least for me, when I read config, I think
configuration and not configure (this is a very personal opinion, so if you are
ok with config, be my guest). A method, for me, means an action that is
executed, so it seems a better fit the word “configure” (verb).
The method “testConfigNestedVirtualization”, I think it should check if the
“VmDetailConstants.NESTED_VIRTUALIZATION_FLAG” flag (parameter) is being loaded
properly from the “vmDetails”. I also suggest you using the “inOrder” to verify
the calls in order. If the order of the calls changes, the behavior of the
method changes too, right?
About the method “enableNestedVirtualizationBaseTest”, I think it could be
a little bit more self-explaining, such as:
executeAndVerify<nameOfTheMethod>Test.
And finally, about the others test methods, I think instead of TFT, TFN,
and others at the end, I think if you were a bit more literal, and
self-explaining, it would be better. For instance, the test method
“testEnableNestedVirtualizationCaseTFF”, in a more detailed version, could be
read as
“testEnableNestedVirtualizationCaseGlobalNvTrueGlobalNvPvFalseLocalVmfalse”. I
know it is a huge method name, but I think it facilitates for newcomers and
also for the @nvazquez of the future ;)
> Support nested virtualization at VM level on VMware Hypervisor
> --------------------------------------------------------------
>
> Key: CLOUDSTACK-9379
> URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9379
> Project: CloudStack
> Issue Type: Improvement
> Security Level: Public(Anyone can view this level - this is the
> default.)
> Components: VMware
> Affects Versions: 4.9.0
> Reporter: Nicolas Vazquez
> Assignee: Nicolas Vazquez
>
> h2. Introduction
> It is desired to support nested virtualization at VM level for VMware
> hypervisor. Current behaviour supports enabling/desabling global nested
> virtualization by modifying global config {{'vmware.nested.virtualization'}}.
> It is wished to improve this feature, having control at VM level instead of a
> global control only.
> h2. Proposal
> A new global configuration is added, to enable/disable VM nested
> virtualization control: {{'vmware.nested.virtualization.perVM'}}. Default
> value=false
> h2. Behaviour
> After a vm deployment or start command, vm params include
> {{nestedVirtualizationFlag}} key and its value is:
> * true -> nested virtualization enabled
> * false -> nested virtualization disabled
> We will determinate nested virtualization enabled/disabled by examining:
> * (1) global configuration {{'vmware.nested.virtualization'}} value
> * (2) global configuration {{'vmware.nested.virtualization.perVM'}} value
> * (3) {{'nestedVirtualizationFlag'}} value in {{user_vm_details}} if present,
> null if not.
> Using this 3 values, there are different use cases:
> # (1) = TRUE, (2) = TRUE, (3) is null -> ENABLED
> # (1) = TRUE, (2) = TRUE, (3) = TRUE -> ENABLED
> # (1) = TRUE, (2) = TRUE, (3) = FALSE -> DISABLED
> # (1) = TRUE, (2) = FALSE -> ENABLED
> # (1) = FALSE, (2) = TRUE, (3) is null -> DISABLED
> # (1) = FALSE, (2) = TRUE, (3) = TRUE -> ENABLED
> # (1) = FALSE, (2) = TRUE, (3) = FALSE -> DISABLED
> # (1) = FALSE, (2) = FALSE -> DISABLED
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
