server: add global configuration for default router service offering by ustcweizhou · Pull Request #3946 · apache/cloudstack

ustcweizhou · 2020-03-05T23:07:06Z

Description

This adds a global setting router.service.offering, which can be uuid of a system offering.
the service offering will be used by virtual routers;
if it is not set, default system offering will be used.

This is a account-scope setting, so it can be set in Account setting as well.

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?

create a system offering
change global setting
restart network with cleanup
new VRs will be created with new offering

svenvogel · 2020-03-06T10:27:10Z

@ustcweizhou nice feature.

harikrishna-patnala · 2020-03-06T10:40:56Z

We already have a provision to mention the service offering id while creating network offering which serves the same purpose right!

weizhouapache · 2020-03-06T11:06:11Z

We already have a provision to mention the service offering id while creating network offering which serves the same purpose right!

@harikrishna-patnala it is an option, but it is complicated to change the offering of a VR :
(1)create network offering with router offering id
(2) update network to new network offering
Suppose there are many networks with different network offerings, we need to create some new offerings. There is some downtime in step (2).

It is very simple with this PR. (1) change setting, (2) restart network with cleanup.

harikrishna-patnala · 2020-03-06T12:20:30Z

I'm not against the new option, but wanted to know the main intention of adding this.
Code changes LGTM.

DaanHoogland · 2020-03-06T12:56:24Z

@blueorangutan package

blueorangutan · 2020-03-06T12:57:14Z

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

blueorangutan · 2020-03-06T13:32:07Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1023

DaanHoogland · 2020-03-07T08:35:17Z

I need a test volunteer, other than
@blueorangutan test

blueorangutan · 2020-03-07T08:36:17Z

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

blueorangutan · 2020-03-07T19:23:57Z

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

Test	Result	Time (s)	Test File

harikrishna-patnala · 2020-03-12T06:10:14Z

I need a test volunteer, other than
@blueorangutan test

I'll test this and post the results

blueorangutan · 2020-03-12T06:10:39Z

@harikrishna-patnala unsupported parameters provided. Supported mgmt server os are: centos6, centos7, ubuntu. Supported hypervisors are: kvm-centos6, kvm-centos7, kvm-ubuntu, xenserver-71, xenserver-65sp1, xenserver-62sp1, vmware-67u3, vmware-65u2, vmware-60u2, vmware-55u3, vmware-51u1, vmware-50u1

blueorangutan · 2020-03-12T07:21:25Z

Packaging result: ✖centos6 ✔centos7 ✔debian. JID-1042

weizhouapache · 2020-03-12T08:09:11Z

I need a test volunteer, other than
@blueorangutan test

I'll test this and post the results

@harikrishna-patnala gooood. if you need my assistance, let me know.

harikrishna-patnala · 2020-03-12T09:59:56Z

Following are the test cases covered for both isolated and VPC networks.

Created a network with 'router.service.offering' set to empty - VR deployed with 'System Offering For Software Router' - PASSED
Created a network with global configuration parameter 'router.service.offering' set to UUID of 'VRcustomCO1' compute offering - VR deployed with 'VRcustomCO1' - PASSED
Created a network with account level configuration parameter 'router.service.offering' set to UUID of 'VRcustomCO2' compute offering - VR deployed with 'VRcustomCO2' - PASSED
Created a network with account level configuration parameter 'router.service.offering' set to an invalid UUID - VR deployed with 'System Offering For Software Router'. Here in this case global configuration parameter is set to 'VRcustomCO1'.

In test case 4, irrespective of global config parameter VR deployed with default SO. If this behavior is expected a log message would help debugging.

weizhouapache · 2020-03-12T13:24:29Z

Following are the test cases covered for both isolated and VPC networks.

Created a network with 'router.service.offering' set to empty - VR deployed with 'System Offering For Software Router' - PASSED

Created a network with global configuration parameter 'router.service.offering' set to UUID of 'VRcustomCO1' compute offering - VR deployed with 'VRcustomCO1' - PASSED

Created a network with account level configuration parameter 'router.service.offering' set to UUID of 'VRcustomCO2' compute offering - VR deployed with 'VRcustomCO2' - PASSED

Created a network with account level configuration parameter 'router.service.offering' set to an invalid UUID - VR deployed with 'System Offering For Software Router'. Here in this case global configuration parameter is set to 'VRcustomCO1'.

In test case 4, irrespective of global config parameter VR deployed with default SO. If this behavior is expected a log message would help debugging.

