Fix failure in validating IP address in case of multiple Management Servers by Pearl1594 · Pull Request #4507 · apache/cloudstack · GitHub

Description

When there are multiple Management servers, their IPs are listed as comma separated string in /var/cache/cloud/cmdline file against the "host" parameter, and when setting up the SSVM, SSVM fails to validate the IP of the management servers:

WARN  [storage.resource.NfsSecondaryStorageResource] (main:null) destIp is not a valid ip address or cidr destIp=x.x.x.x,y.y.y.y

In order to fix this, the IP addresses are split on the delimiter - ',' comma and then the further operations are performed.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

BLOCKER
Critical
Major
Minor
Trivial

How Has This Been Tested?

Deployed a setup with 2 management servers, and validated the fix. No longer fails validation of IP address

@blueorangutan package

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

Packaging result: ✖centos7 ✖centos8 ✖debian. JID-2446

…ervers

@blueorangutan package

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

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2447

@blueorangutan test

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

Though I see the need of this fix, i am a bit wary of it. It must be implemented somewhere for the kvm agent as well, so why is that code not called here. It seems like we are doubling/overlooking something.

@Pearl1594 why not change line 2592 to below ?

                String mgmtHosts = (String)params.get("host");
                for (final String mgmtHost : mgmtHosts.split(",")) {
                    addRouteToInternalIpOrCidr(_localgw, _eth1ip, _eth1mask, mgmtHost);
                }

Trillian test result (tid-3288)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32996 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4507-t3288-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

code LGTM

code changes LGTM, same behavior exists in ConsoleProxyResource.

Though I see the need of this fix, i am a bit wary of it. It must be implemented somewhere for the kvm agent as well, so why is that code not called here. It seems like we are doubling/overlooking something.

This situation does not arise in case of KVM agent @DaanHoogland

@blueorangutan package

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

Packaging result: ✔centos7 ✖centos8 ✔debian. JID-2452

@blueorangutan test

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

Though I see the need of this fix, i am a bit wary of it. It must be implemented somewhere for the kvm agent as well, so why is that code not called here. It seems like we are doubling/overlooking something.

This situation does not arise in case of KVM agent @DaanHoogland

I think I wasn't clear , @Pearl1594 . The kvm agent must have code to handle the case of multiple ip addresses for the MS, why isn't the same code used for both? The ip selection process during connecting is the same for all agent so the code should be the same bit as well.

Trillian test result (tid-3299)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 33607 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4507-t3299-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_internal_lb.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Smoke tests completed. 83 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

@DaanHoogland wrt KVM agent, the workflow followed is a bit different:
https://github.com/apache/cloudstack/blob/master/agent/src/main/java/com/cloud/agent/Agent.java#L194
https://github.com/apache/cloudstack/blob/master/agent/src/main/java/com/cloud/agent/AgentShell.java#L152
And here, we correctly consider the possibility of having multiple MS and split them accordingly.

This piece of code aligns to how ConsoleProxyResource handles multiple MSs.

ok, I'll shut my loud mouth, @Pearl1594
@PaulAngus this is minor but meet merge criteria. At your discretion.

LGTM

* master: server: add conditions for custom offerings (apache#4540) vr: Ensuring dnsmasq.leases file is populated (apache#4529) template: Ensuring template is cross zone if type changed to system (apache#4522) storage: Fix hypervisor type cast to string (apache#4516) db upgrade: fix sql exception: Access denied; you need (at least one of) the SUPER privilege(s) for this operation (apache#4533) CLOUDSTACK-10423:Potential sensitive information disclosure (apache#4536) jobs: The patch remove the password from resultObject and make it be humanreadable (apache#4538) listphysicalnetworks: Honouring keyword parameter (apache#4511) Fix NPE when Volume exists on secondary store but doesn't have a download URL (apache#4530) apidoc issue (apache#4532) db: Fix description of volume.stats.interval which is in milliseconds not seconds (apache#4526) kvm: set cpu topology only if cpucore per socket is positive value (apache#4527) xenserver: check and eject patch vbd for systemvms (apache#4525) Fix warning when setup cloudstack-common (apache#4523) kvm: FIX cpucorespersocket is not working on KVM (apache#4497) change debug to warn for unknown exceptions (apache#4521) Fix failure in validating IP address in case of multiple Management Servers (apache#4507) Update log output for FirstFitPlanner (apache#4515) ui: deprecate old UI and move to legacy to be served at /client/legacy (apache#4518)

Pearl1594 added this to the 4.14.1.0 milestone Nov 30, 2020

Fix failure in validating IP address in case of multiple Management S…

dad3f70

…ervers

Pearl1594 force-pushed the sec-stg-multiple-ms branch from 5d8ff91 to dad3f70 Compare November 30, 2020 10:15

yadvr requested review from DaanHoogland and PaulAngus November 30, 2020 10:52

refactor code

5b165bc

weizhouapache approved these changes Dec 1, 2020

View reviewed changes

sureshanaparti approved these changes Dec 1, 2020

View reviewed changes

shwstppr approved these changes Dec 8, 2020

View reviewed changes

yadvr approved these changes Dec 8, 2020

View reviewed changes

yadvr merged commit 8373724 into apache:4.14 Dec 8, 2020

Fix failure in validating IP address in case of multiple Management Servers by Pearl1594 · Pull Request #4507 · apache/cloudstack · GitHub

Repository navigation

Fix failure in validating IP address in case of multiple Management Servers - #4507

Fix failure in validating IP address in case of multiple Management Servers#4507yadvr merged 2 commits intoapache:4.14apache/cloudstack:4.14from shapeblue:sec-stg-multiple-msshapeblue/cloudstack:sec-stg-multiple-msCopy head branch name to clipboard

Conversation

Pearl1594 commented Nov 30, 2020

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

How Has This Been Tested?

Uh oh!

Pearl1594 commented Nov 30, 2020

Uh oh!

blueorangutan commented Nov 30, 2020

Uh oh!

blueorangutan commented Nov 30, 2020

Uh oh!

Pearl1594 commented Nov 30, 2020

Uh oh!

blueorangutan commented Nov 30, 2020

Uh oh!

blueorangutan commented Nov 30, 2020

Uh oh!

Pearl1594 commented Nov 30, 2020

Uh oh!

blueorangutan commented Nov 30, 2020

Uh oh!

DaanHoogland commented Nov 30, 2020

Uh oh!

weizhouapache commented Nov 30, 2020

Uh oh!

blueorangutan commented Nov 30, 2020

Uh oh!

weizhouapache left a comment

Choose a reason for hiding this comment

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

Pearl1594 commented Dec 1, 2020

Uh oh!

Pearl1594 commented Dec 1, 2020

Uh oh!

blueorangutan commented Dec 1, 2020

Uh oh!

blueorangutan commented Dec 1, 2020

Uh oh!

Pearl1594 commented Dec 2, 2020

Uh oh!

blueorangutan commented Dec 2, 2020

Uh oh!

DaanHoogland commented Dec 2, 2020

Uh oh!

blueorangutan commented Dec 2, 2020

Uh oh!

Pearl1594 commented Dec 3, 2020

Uh oh!

DaanHoogland commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shwstppr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Fix failure in validating IP address in case of multiple Management Servers#4507
yadvr merged 2 commits into
apache:4.14from
shapeblue:sec-stg-multiple-ms

DaanHoogland commented Dec 3, 2020 •
edited
Loading