Refactor and improvements for method com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.createVMFromSpec() #5149

SadiJr · 2021-06-23T16:19:54Z

Description

This PR goal is to refactor the method createVMFromSpec(final VirtualMachineTO vmTO) in class com.cloud.hypervisor.kvm.resource.LibvirtComputingResource and improve its logs, javadocs, while adding unit tests for all of the code that is extracted and modified.

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)

How Has This Been Tested?

All unit tests have been tested, and I've checked that the results on the main branch are the same of this refactor.

DaanHoogland

great improvement in readability. I am missing how you verified that the "results are the same", as you say. Did you test all kinds of configurations i.e. with config drive and in different kinds of networks (L2, VPC). Can you expand on that please?

blueorangutan · 2021-06-24T08:49:01Z

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

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

SadiJr · 2021-06-24T14:49:41Z

@DaanHoogland I literally debug the existing tests in main and checked the VM XML settings for each test. And I repeated this process on the branch where I made the changes to compare the results.

DaanHoogland · 2021-06-25T10:29:45Z

@DaanHoogland I literally debug the existing tests in main and checked the VM XML settings for each test. And I repeated this process on the branch where I made the changes to compare the results.

tnx, code looks good, but need testing.

GutoVeronezi

CLGTM

GabrielBrascher

@SadiJr some adjustments are necessary. I've left some comments, please take a look at them.

I want to be clear that I am surprised (in a good way) by the quality of this refactoring.

Kudos for 1) documentation, 2) code extraction to concise methods, and 3) addition of JUnit tests covering some key parts of the execution flow.

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

SadiJr · 2021-07-01T15:15:08Z

@GabrielBrascher Thanks for the suggestions. Most have been implemented, although there is a suggestion that I don't quite understand how useful it is.

api/src/main/java/com/cloud/agent/api/to/VirtualMachineTO.java

…ctor-method-createVMFromSpec

blueorangutan · 2021-07-16T08:54:22Z

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

blueorangutan · 2021-07-16T20:00:38Z

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

Test	Result	Time (s)	Test File

DaanHoogland

clgtm and good volume of unit tests added. I do wonder if the continued support for LXC is realistic, though.

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

…s/cloudstack into refactor-method-createVMFromSpec

SadiJr · 2021-07-19T17:36:26Z

@DaanHoogland Thanks for the review.

I agree with your opinion about the LXC support, but I think is better to discuss this in an issue or an PR specific for it.

…ctor-method-createVMFromSpec

GabrielBrascher

Code LGTM

nvazquez · 2021-07-20T03:10:00Z

Looks really nice @SadiJr.
@davidjumani this PR will require some effort to test on KVM ensuring there are no regressions, let's try to find some time for testing it

…ctor-method-createVMFromSpec

davidjumani · 2021-07-21T04:44:16Z

@blueorangutan package

blueorangutan · 2021-07-21T04:45:02Z

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

davidjumani

CLGTM. Will test and report

blueorangutan · 2021-07-21T05:15:47Z

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

davidjumani · 2021-07-21T05:21:25Z

@blueorangutan test keepEnv

blueorangutan · 2021-07-21T05:22:03Z

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

davidjumani

Tested against main, similar xmls created

blueorangutan · 2021-07-21T17:51:33Z

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

