Skip to content

server: add global configuration for default router service offering #3946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
DaanHoogland merged 5 commits into from
Oct 20, 2020
Merged

server: add global configuration for default router service offering #3946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3f5a989
server: add global configuration for default router service offering
ustcweizhou Feb 28, 2020
a39c3e6
server: use router offering on vpc vr
ustcweizhou Mar 5, 2020
454f5aa
server: verify global setting if account setting is invalid
ustcweizhou Mar 12, 2020
3783a09
server: change debug info
ustcweizhou Mar 13, 2020
576bd89
Merge remote-tracking branch 'apache/master' into 4.14-add-default-ro…
ustcweizhou May 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public interface VirtualNetworkApplianceManager extends Manager, VirtualNetworkA
static final String RouterTemplateOvm3CK = "router.template.ovm3";
static final String SetServiceMonitorCK = "network.router.EnableServiceMonitoring";
static final String RouterAlertsCheckIntervalCK = "router.alerts.check.interval";
static final String VirtualRouterServiceOfferingCK = "router.service.offering";

static final String RouterHealthChecksConfigRefreshIntervalCK = "router.health.checks.config.refresh.interval";
static final String RouterHealthChecksResultFetchIntervalCK = "router.health.checks.results.fetch.interval";
Expand Down Expand Up @@ -74,6 +75,9 @@ public interface VirtualNetworkApplianceManager extends Manager, VirtualNetworkA
static final ConfigKey<Boolean> ExposeDnsAndBootpServer = new ConfigKey<Boolean>(Boolean.class, "expose.dns.externally", "Advanced", "true",
"open dns, dhcp and bootp on the public interface", true, ConfigKey.Scope.Zone, null);

static final ConfigKey<String> VirtualRouterServiceOffering = new ConfigKey<String>(String.class, VirtualRouterServiceOfferingCK, "Advanced", "",
"Uuid of the service offering used by virtual routers; if NULL - system offering will be used", true, ConfigKey.Scope.Account, null);

// Health checks
static final ConfigKey<Boolean> RouterHealthChecksEnabled = new ConfigKey<Boolean>(Boolean.class, "router.health.checks.enabled", "Advanced", "true",
"If true, router health checks are allowed to be executed and read. If false, all scheduled checks and API calls for on demand checks are disabled.",
Expand Down
1 change: 1 addition & 0 deletions server/src/main/java/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3245,6 +3245,7 @@ public ConfigKey<?>[] getConfigKeys() {
UseExternalDnsServers,
RouterVersionCheckEnabled,
SetServiceMonitor,
VirtualRouterServiceOffering,
RouterAlertsCheckInterval,
RouterHealthChecksEnabled,
RouterHealthChecksBasicInterval,
Expand Down
29 changes: 28 additions & 1 deletion server/src/main/java/org/cloud/network/router/deployment/RouterDeploymentDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
import com.cloud.network.dao.UserIpv6AddressDao;
import com.cloud.network.dao.VirtualRouterProviderDao;
import com.cloud.network.router.NetworkHelper;
import com.cloud.network.router.VirtualNetworkApplianceManager;
import com.cloud.network.router.VirtualRouter.Role;
import com.cloud.network.vpc.Vpc;
import com.cloud.offering.ServiceOffering;
Expand Down Expand Up @@ -389,8 +390,34 @@ protected void findDefaultServiceOfferingId() {
serviceOfferingId = serviceOffering.getId();
}

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) {
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland correct. it checks following offerings

  1. Router service Offering of network/vpc offering
  2. Account route service offering
  3. Global route service offering
  4. Default route offering

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

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Mar 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()?

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();
}
}
}

protected void findServiceOfferingId() {
serviceOfferingId = networkOfferingDao.findById(guestNetwork.getNetworkOfferingId()).getServiceOfferingId();
if (serviceOfferingId == null) {
findAccountServiceOfferingId(guestNetwork.getAccountId());
}
if (serviceOfferingId == null) {
findDefaultServiceOfferingId();
}
Expand Down Expand Up @@ -482,4 +509,4 @@ protected boolean routersNeedReset() {

return needReset;
}
}
}
3 changes: 3 additions & 0 deletions server/src/main/java/org/cloud/network/router/deployment/VpcRouterDeploymentDefinition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,9 @@ protected void findVirtualProvider() {
@Override
protected void findServiceOfferingId() {
serviceOfferingId = vpcOffDao.findById(vpc.getVpcOfferingId()).getServiceOfferingId();
if (serviceOfferingId == null) {
findAccountServiceOfferingId(vpc.getAccountId());
}
if (serviceOfferingId == null) {
findDefaultServiceOfferingId();
}
Expand Down