CLOUDSTACK-10377: Fix Network restart for Nuage by fmaximus · Pull Request #2672 · apache/cloudstack

fmaximus · 2018-05-24T10:03:01Z

Description

Changes in PR #2508 have caused network restart to fail in a Nuage setup,
as the new VR takes the same IP as the old one, and the old VR is still running.
Nuage doesn't support multiple VM's having the same IP.
We delay provisioning the interfaces in VSD until the old VR interface is released.

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?

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

yadvr

LGTM, we cannot test nuage related code but I'll help kick trillian test.

yadvr · 2018-05-24T10:49:43Z

@blueorangutan package

blueorangutan · 2018-05-24T10:49:52Z

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

blueorangutan · 2018-05-24T11:31:11Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2085

yadvr · 2018-05-24T11:38:49Z

@blueorangutan test

blueorangutan · 2018-05-24T11:38:53Z

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

blueorangutan · 2018-05-24T19:03:17Z

Trillian test result (tid-2710)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 25005 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2672-t2710-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 65 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	`Error`	130.64	test_privategw_acl.py

rafaelweingartner · 2018-05-25T10:48:29Z

+                ReserveVmInterfaceVspCommand cmd = new ReserveVmInterfaceVspCommand(vspNetwork, vspVm, vspNic, vspStaticNat, dhcpOption);
+                Answer answer = _agentMgr.easySend(nuageVspHost.getId(), cmd);
+
+                if (answer == null || !answer.getResult()) {


You can use BooleanUtils here.

The null check is on answer, not on answer.getResult().

rafaelweingartner · 2018-05-25T10:50:57Z

-            VspDhcpVMOption dhcpOption = _nuageVspEntityBuilder.buildVmDhcpOption(nicFromDb, defaultHasDns, networkHasDns);
-            ReserveVmInterfaceVspCommand cmd = new ReserveVmInterfaceVspCommand(vspNetwork, vspVm, vspNic, vspStaticNat, dhcpOption);
-            Answer answer = _agentMgr.easySend(nuageVspHost.getId(), cmd);
+            if (!Boolean.TRUE.equals(vm.getParameter(VirtualMachineProfile.Param.RollingRestart))) {


Would you mind extracting these blocks of code to specific methods and unit testing them? Some documentation might help as well.

This is a comment to for this PR only: We really need to change our mind set and start using smaller, concise, well-documented and unit-tested methods. This in turn can greatly improve our productivity.

rafaelweingartner · 2018-05-25T10:51:28Z

-            VspDhcpVMOption dhcpOption = _nuageVspEntityBuilder.buildVmDhcpOption(nicFromDb, defaultHasDns, networkHasDns);
-            ReserveVmInterfaceVspCommand cmd = new ReserveVmInterfaceVspCommand(vspNetwork, vspVm, vspNic, vspStaticNat, dhcpOption);
-            Answer answer = _agentMgr.easySend(nuageVspHost.getId(), cmd);
+            if (!Boolean.TRUE.equals(vm.getParameter(VirtualMachineProfile.Param.RollingRestart))) {


You can use BooleanUtils to evaluate vm.getParameter(VirtualMachineProfile.Param.RollingRestart))

Then I would have to cast, as vm.getParameter() returns Object

rafaelweingartner · 2018-05-25T10:52:16Z

                super.deallocate(network, nic, vm);
            }
+
+            if (virtualMachine.getType() == VirtualMachine.Type.DomainRouter) {


Can you extract this blob to a method?

Do we really want to go into creating methods for checking enum values?
I'm not in favor of creating that kind of method in this class,
so it should go either in VirtualMachine or in VirtualMachine.Type

I am pretty sure @rafaelweingartner means the entire if block and not the enum check, @fmaximus

Spot on. I was referring to the IF body, and not to the IF condition. We really need shorter methods and more unit tests.

yadvr · 2018-05-29T10:40:03Z

@fmaximus can you address outstanding review comments?

Address review comments

yadvr · 2018-06-04T07:53:30Z

While one comment around refactoring code to a different method is outstanding, @fmaximus can you comment? I'm okay to merge this in order to unblock for RC2. /cc @PaulAngus @DaanHoogland

fmaximus · 2018-06-04T09:17:09Z

Still was running some tests on the refactoring.

yadvr · 2018-06-04T11:08:31Z

@blueorangutan package

blueorangutan · 2018-06-04T11:08:42Z

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

DaanHoogland · 2018-06-04T11:31:11Z

ok by me @rhtyd should it be packaged and smoke tested again? I saw you started that

blueorangutan · 2018-06-04T11:35:47Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2088

yadvr · 2018-06-04T11:43:20Z

@blueorangutan test

blueorangutan · 2018-06-04T11:43:42Z

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

fmaximus · 2018-06-04T12:34:54Z

We found some issues with the fix, we're looking into it.

blueorangutan · 2018-06-04T18:59:40Z

Trillian test result (tid-2719)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 24362 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2672-t2719-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	141.16	test_privategw_acl.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	1.55	test_hostha_kvm.py

yadvr · 2018-06-05T10:32:35Z

Test LGTM, let us know (soon) @fmaximus when you're good with your PR(s). Also, please use Github to log issues, thanks.

fmaximus · 2018-06-05T13:21:08Z

It's good!

DaanHoogland · 2018-06-05T14:07:53Z

@PaulAngus I did a walk through and am fine to merge this one, ok?

yadvr · 2018-06-06T06:46:54Z

LGTM, based on code reviews and results I'm okay to merge this as well

CLOUDSTACK-10377: Fix Network restart for Nuage

5e0dd3f

fmaximus added the type:bug label May 24, 2018

fmaximus added this to the 4.11.1.0 milestone May 24, 2018

fmaximus self-assigned this May 24, 2018

fmaximus requested a review from yadvr May 24, 2018 10:03

yadvr approved these changes May 24, 2018

View reviewed changes

yadvr added the Severity:BLOCKER label May 24, 2018

yadvr requested review from DaanHoogland, GabrielBrascher, rafaelweingartner and wido May 25, 2018 06:04

krissterckx approved these changes May 25, 2018

View reviewed changes

rafaelweingartner reviewed May 25, 2018

View reviewed changes

CLOUDSTACK-10377: Network Restart for Nuage

6985f28

Address review comments

yadvr merged commit 8798014 into apache:4.11 Jun 6, 2018

Conversation

fmaximus commented May 24, 2018

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented May 24, 2018

Uh oh!

blueorangutan commented May 24, 2018

Uh oh!

blueorangutan commented May 24, 2018

Uh oh!

yadvr commented May 24, 2018

Uh oh!

blueorangutan commented May 24, 2018

Uh oh!

blueorangutan commented May 24, 2018

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented May 29, 2018

Uh oh!

yadvr commented Jun 4, 2018

Uh oh!

fmaximus commented Jun 4, 2018

Uh oh!

yadvr commented Jun 4, 2018

Uh oh!

blueorangutan commented Jun 4, 2018

Uh oh!

DaanHoogland commented Jun 4, 2018

Uh oh!

blueorangutan commented Jun 4, 2018

Uh oh!

yadvr commented Jun 4, 2018

Uh oh!

blueorangutan commented Jun 4, 2018

Uh oh!

fmaximus commented Jun 4, 2018

Uh oh!

blueorangutan commented Jun 4, 2018

Uh oh!

yadvr commented Jun 5, 2018

Uh oh!

fmaximus commented Jun 5, 2018

Uh oh!

DaanHoogland commented Jun 5, 2018

Uh oh!

yadvr commented Jun 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants