make control.cidr and control.gateway dynamic by DaanHoogland · Pull Request #7746 · apache/cloudstack

DaanHoogland · 2023-07-18T06:12:27Z

Description

This PR attempts to make choosing local ip choices adhere to a more dynamics control.cidr so people can actually choose their control network....

Fixes: #6715

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

Screenshots (if appropriate):

How Has This Been Tested?

codecov · 2023-07-18T07:10:11Z

Codecov Report

Merging #7746 (3dd5858) into 4.18 (439d70f) will increase coverage by 0.00%.
The diff coverage is 22.72%.

@@            Coverage Diff            @@
##               4.18    #7746   +/-   ##
=========================================
  Coverage     13.06%   13.06%           
  Complexity     9093     9093           
=========================================
  Files          2720     2721    +1     
  Lines        257431   257395   -36     
  Branches      40141    40132    -9     
=========================================
- Hits          33622    33620    -2     
+ Misses       219582   219549   -33     
+ Partials       4227     4226    -1

Files Changed	Coverage Δ
...a/com/cloud/network/lb/LoadBalanceRuleHandler.java	`12.25% <0.00%> (ø)`
.../network/lb/InternalLoadBalancerVMManagerImpl.java	`25.23% <0.00%> (ø)`
.../src/main/java/com/cloud/configuration/Config.java	`88.86% <ø> (-0.06%)`	⬇️
...om/cloud/consoleproxy/ConsoleProxyManagerImpl.java	`7.30% <0.00%> (ø)`
...om/cloud/deploy/DeploymentPlanningManagerImpl.java	`28.92% <0.00%> (ø)`
...ava/com/cloud/network/guru/ControlNetworkGuru.java	`5.19% <0.00%> (+0.37%)`	⬆️
...java/com/cloud/server/ConfigurationServerImpl.java	`2.38% <ø> (+0.09%)`	⬆️
...ain/java/com/cloud/storage/StorageManagerImpl.java	`1.59% <0.00%> (ø)`
.../secondarystorage/SecondaryStorageManagerImpl.java	`6.46% <0.00%> (ø)`
.../cloud/configuration/ConfigurationManagerImpl.java	`15.44% <50.00%> (-0.03%)`	⬇️
... and 2 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DaanHoogland · 2023-07-20T11:27:37Z

@blueorangutan package

blueorangutan · 2023-07-20T11:28:03Z

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-07-20T12:20:15Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6522

GutoVeronezi · 2023-07-20T12:27:00Z

@DaanHoogland, there are some refactorings being done that are not related to the PR proposal; would not be better to create a new PR for the refactoring and separate the contexts?

DaanHoogland · 2023-07-20T13:00:10Z

@DaanHoogland, there are some refactorings being done that are not related to the PR proposal; would not be better to create a new PR for the refactoring and separate the contexts?

I could do @GutoVeronezi , but I find that I do these refactoring for undestanding the software and the problem. Thus the changes are usually intertwined a bit. I gather you ask this because it makes reviewing harder?

(btw, i still have to think of a way to verify this change)

GutoVeronezi · 2023-07-20T13:25:34Z

@DaanHoogland, there are some refactorings being done that are not related to the PR proposal; would not be better to create a new PR for the refactoring and separate the contexts?

I could do @GutoVeronezi , but I find that I do these refactoring for undestanding the software and the problem. Thus the changes are usually intertwined a bit. I gather you ask this because it makes reviewing harder?

(btw, i still have to think of a way to verify this change)

In this scale of changes (~250 lines), the reviewing still is quite simple. Personally, I like to separate the contexts when possible, to better organize and track the changes. It is just a suggestion, though. 😁

harikrishna-patnala

Code LGTM

Rubueno · 2023-07-20T21:43:02Z

Thanks for spending time to work on this Daan! LGTM

DaanHoogland · 2023-08-14T14:19:14Z

@blueorangutan package

blueorangutan · 2023-08-14T14:20:03Z

@DaanHoogland a [SF] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2023-08-14T15:30:51Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6773

DaanHoogland · 2023-08-15T06:35:25Z

@blueorangutan test

blueorangutan · 2023-08-15T06:36:03Z

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

blueorangutan · 2023-08-15T23:42:49Z