@harikrishna-patnala thanks for testing. I think it make more sense to use 'VRcustomCO1' instead of 'System Offering For Software Router' in case 4. I will update the PR.

harikrishna-patnala · 2020-03-13T03:46:36Z

+        if (serviceOffering != null && serviceOffering.isSystemUse()) {
+            boolean isLocalStorage = ConfigurationManagerImpl.SystemVMUseLocalStorage.valueIn(dest.getDataCenter().getId());
+            if (isLocalStorage == serviceOffering.isUseLocalStorage()) {
+                logger.debug("service offering " + serviceOffering.getId() + " will be used on virtual router");


offeringUuid would be better than id in the log message. with that LGTM

weizhouapache · 2020-03-13T16:27:33Z

updated based on @harikrishna-patnala 's comment

DaanHoogland · 2020-03-13T16:39:31Z

+        if (accountRouterOffering != null) {
+            verifyServiceOfferingByUuid(accountRouterOffering);
+        }
+        if (serviceOfferingId == null && globalRouterOffering != accountRouterOffering) {


Wouldn't we want globalRouterOffering to take precedence over serviceOfferingId, @weizhouapache?

@DaanHoogland serviceOfferingId is the final offering which will be used on virtual router.

yes, but if we configured a global router offering wouldn't we prefer that even if serviceOfferingId is not null?

@DaanHoogland correct. it checks following offerings

Router service Offering of network/vpc offering

Account route service offering

Global route service offering

Default route offering

If 1 or 2 is set and validated, global setting will not be used.

@weizhouapache if (serviceOffering != null), which should be always, globalRouterOffering will not be checked, whether or not globalRouterOffering == accountRouterOffering.
so I don't think your 3 and 4 are correct but this condition will never be met and verifyServiceOfferingByUuid(globalRouterOffering) will never be executed.
So, why check for serviceOfferingId before globalRouterOffering when it is the last resort and is only relevant after globalRouterOffering?

@weizhouapache if (serviceOffering != null), which should be always, globalRouterOffering will not be checked, whether or not globalRouterOffering == accountRouterOffering.
so I don't think your 3 and 4 are correct but this condition will never be met and verifyServiceOfferingByUuid(globalRouterOffering) will never be executed.
So, why check for serviceOfferingId before globalRouterOffering when it is the last resort and is only relevant after globalRouterOffering?

@DaanHoogland it is serviceOfferingId , not serviceOffering.

cloudstack/server/src/main/java/org/cloud/network/router/deployment/RouterDeploymentDefinition.java

Line 109 in 3783a09

protected Long serviceOfferingId;

the check procedure

cloudstack/server/src/main/java/org/cloud/network/router/deployment/RouterDeploymentDefinition.java

Lines 416 to 424 in 3783a09

protected void findServiceOfferingId() {

serviceOfferingId = networkOfferingDao.findById(guestNetwork.getNetworkOfferingId()).getServiceOfferingId();

if (serviceOfferingId == null) {

findAccountServiceOfferingId(guestNetwork.getAccountId());

}

if (serviceOfferingId == null) {

findDefaultServiceOfferingId();

}

}

in the account part, considering the uuid might be invalid, so check account setting at first, then global setting if account setting is invalid. (if account setting is not set, accountRouterOffering is actually the global setting, so skip the check if accountRouterOffering equals to globalRouterOffering )

cloudstack/server/src/main/java/org/cloud/network/router/deployment/RouterDeploymentDefinition.java

Lines 393 to 414 in 3783a09

protected void findAccountServiceOfferingId(long accountId) {

String accountRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.valueIn(accountId);

String globalRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.value();

if (accountRouterOffering != null) {

verifyServiceOfferingByUuid(accountRouterOffering);

}

if (serviceOfferingId == null && globalRouterOffering != accountRouterOffering) {

verifyServiceOfferingByUuid(globalRouterOffering);

}

}

private void verifyServiceOfferingByUuid(String offeringUuid) {

logger.debug("Verifying router service offering with uuid : " + offeringUuid);

ServiceOfferingVO serviceOffering = serviceOfferingDao.findByUuid(offeringUuid);

if (serviceOffering != null && serviceOffering.isSystemUse()) {

boolean isLocalStorage = ConfigurationManagerImpl.SystemVMUseLocalStorage.valueIn(dest.getDataCenter().getId());

if (isLocalStorage == serviceOffering.isUseLocalStorage()) {

logger.debug(String.format("Service offering %s (uuid: %s) will be used on virtual router", serviceOffering.getName(), serviceOffering.getUuid()));

serviceOfferingId = serviceOffering.getId();

}

}

}

hope it helps you understanding all changes in PR.

ah, i missed the side effect of verifyServiceOfferingByUuid(), where the id is set, thanks.

Maybe to make this more obvious for stupid people like me we can make verifyServiceOfferingByUuid() return a uuid or null and set it in the findAccountServiceOfferingId()?

…uter-offering

shwstppr · 2020-09-11T08:49:53Z

@blueorangutan package

blueorangutan · 2020-09-11T08:50:41Z

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

blueorangutan · 2020-09-11T09:10:42Z

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

blueorangutan · 2020-09-11T09:41:46Z

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

weizhouapache · 2020-09-11T11:49:01Z

@blueorangutan package

blueorangutan · 2020-09-11T11:49:52Z

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

blueorangutan · 2020-09-11T12:30:09Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-1965

DaanHoogland · 2020-09-18T15:09:08Z

@blueorangutan test

blueorangutan · 2020-09-18T15:10:31Z

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

blueorangutan · 2020-09-19T04:57:16Z

Trillian test result (tid-2787)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47655 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3946-t2787-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_supported_versions.py
Intermittent failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 82 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_add_delete_kubernetes_supported_version	`Error`	1807.64	test_kubernetes_supported_versions.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Failure`	372.03	test_vpc_redundant.py
test_hostha_kvm_host_fencing	`Error`	101.27	test_hostha_kvm.py

DaanHoogland · 2020-09-21T10:31:41Z

@shwstppr do you 👍 this, based on your tests?

shwstppr

LGTM. Tested in a local env.

Adding new compute offering and setting it in new global setting results in VR using default offering
Adding new system service offering for routers and setting it in new global setting results in VR using new offering
Setting global setting empty results in VR using default offering as expected

Test failures don't seem related. Will be great if we can have corresponding doc PR for user documentation if not there already @weizhouapache

weizhouapache · 2020-09-21T19:13:07Z

LGTM. Tested in a local env.

Adding new compute offering and setting it in new global setting results in VR using default offering

Adding new system service offering for routers and setting it in new global setting results in VR using new offering

Setting global setting empty results in VR using default offering as expected

Test failures don't seem related. Will be great if we can have corresponding doc PR for user documentation if not there already @weizhouapache

@shwstppr thanks for testing. Will add doc if this is merged.

DaanHoogland · 2020-10-20T11:11:52Z

@weizhouapache is this still up to date?

weizhouapache · 2020-10-20T11:23:27Z

@weizhouapache is this still up to date?

@DaanHoogland yes.

DaanHoogland · 2020-10-21T08:31:04Z

@weizhouapache I see consistent smoke test errors for redundant VPCs since this weekend and this is the only related PR I have found so far. can you have a look e.g. at #4200 (comment)
@blueorangutan package

blueorangutan · 2020-10-21T08:31:58Z

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

…ffering (apache#3946)" This reverts commit 2661ce8.

blueorangutan · 2020-10-21T09:28:52Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2244

weizhouapache · 2020-10-21T10:11:17Z

@weizhouapache I see consistent smoke test errors for redundant VPCs since this weekend and this is the only related PR I have found so far. can you have a look e.g. at #4200 (comment)
@blueorangutan package

@DaanHoogland
if you search "test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL" in your mailbox, you may get many emails :-D
it is not caused by this pr.
as I said in #4386 (comment), it is caused by keepalived issue, which should be fixed by #4413

blueorangutan · 2020-10-21T10:11:59Z

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

DaanHoogland · 2020-10-21T10:19:20Z

thanks @weizhouapache , i saw it happening a lot suddenly overnight and got a little worried I had merged something overhastely, sorry, never mind.

blueorangutan · 2020-10-21T10:45:42Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2248

ustcweizhou added 2 commits March 5, 2020 21:46

server: add global configuration for default router service offering

3f5a989

server: use router offering on vpc vr

a39c3e6

svenvogel added component:virtual-router type:new-feature labels Mar 6, 2020

server: verify global setting if account setting is invalid

454f5aa

ustcweizhou force-pushed the 4.14-add-default-router-offering branch from 981c314 to 454f5aa Compare March 12, 2020 22:15

harikrishna-patnala reviewed Mar 13, 2020

View reviewed changes

server: change debug info

3783a09

DaanHoogland reviewed Mar 13, 2020

View reviewed changes

weizhouapache closed this Mar 13, 2020

weizhouapache reopened this Mar 13, 2020

Merge remote-tracking branch 'apache/master' into 4.14-add-default-ro…

576bd89

…uter-offering

DaanHoogland added this to the 4.15.0.0 milestone May 14, 2020

shwstppr approved these changes Sep 21, 2020

View reviewed changes

ustcweizhou mentioned this pull request Sep 21, 2020

Virtual Router Service Offering apache/cloudstack-documentation#157

Merged

DaanHoogland approved these changes Oct 20, 2020

View reviewed changes

DaanHoogland merged commit 2661ce8 into apache:master Oct 20, 2020

davidjumani added a commit to shapeblue/cloudstack that referenced this pull request Oct 21, 2020

Revert "server: add global configuration for default router service o…

606e98b

…ffering (apache#3946)" This reverts commit 2661ce8.

ACSGitBot added the type:new_feature label Nov 8, 2020

	protected void findServiceOfferingId() {
	serviceOfferingId = networkOfferingDao.findById(guestNetwork.getNetworkOfferingId()).getServiceOfferingId();
	if (serviceOfferingId == null) {
	findAccountServiceOfferingId(guestNetwork.getAccountId());
	}
	if (serviceOfferingId == null) {
	findDefaultServiceOfferingId();
	}
	}

	protected void findAccountServiceOfferingId(long accountId) {
	String accountRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.valueIn(accountId);
	String globalRouterOffering = VirtualNetworkApplianceManager.VirtualRouterServiceOffering.value();
	if (accountRouterOffering != null) {
	verifyServiceOfferingByUuid(accountRouterOffering);
	}
	if (serviceOfferingId == null && globalRouterOffering != accountRouterOffering) {
	verifyServiceOfferingByUuid(globalRouterOffering);
	}
	}

	private void verifyServiceOfferingByUuid(String offeringUuid) {
	logger.debug("Verifying router service offering with uuid : " + offeringUuid);
	ServiceOfferingVO serviceOffering = serviceOfferingDao.findByUuid(offeringUuid);
	if (serviceOffering != null && serviceOffering.isSystemUse()) {
	boolean isLocalStorage = ConfigurationManagerImpl.SystemVMUseLocalStorage.valueIn(dest.getDataCenter().getId());
	if (isLocalStorage == serviceOffering.isUseLocalStorage()) {
	logger.debug(String.format("Service offering %s (uuid: %s) will be used on virtual router", serviceOffering.getName(), serviceOffering.getUuid()));
	serviceOfferingId = serviceOffering.getId();
	}
	}
	}

Conversation

ustcweizhou commented Mar 5, 2020

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

svenvogel commented Mar 6, 2020

Uh oh!

harikrishna-patnala commented Mar 6, 2020

Uh oh!

weizhouapache commented Mar 6, 2020

Uh oh!

harikrishna-patnala commented Mar 6, 2020

Uh oh!

DaanHoogland commented Mar 6, 2020

Uh oh!

blueorangutan commented Mar 6, 2020

Uh oh!

blueorangutan commented Mar 6, 2020

Uh oh!

DaanHoogland commented Mar 7, 2020

Uh oh!

blueorangutan commented Mar 7, 2020

Uh oh!

blueorangutan commented Mar 7, 2020

Uh oh!

harikrishna-patnala commented Mar 12, 2020

Uh oh!

blueorangutan commented Mar 12, 2020

Uh oh!

blueorangutan commented Mar 12, 2020

Uh oh!

weizhouapache commented Mar 12, 2020

Uh oh!

harikrishna-patnala commented Mar 12, 2020

Uh oh!

weizhouapache commented Mar 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Mar 13, 2020

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!

shwstppr commented Sep 11, 2020

Uh oh!

blueorangutan commented Sep 11, 2020

Uh oh!

blueorangutan commented Sep 11, 2020

Uh oh!

blueorangutan commented Sep 11, 2020

Uh oh!

weizhouapache commented Sep 11, 2020

Uh oh!

blueorangutan commented Sep 11, 2020

Uh oh!

blueorangutan commented Sep 11, 2020

Uh oh!

DaanHoogland commented Sep 18, 2020

Uh oh!

blueorangutan commented Sep 18, 2020

Uh oh!

blueorangutan commented Sep 19, 2020

Uh oh!

DaanHoogland commented Sep 21, 2020

Uh oh!

shwstppr left a comment