Test	Result	Time (s)	Test File
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	571.39	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Failure`	422.31	test_vpc_redundant.py

nvazquez

LGTM

SadiJr added 3 commits June 21, 2021 18:12

Refactor method createVMFromSpec

801f842

Add unit tests

220672f

Fix test

e19f2a5

DaanHoogland reviewed Jun 24, 2021

View reviewed changes

GutoVeronezi reviewed Jun 24, 2021

View reviewed changes

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java Outdated Show resolved Hide resolved

Extract if block to method for add extra configs to VM Domain XML

631532b

DaanHoogland added the status:needs-testing label Jun 25, 2021

GutoVeronezi approved these changes Jun 29, 2021

View reviewed changes

SadiJr added 2 commits June 29, 2021 17:31

Merge branch 'main' into refactor-method-createVMFromSpec

bcec279

Split travis tests trying to isolate which test is causing an error

827fd22

GabrielBrascher reviewed Jul 1, 2021

View reviewed changes

SadiJr added 3 commits July 1, 2021 11:49

Override toString() method

b1bbc53

Update documentation

ff482d9

Fix checkstyle error (line with trailing spaces)

f9f2de7

GabrielBrascher reviewed Jul 1, 2021

View reviewed changes

api/src/main/java/com/cloud/agent/api/to/VirtualMachineTO.java Outdated Show resolved Hide resolved

SadiJr added 5 commits July 1, 2021 16:57

Change VirtualMachineTO print of object

fb37ef5

Add try except to find message error. Remove after test

7f49350

Fix indent

62f8a2c

Trying to understanding why is happening in this code

3a5dde9

Merge branch 'main' of https://github.com/apache/cloudstack into refa…

3292df6

…ctor-method-createVMFromSpec

rafaelweingartner force-pushed the refactor-method-createVMFromSpec branch from 3292df6 to fb37ef5 Compare July 6, 2021 12:07

SadiJr added 5 commits July 6, 2021 11:13

Refactor method createVMFromSpec

cc8df0f

Add unit tests

2d3005e

Fix test

2d02ede

Extract if block to method for add extra configs to VM Domain XML

e7a88ae

Split travis tests trying to isolate which test is causing an error

e35ee3c

SadiJr added 2 commits July 6, 2021 11:13

Override toString() method

9377161

Update documentation

1bf0d0f

rafaelweingartner force-pushed the refactor-method-createVMFromSpec branch from 897fe52 to 7300e03 Compare July 6, 2021 14:14

Fix checkstyle error (line with trailing spaces)

e2a52e5

rafaelweingartner force-pushed the refactor-method-createVMFromSpec branch from d4428f3 to e2a52e5 Compare July 8, 2021 20:03

SadiJr requested review from DaanHoogland and GabrielBrascher July 15, 2021 13:19

sureshanaparti added this to the 4.16.0.0 milestone Jul 16, 2021

DaanHoogland reviewed Jul 19, 2021

View reviewed changes

...ypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java Outdated Show resolved Hide resolved

SadiJr added 2 commits July 19, 2021 11:39

Remove unnecessary comment

58a4d73

Merge branch 'refactor-method-createVMFromSpec' of github.com:sccloud…

97205d7

…s/cloudstack into refactor-method-createVMFromSpec

SadiJr added 2 commits July 19, 2021 14:42

Revert travis tests

1c54f8a

Merge branch 'main' of https://github.com/apache/cloudstack into refa…

89e32a8

…ctor-method-createVMFromSpec

GabrielBrascher approved these changes Jul 20, 2021

View reviewed changes

nvazquez added the type:cleanup label Jul 20, 2021

Merge branch 'main' of https://github.com/apache/cloudstack into refa…

1eda61f

…ctor-method-createVMFromSpec

davidjumani reviewed Jul 21, 2021

View reviewed changes

davidjumani approved these changes Jul 21, 2021

View reviewed changes

nvazquez approved these changes Jul 21, 2021

View reviewed changes

nvazquez merged commit eff2da2 into apache:main Jul 21, 2021

Refactor and improvements for method com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.createVMFromSpec() #5149

Refactor and improvements for method com.cloud.hypervisor.kvm.resource.LibvirtComputingResource.createVMFromSpec() #5149

Uh oh!

Conversation

SadiJr commented Jun 23, 2021

Description

Types of changes

How Has This Been Tested?

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jun 24, 2021

Uh oh!

Uh oh!

SadiJr commented Jun 24, 2021

Uh oh!

DaanHoogland commented Jun 25, 2021

Uh oh!

GutoVeronezi left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SadiJr commented Jul 1, 2021

Uh oh!

Uh oh!

blueorangutan commented Jul 16, 2021

Uh oh!

blueorangutan commented Jul 16, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SadiJr commented Jul 19, 2021

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

nvazquez commented Jul 20, 2021

Uh oh!

davidjumani commented Jul 21, 2021

Uh oh!

blueorangutan commented Jul 21, 2021

Uh oh!

davidjumani left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 21, 2021

Uh oh!

davidjumani commented Jul 21, 2021

Uh oh!

blueorangutan commented Jul 21, 2021

Uh oh!

davidjumani left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 21, 2021

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development