[SF] Trillian test result (tid-7409)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 60109 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7746-t7409-kvm-centos7.zip
Smoke tests completed. 106 look OK, 2 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
test_01_migrate_VM_and_root_volume	`Error`	81.80	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	57.55	test_vm_life_cycle.py

blueorangutan · 2023-08-16T13:25:14Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 6801

DaanHoogland · 2023-08-17T09:39:25Z

@Rubueno @wido I gave this another look and it is not going to work. The link local addresses are created on createPod and changing the configurations will break any SVM created after changing the settings. We'll need a more extensive disign. Can you give some input on what a valid scenario for you would be?

thanks

wido · 2023-08-18T08:21:00Z

@Rubueno @wido I gave this another look and it is not going to work. The link local addresses are created on createPod and changing the configurations will break any SVM created after changing the settings. We'll need a more extensive disign. Can you give some input on what a valid scenario for you would be?

thanks

We should move to IPv6 Link-Local at all for the SSVM :-) These addresses with IPv4 aren't needed at all ;-) Makes life much easier, I promise!

Anyway, this isn't the biggest problem now.

In RFC5549 the IPv4 address 169.254.0.1 is used for routing IPv4 traffic via IPv6, something which you will use when using EVPN+BGP+VXLAN.

Therefor you want to change the control CIDR to something else. It's 'stupid' that we allocate a /16 to this as that's not needed at all, a /20 by default should be more then enough.

Any existing installations work just fine. Maybe we should just change the default from this /16 to a /20 (169.254.240.0/20) and be done with it for now.

weizhouapache · 2023-08-18T08:36:25Z

@Rubueno @wido I gave this another look and it is not going to work. The link local addresses are created on createPod and changing the configurations will break any SVM created after changing the settings. We'll need a more extensive disign. Can you give some input on what a valid scenario for you would be?
thanks

We should move to IPv6 Link-Local at all for the SSVM :-) These addresses with IPv4 aren't needed at all ;-) Makes life much easier, I promise!

Anyway, this isn't the biggest problem now.

In RFC5549 the IPv4 address 169.254.0.1 is used for routing IPv4 traffic via IPv6, something which you will use when using EVPN+BGP+VXLAN.

Therefor you want to change the control CIDR to something else. It's 'stupid' that we allocate a /16 to this as that's not needed at all, a /20 by default should be more then enough.

Any existing installations work just fine. Maybe we should just change the default from this /16 to a /20 (169.254.240.0/20) and be done with it for now.

thanks @wido

@DaanHoogland I think the main problem for this PR is, all link local IPs are saved in database table op_dc_link_local_ip_address_alloc which is not needed at all.
We need to save only the allocated IPs in the table and remove all unused IPs.
Some classes need to be updated to support the IP allocation from cidr/gateway, instead of picking up IPs from op_dc_link_local_ip_address_alloc table.

wido · 2023-08-20T19:58:35Z

@Rubueno @wido I gave this another look and it is not going to work. The link local addresses are created on createPod and changing the configurations will break any SVM created after changing the settings. We'll need a more extensive disign. Can you give some input on what a valid scenario for you would be?
thanks

We should move to IPv6 Link-Local at all for the SSVM :-) These addresses with IPv4 aren't needed at all ;-) Makes life much easier, I promise!
Anyway, this isn't the biggest problem now.
In RFC5549 the IPv4 address 169.254.0.1 is used for routing IPv4 traffic via IPv6, something which you will use when using EVPN+BGP+VXLAN.
Therefor you want to change the control CIDR to something else. It's 'stupid' that we allocate a /16 to this as that's not needed at all, a /20 by default should be more then enough.
Any existing installations work just fine. Maybe we should just change the default from this /16 to a /20 (169.254.240.0/20) and be done with it for now.

thanks @wido

@DaanHoogland I think the main problem for this PR is, all link local IPs are saved in database table op_dc_link_local_ip_address_alloc which is not needed at all. We need to save only the allocated IPs in the table and remove all unused IPs. Some classes need to be updated to support the IP allocation from cidr/gateway, instead of picking up IPs from op_dc_link_local_ip_address_alloc table.

And in addition I think we should opt for a /20 by default for all new installations. If we do that, this whole problem is kind of solved. For new deployments the /20 would be used and never get into the way of BGP with v4 via v6 routing.

DaanHoogland · 2023-08-29T08:21:47Z

+            String.class,
+            "control.cidr",
+            "Advanced",
+            "169.254.0.0/16",


