[GitHub] cloudstack issue #1542: CLOUDSTACK-9379: Support nested virtualization at VM...

[rafaelweingartner]

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


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---