Fix stop VM issue on basic zones by nvazquez · Pull Request #3501 · apache/cloudstack

nvazquez · 2019-07-18T04:12:22Z

Description

CloudStack reports an error while stopping/expunging VMs on basic zones

Fixes: #3497

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?

VM lifecycle

nvazquez · 2019-07-18T04:13:18Z

@blueorangutan package

blueorangutan · 2019-07-18T04:13:45Z

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

blueorangutan · 2019-07-18T04:33:48Z

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

nvazquez · 2019-07-18T04:58:24Z

@blueorangutan package

blueorangutan · 2019-07-18T04:58:44Z

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

blueorangutan · 2019-07-18T05:17:45Z

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

nvazquez · 2019-07-18T05:21:16Z

@blueorangutan test

blueorangutan · 2019-07-18T05:21:44Z

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

yadvr

Did not test it

blueorangutan · 2019-07-18T14:03:12Z

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

Test	Result	Time (s)	Test File
test_04_rvpc_network_garbage_collector_nics	`Failure`	309.25	test_vpc_redundant.py

server24 · 2019-07-18T14:21:18Z

Did not test it

@rhtyd @anuragaw @nvazquez
This would avoid the command being executed at all, which is not the original intention. The destination is not necessary when removing something, as the DhcpRule in this case should already be deployed. So wouldn't it make more sense to change the if to:

if (router != null &&
(remove || (destination != null &&
destination.getPod() != null &&
router.getPodIdToDeployIn() != null &&
router.getPodIdToDeployIn().longValue() == destination.getPod().getId()))) {

I have recompiled with this test and the DHCP-entries are correct in the Basic Networking (create, start, stop, destroy, expunge VM tested).

borisstoyanov

IDK if this is expected but if you try to restart the network with clean up in a basic zone you get the following exception:

2019-07-19 09:04:23,245 ERROR [c.c.a.ApiAsyncJobDispatcher] (API-Job-Executor-20:ctx-4eb94b11 job-40) (logid:bfca90d8) Unexpected exception while executing org.apache.cloudstack.api.command.user.network.RestartNetworkCmd
com.cloud.exception.ResourceUnavailableException: Resource [DataCenter:1] is unreachable: Can't find all necessary running routers!
at com.cloud.network.element.VirtualRouterElement.implement(VirtualRouterElement.java:244)
at org.apache.cloudstack.engine.orchestration.NetworkOrchestrator.implementNetworkElements(NetworkOrchestrator.java:1287)
at org.apache.cloudstack.engine.orchestration.NetworkOrchestrator.rollingRestartRouters(NetworkOrchestrator.java:3063)
at org.apache.cloudstack.engine.orchestration.NetworkOrchestrator.restartNetwork(NetworkOrchestrator.java:2940)
at com.cloud.network.NetworkServiceImpl.restartNetwork(NetworkServiceImpl.java:1902)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

server24 · 2019-07-19T18:41:43Z

IDK if this is expected but if you try to restart the network with clean up in a basic zone you get the
following exception:

this is an issue that I had already reported the 3rd of January, 2019 to the developers list, relevant excerpt:

...
a Network restart triggered using the UI fails with:

"Can't find all necessary running routers!"

I have looked up the source code for it and came to the following relevant snippet in

server/src/main/java/com/cloud/network/element/VirtualRouterElement.java

starting at line 234:

 if (offering.isRedundantRouter() || network.isRollingRestart()) {
expectedRouters = 2;
}

From the log files I see that a rollingRestart is indeed triggered, but why does it expect two routers then? Because there might be two routers running simultaneously during the restart? If that was the case, then why in the following lines, relevant snippet (starting at line 237):

 if (routers == null || routers.size() < expectedRouters) {
//we might have a router which is already deployed and running.
//so check the no of routers in network currently.
List<DomainRouterVO> current_routers = _routerDao.listByNetworkAndRole(network.getId(), Role.VIRTUAL_ROUTER);
if (current_routers.size() < 2) {
updateToFailedState(network);
throw new ResourceUnavailableException("Can't find all necessary running routers!", DataCenter.class, network.getDataCenterId());
}
}

is throwing an Exception when current_routers.size() is smaller than 2 and not smaller than 1?

I understand that the logic is designed towards more complex networks, but will this logic not always fails for an already implemented BASIC network on KVM, with no redudant routers enabled and one VR already active?
...

Back then nobody replied or gave feedback, glad that finally somebody is looking at this.

Generally I have the feeling that no testing is ever done for Basic Networking... is everybody just using Advanced Networking with CS?

yadvr · 2019-07-22T05:53:32Z

@nvazquez can you check @server24 's comment?

yadvr · 2019-07-22T05:54:14Z

@blueorangutan package

blueorangutan · 2019-07-22T05:55:03Z

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

blueorangutan · 2019-07-22T07:03:02Z

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

yadvr · 2019-07-22T09:47:29Z

@blueorangutan test

blueorangutan · 2019-07-22T09:48:06Z

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

DaanHoogland · 2019-07-22T11:14:33Z

Aren't we going to drop support for basic zones? How does this fit in that scenario? cc @GabrielBrascher @wido @rhtyd

yadvr · 2019-07-22T12:04:18Z

@DaanHoogland it has been discussed but a concrete plan has not been proposed. I think that's a very long term thing, till then the feature should be supported like other features.

yadvr · 2019-07-22T16:26:34Z

@server24 can you test the PR?

nvazquez · 2019-07-22T17:03:01Z

@rhtyd @server24 I'll address the open comments today

blueorangutan · 2019-07-22T20:10:08Z

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

Test	Result	Time (s)	Test File
test_05_rvpc_multi_tiers	`Failure`	414.60	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	440.82	test_vpc_redundant.py

nvazquez · 2019-07-23T04:05:39Z

@server24 I have pushed a fix based on your previous comment, thanks!

Regarding network restart with cleanup, it is possible by previously changing the setting 'network.rolling.restart' to false.

nvazquez · 2019-07-23T04:06:17Z

@blueorangutan package

blueorangutan · 2019-07-23T04:07:07Z

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

blueorangutan · 2019-07-23T04:27:10Z

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

nvazquez · 2019-07-23T04:31:15Z

@blueorangutan test matrix

blueorangutan · 2019-07-23T04:32:07Z

@nvazquez a Trillian-Jenkins matrix job (centos6 mgmt + xs71, centos7 mgmt + vmware65, centos7 mgmt + kvmcentos7) has been kicked to run smoke tests

borisstoyanov

I've manually checked the fix on a basic zone and it looks good. Let's wait for the final results

anuragaw

LGTM.

(Tricky bug)

blueorangutan · 2019-07-23T14:00:10Z

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

Test	Result	Time (s)	Test File

nvazquez · 2019-07-23T14:14:17Z

@rhtyd there are still vmware and xenserver test jobs running

blueorangutan · 2019-07-23T19:46:57Z

Trillian test result (tid-197)
Environment: vmware-65u2 (x2), Advanced Networking with Mgmt server 7
Total time taken: 52509 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3501-t197-vmware-65u2.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Intermittent failure detected: /marvin/tests/smoke/test_routers.py
Intermittent failure detected: /marvin/tests/smoke/test_snapshots.py
Intermittent failure detected: /marvin/tests/smoke/test_usage.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 74 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_reboot_router	`Error`	337.88	test_network.py
test_02_list_snapshots_with_removed_data_store	`Error`	1.21	test_snapshots.py
test_01_volume_usage	`Error`	9.46	test_usage.py

Fix stop VM issue on basic zones
dbc6ac9

nvazquez requested review from andrijapanicsb, borisstoyanov and yadvr July 18, 2019 04:12

nvazquez added component:basic-network type:bug labels Jul 18, 2019

anuragaw reviewed Jul 18, 2019
View reviewed changes

Comment threadserver/src/main/java/org/apache/cloudstack/network/topology/BasicNetworkVisitor.java Outdated

Add defensive checks
a992f32

borisstoyanov added this to the 4.13.0.0 milestone Jul 18, 2019

yadvr approved these changes Jul 18, 2019
View reviewed changes

borisstoyanov reviewed Jul 19, 2019
View reviewed changes

yadvr approved these changes Jul 22, 2019
View reviewed changes

Fix condition on basic network visitor
6e190e0

borisstoyanov reviewed Jul 23, 2019
View reviewed changes

anuragaw approved these changes Jul 23, 2019
View reviewed changes

nvazquez self-assigned this Jul 23, 2019

yadvr merged commit 281148d into apache:masterJul 23, 2019

GabrielBrascher mentioned this pull request Oct 28, 2019
Have persistent DHCP leases file on VRs and cleanup /etc/hosts on VM deletion #3351
Merged
5 tasks

GabrielBrascher added a commit to CLDIN/cloudstack that referenced this pull request Oct 28, 2019
Add changes from PRapache#3501into CloudStack 4.11.3
e3a7348

nvazquez deleted the fix3497 branch April 6, 2020 14:36

Uh oh!

Conversation

nvazquez commented Jul 18, 2019

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

nvazquez commented Jul 18, 2019

Uh oh!

blueorangutan commented Jul 18, 2019

Uh oh!

Uh oh!

blueorangutan commented Jul 18, 2019

Uh oh!

nvazquez commented Jul 18, 2019

Uh oh!

blueorangutan commented Jul 18, 2019

Uh oh!

blueorangutan commented Jul 18, 2019

Uh oh!

nvazquez commented Jul 18, 2019

Uh oh!

blueorangutan commented Jul 18, 2019

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 18, 2019

Uh oh!

server24 commented Jul 18, 2019

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

server24 commented Jul 19, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

DaanHoogland commented Jul 22, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

yadvr commented Jul 22, 2019

Uh oh!

nvazquez commented Jul 22, 2019

Uh oh!

blueorangutan commented Jul 22, 2019

Uh oh!

nvazquez commented Jul 23, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nvazquez commented Jul 23, 2019

Uh oh!

blueorangutan commented Jul 23, 2019

Uh oh!

blueorangutan commented Jul 23, 2019

Uh oh!

nvazquez commented Jul 23, 2019

Uh oh!

blueorangutan commented Jul 23, 2019

Uh oh!

borisstoyanov left a comment

Choose a reason for hiding this comment

Uh oh!

anuragaw left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Jul 23, 2019

server24 commented Jul 19, 2019•
edited
Loading

nvazquez commented Jul 23, 2019•
edited
Loading