Suggested change

"169.254.0.0/16",

"169.254.240.0/20",

like this and abandon all other changes in this PR, right @wido ?

Almost @DaanHoogland , but also in:

NetUtils.java

AgentProperties.java

configuration/Config.java

If we change it to a /20 in those files we should be good.

This will only work for new environments. I don't think this will work when upgrading environment. These hardcoded values of an already otherwise deplayed config item will break systems and needs a bit more TLC .

@DaanHoogland
as I understand, Wido wants /20 to be the default in new environments.
It would be good to update the config description to emphasize that the new value only applies on new pods.

Yes. For upgrades we shouldn't touch anything. Unless somebody is going to use BGP Unnumbered in their existing environment. Otherwise just using the /20 for new deployments is fine.

well, this means that the hardcoded values in NetUtils.java and AgentProperties.java have to be intelligent... we can not assume returning the one or the other in these methods. That complicates things a bit.

DaanHoogland · 2023-10-02T13:51:03Z

i disagree with this PR myself

DaanHoogland added this to the 4.18.1.0 milestone Jul 18, 2023

boring-cyborg Bot added component:management-server component:networking component:secondary-storage component:ssvm component:storage labels Jul 18, 2023

apache deleted a comment from blueorangutan Jul 19, 2023

DaanHoogland mentioned this pull request Jul 19, 2023

kvm agent: get the right controll cidr #7580

Merged

3 of 12 tasks

harikrishna-patnala reviewed Jul 20, 2023

View reviewed changes

Comment thread engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java

Comment thread server/src/main/java/com/cloud/network/guru/ControlNetworkGuru.java Outdated

apache deleted a comment from blueorangutan Jul 20, 2023

harikrishna-patnala approved these changes Jul 20, 2023

View reviewed changes

weizhouapache reviewed Aug 14, 2023

View reviewed changes

Comment thread engine/components-api/src/main/java/com/cloud/configuration/ConfigurationManager.java Outdated

weizhouapache marked this pull request as draft August 15, 2023 08:03

weizhouapache changed the title ~~make control.cidr dynamic~~ make control.cidr and control.gateway dynamic Aug 15, 2023

DaanHoogland force-pushed the localAddressInControlCidr branch from 371f7c3 to 8490193 Compare August 16, 2023 11:18

DaanHoogland modified the milestones: 4.18.1.0, 4.18.2.0 Aug 17, 2023

weizhouapache requested a review from wido August 17, 2023 11:28

DaanHoogland added 7 commits August 28, 2023 15:32

make control.cidr dynamic

e83c568

merge mistake fixed

3d390b1

some open ends

8cd48ac

double var

34dabe4

dynamic gateway value (per zone)

a7b9fbe

import

efd2cd9

control cidr/gateway not dynamoic

3dd5858

DaanHoogland force-pushed the localAddressInControlCidr branch from 7b92f4c to 3dd5858 Compare August 28, 2023 13:32

DaanHoogland commented Aug 29, 2023

View reviewed changes

DaanHoogland closed this Oct 2, 2023

DaanHoogland removed this from the 4.18.2.0 milestone Nov 1, 2023

Conversation

DaanHoogland commented Jul 18, 2023

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

codecov Bot commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Jul 20, 2023

Uh oh!

blueorangutan commented Jul 20, 2023

Uh oh!

blueorangutan commented Jul 20, 2023

Uh oh!

GutoVeronezi commented Jul 20, 2023

Uh oh!

DaanHoogland commented Jul 20, 2023

Uh oh!

GutoVeronezi commented Jul 20, 2023

Uh oh!

harikrishna-patnala left a comment

Choose a reason for hiding this comment

Uh oh!

Rubueno commented Jul 20, 2023

Uh oh!

Uh oh!

DaanHoogland commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

blueorangutan commented Aug 14, 2023

Uh oh!

DaanHoogland commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 15, 2023

Uh oh!

blueorangutan commented Aug 16, 2023

Uh oh!

DaanHoogland commented Aug 17, 2023

Uh oh!

wido commented Aug 18, 2023

Uh oh!

weizhouapache commented Aug 18, 2023

Uh oh!

wido commented Aug 20, 2023

Uh oh!

DaanHoogland Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

wido Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

weizhouapache Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

wido Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

DaanHoogland Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Oct 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov Bot commented Jul 18, 2023 •

edited

Loading