Skip to content

Restart VPC with clean-up not applying all LB rules #8765

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

Closed
SadiJr wants to merge 1 commit into from
Closed

Restart VPC with clean-up not applying all LB rules #8765

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 8 additions & 2 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 @@ -2498,6 +2498,7 @@ protected void finalizeNetworkRulesForNetwork(final Commands cmds, final DomainR
if (!ipv6firewallRules.isEmpty()) {
_commandSetupHelper.createIpv6FirewallRulesCommands(ipv6firewallRules, router, cmds, guestNetworkId);
}
Network guestNetwork = _networkDao.findById(guestNetworkId);

if (publicIps != null && !publicIps.isEmpty()) {
final List<RemoteAccessVpn> vpns = new ArrayList<RemoteAccessVpn>();
Expand Down Expand Up @@ -2578,7 +2579,13 @@ protected void finalizeNetworkRulesForNetwork(final Commands cmds, final DomainR
}
}

final List<LoadBalancerVO> lbs = _loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
List<LoadBalancerVO> lbs = null;
Long vpcId = guestNetwork.getVpcId();
if (vpcId != null) {
lbs = _loadBalancerDao.listByVpcIdAndScheme(vpcId, Scheme.Public);
Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Mar 8, 2024

Choose a reason for hiding this comment

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

LB rules will be applied multiple times (=number of vpc tiers). There might be exceptions as some guest nics are missing.

It can be applied when all guest nics are added to the VPC VR

Copy link
Copy Markdown
Contributor Author

@SadiJr SadiJr Mar 12, 2024

Choose a reason for hiding this comment

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

I'm not sure if I understood exactly what you meant; since LB rules are being searched for by VPC ID, exceptions should not occur if there are missing NICs. In the worst case, there will be a rule added to VR of one network that does not exists.

Regarding applying the rules when all guest networks are added to VR, from what I saw of the current ACS workflow, the solution I presented corrects the problem, and I didn't find another part of the code where it can be solved besides this section. If you think that this solution could be applied to another section, could you please point to where that would be?

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Mar 12, 2024

Choose a reason for hiding this comment

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

@SadiJr
the method finalizeNetworkRulesForNetwork is called for each guest network when VPC VR is started

cloudstack/server/src/main/java/com/cloud/network/router/VpcVirtualNetworkApplianceManagerImpl.java

Lines 507 to 523 in 706f3f6

for (final Pair<Nic, Network> nicNtwk : guestNics) {
final Nic guestNic = nicNtwk.first();
final long guestNetworkId = guestNic.getNetworkId();
final AggregationControlCommand startCmd = new AggregationControlCommand(Action.Start, domainRouterVO.getInstanceName(), controlNic.getIPv4Address(), _routerControlHelper.getRouterIpInNetwork(
guestNetworkId, domainRouterVO.getId()));
cmds.addCommand(startCmd);
if (reprogramGuestNtwks) {
finalizeIpAssocForNetwork(cmds, domainRouterVO, provider, guestNetworkId, vlanMacAddress);
finalizeNetworkRulesForNetwork(cmds, domainRouterVO, provider, guestNetworkId);
finalizeMonitorService(cmds, profile, domainRouterVO, provider, guestNetworkId, true, routerHealthCheckConfig);
}
finalizeUserDataAndDhcpOnStart(cmds, domainRouterVO, provider, guestNetworkId);
final AggregationControlCommand finishCmd = new AggregationControlCommand(Action.Finish, domainRouterVO.getInstanceName(), controlNic.getIPv4Address(), _routerControlHelper.getRouterIpInNetwork(
guestNetworkId, domainRouterVO.getId()));
cmds.addCommand(finishCmd);
}

If you search for StartCommand in management server logs, you will see multiple LoadBalancerConfigCommand with same parameters.

Copy link
Copy Markdown
Contributor Author

@SadiJr SadiJr Mar 18, 2024

Choose a reason for hiding this comment

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

Correct, the method is indeed called for each guest network, but the LoadBalancerConfigCommand command, from what I've analyzed in the logs, is not the same. Instead, for each guest network that calls such a method, only the load balancer rules for that network are passed in the LoadBalancerConfigCommand command. This is the reason why, in a VPC with multiple guest networks, each of which has its own load balancer rules, only the rules of the last implemented network are applied.

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Mar 18, 2024

Choose a reason for hiding this comment

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

I know what caused the issue and why you made the changes.
What I meant is, the process can be optimized. Only one LoadBalancerConfigCommand (with LB of all tiers) is needed, maybe after all other rules are applied.

Copy link
Copy Markdown
Contributor Author

@SadiJr SadiJr Mar 20, 2024

Choose a reason for hiding this comment

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

I can understand your point of view, however, considering the criticality and the time it would take me to implement and test your proposal, I'll continue with the current implementation and create an issue to optimize such a process in the future, using your idea. What do you think?

Copy link
Copy Markdown
Member

@weizhouapache weizhouapache Mar 20, 2024

Choose a reason for hiding this comment

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

@SadiJr
if you do not mind, I can create a new PR for it tomorrow

Copy link
Copy Markdown
Contributor Author

@SadiJr SadiJr Mar 27, 2024

Choose a reason for hiding this comment

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

Sure, feel free to create an alternative PR, as it allows the community to compare the solutions and choose the best one.

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Apr 4, 2024

Choose a reason for hiding this comment

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

@SadiJr if you do not mind, I can create a new PR for it tomorrow

@weizhouapache , would this be an alternative or an addition?

Have you created it?

} else {
lbs = _loadBalancerDao.listByNetworkIdAndScheme(guestNetworkId, Scheme.Public);
}
final List<LoadBalancingRule> lbRules = new ArrayList<LoadBalancingRule>();
if (_networkModel.isProviderSupportServiceInNetwork(guestNetworkId, Service.Lb, provider)) {
// Re-apply load balancing rules
Expand All @@ -2599,7 +2606,6 @@ protected void finalizeNetworkRulesForNetwork(final Commands cmds, final DomainR
}
}
// Reapply dhcp and dns configuration.
final Network guestNetwork = _networkDao.findById(guestNetworkId);
if (guestNetwork.getGuestType() == GuestType.Shared && _networkModel.isProviderSupportServiceInNetwork(guestNetworkId, Service.Dhcp, provider)) {
final Map<Network.Capability, String> dhcpCapabilities = _networkSvc.getNetworkOfferingServiceCapabilities(
_networkOfferingDao.findById(_networkDao.findById(guestNetworkId).getNetworkOfferingId()), Service.Dhcp);
Expand Down