[bhouse-nexthop]

Description

This PR stops the virtual router agent from dropping aggregated commands, and from failing with an NPE, when two jobs aggregate commands for the same router at the same time. Parallel tier deploys in one VPC do this.

VirtualRoutingResource keeps one aggregation queue per router. Before this change:

• A second AggregationControlCommand Start replaced the first job's queue, so the commands queued so far were never applied, although they had already been answered with success.
• The first Finish then applied and removed the second job's queue, and the second Finish failed with NullPointerException on Queue.clear().

Changes, all in VirtualRoutingResource:

• Start while a queue exists joins that queue instead of replacing it, and logs a warning. The first Finish applies the commands of both jobs.
• Finish with no queue no longer throws. The commands of that job were applied by the Finish that ran before it, or directly after that one. So it returns the result of the last aggregated apply on that router: success, or the failure details of that apply. That result is kept per router in a new map.
• Thread safety: the lock map and the aggregation maps are Collections.synchronizedMap, so null router names behave as before. The per-router lock is created with computeIfAbsent. Before, two threads could create two different locks for one router. The aggregation maps are still only changed for a router while holding that router's lock.
• The assert on Start that no queue exists is removed, since that case is now handled.

A single aggregation, with no overlap, behaves exactly as before.

Limits of this change:

• Commands of the two jobs are interleaved in one apply, in the order they arrived. That was already the case for any commands sent while a queue existed.
• If a router-start batch stopped on an error between Start and Finish, its queue is left behind, as before. Previously the next Start replaced it. Now the next Start joins it, so those older commands for the same router are applied, ahead of the new ones.
• The complete fix would serialise aggregated applies per router on the management server, or tag each aggregation with an id. Both are larger changes. This PR only makes the agent side safe.

This may help #11710, but I haven't verified that it fixes it.

Fixes: #14639

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)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

How Has This Been Tested?

• New VirtualRoutingResourceAggregationTest in core (mvn -pl core -am test -Dtest=VirtualRoutingResourceAggregationTest), 5 tests, all pass. It uses a mocked VirtualRouterDeployer:
  • two overlapping aggregations: one config file is applied, and it holds the commands of both jobs; the second Finish succeeds without applying again;
  • the apply of a joined aggregation fails: both Finish answers fail, with the apply's details;
  • a later successful apply clears the recorded failure;
  • a Finish without a Start succeeds and applies nothing;
  • after a Finish, commands are applied directly again.
• Without this change, 4 of the 5 tests fail: the overlapping and failure cases, and the NPE on a Finish without a Start.
• VirtualRoutingResourceTest is @Ignored upstream, so it didn't run.
• The bug was found in a lab run of 24 parallel tier deploys in a redundant VPC on KVM. This change touches agent concurrency and has not been run in that lab yet. It needs this live test: repeat the 24 parallel tier deploys, several rounds, on a redundant VPC. Check for:
  • no deploy failing with aggregated commands execution failure;
  • no Queue.clear() NPE in the agent logs;
  • the same keepalived VIP set on both routers, with one PRIMARY;
  • every tier's interfaces and rules present on both routers.

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 16.49%. Comparing base (e6f193d) to head (f7282a1).

Files with missing lines	Patch %	Lines
...esource/virtualnetwork/VirtualRoutingResource.java	95.45%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #14640      +/-   ##
============================================
+ Coverage     16.45%   16.49%   +0.03%     
- Complexity    13710    13741      +31     
============================================
  Files          5669     5669              
  Lines        501734   501748      +14     
  Branches      60961    60964       +3     
============================================
+ Hits          82584    82765     +181     
+ Misses       409895   409701     -194     
- Partials       9255     9282      +27     

Flag	Coverage Δ
uitests	4.16% <ø> (ø)
unittests	17.36% <95.45%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bhouse-nexthop]

Test report: parallel tier deploys on a redundant VPC

Summary: I repeated the parallel tier bring-up that found #14639, for 40 deploys in 5 rounds. Every deploy succeeded. The concurrent aggregation the bug is about happened 27 times on one router. Each time, the second Start joined the queue and the late Finish reported the earlier apply's result. There was no NPE. After every round both routers had all 8 tiers, identical keepalived VIP sets, and exactly one PRIMARY. The stock run failed half its deploys.

Environment

• A CloudStack 24.0 (main) build with this PR, alongside some of my other open PRs. The management server and all three agents were upgraded in place to this build. The change is in the agent (VirtualRoutingResource, in core).
• The PR targets 4.22; it wasn't run on a 4.22 build. The code it changes is the same on both branches: 4.22's VirtualRoutingResource still does assert (!_vrAggregateCommandsSet.containsKey(routerName)) on Start, then replaces the queue, in a plain HashMap.
• Three KVM hosts, Ceph RBD primary storage, one management server. System VM template 4.22.0.
• A new VPC on the default Redundant VPC offering. Its two routers ran on different hosts, router 1 on host 3 and router 2 on host 1.
• vpc.max.networks was already 12 and was not changed.

Method

• Rounds 1-3 have the same shape as the stock run. Each round creates 8 tiers one after another (allocated, not yet implemented), then deploys one cirros VM into each tier, all 8 deploys at once. The tiers are implemented by those deploys, so 8 jobs program both routers at the same time.
• Rounds 4-5 run 8 workers at once. Each worker creates its tier and then deploys a VM into it, so the createNetwork calls are also concurrent.
• After each round I waited 60 s and checked:
  • every deploy's result;
  • the management server log for aggregated commands execution failure;
  • the agent logs on all three hosts for NullPointerException, Queue.clear and any exception in VirtualRoutingResource, and for the PR's two new warnings;
  • listRouters redundantstate, and checkrouter.sh on each router;
  • on both routers: the keepalived virtual_ipaddress set; for each tier, the gateway VIP in that set, an interface holding the router's own address in the tier (taken from nics), and that interface's ACL_INBOUND_ethN chain in iptables-save.
• Then the round's VMs and tiers were destroyed.

Results

Round	Deploys Running	aggregated commands execution failure	Agent NPE / VRR exceptions	Joined Starts / Finishes with no queue	Routers	VIP sets	Every tier on both routers
1	8 / 8	0	0 / 0	3 / 3	PRIMARY + BACKUP	identical (8)	yes
2	8 / 8	0	0 / 0	6 / 6	BACKUP + PRIMARY	identical (8)	yes
3	8 / 8	0	0 / 0	6 / 6	PRIMARY + BACKUP	identical (8)	yes
4 (concurrent create)	8 / 8	0	0 / 0	6 / 6	PRIMARY + BACKUP	identical (8)	yes
5 (concurrent create)	8 / 8	0	0 / 0	6 / 6	PRIMARY + BACKUP	identical (8)	yes
Total	40 / 40	0	0	27 / 27	one PRIMARY every round

Before this PR, with the same shape (the earlier stock run, 3 rounds of 8):

• 12 of 24 deploys failed with aggregated commands execution failure.
• The agents logged Cannot invoke "java.util.Queue.clear()" because "queue" is null 34 times.
• In two of the three rounds the routers' VIP sets differed by one tier gateway, and both routers were PRIMARY.

The race was hit, and handled

• All 27 overlaps were on router 2. Router 1 never had two aggregations at once in these runs. Agent log on router 2's host, round 2:

  20:50:07,611 WARN  VirtualRoutingResource  Command aggregation is already in progress for router <router 2>, joining it
  20:50:28,515 WARN  VirtualRoutingResource  Command aggregation is already in progress for router <router 2>, joining it
  20:50:28,515 WARN  VirtualRoutingResource  Command aggregation is already in progress for router <router 2>, joining it
  ...
  WARN  VirtualRoutingResource  No command aggregation in progress for router <router 2>, the last aggregated apply succeeded
  

• Each join was matched by one Finish that found no queue, and every one of those reported the last aggregated apply succeeded. Before this PR, each of those Finishes would have been the Queue.clear() NPE and a failed deploy.
• The commands of the joined jobs were applied. In every round router 2 had all 8 tiers: the gateway VIP, its own address on an interface, and that interface's ACL chain. Round 5, router 1, as an example (router 2 is the same):

  10.30.51.1/24 ... 10.30.58.1/24                       8 gateway VIPs, the same set on both routers
  ACL_INBOUND_eth2 ... ACL_INBOUND_eth9                 one chain per tier interface
  

• No agent logged any exception in these rounds.

Note on round 2: the routers swapped roles: router 2 was PRIMARY and router 1 BACKUP. There was still exactly one PRIMARY, and both routers had the same configuration. From round 3 on, router 1 was PRIMARY again.

Result: pass. Concurrent aggregations on one router no longer drop commands or fail jobs. 40 of 40 parallel deploys succeeded. Before the PR, 12 of 24 failed with the same shape. Both routers end each round with every tier configured, the same VIPs and one PRIMARY.

Not covered

• A failed aggregated apply while another job has joined it. Then both Finishes should report the failure; all applies in this run succeeded. That case is covered by the unit tests.
• A router-start batch that stops between Start and Finish, leaving a queue behind. The PR lists this as a limit.
• Only one of the two routers saw overlapping aggregations. The other router's tiers were applied without overlap, so they show the normal path only.
• Non-redundant VPCs and isolated networks: they use the same code, but I didn't run them in parallel here.

[DaanHoogland]

I would understand if you can't be bothered, but you reported this to be on 4.20 as well. Are you willing to rebase this @bhouse-nexthop ?

[bhouse-nexthop]

@DaanHoogland happy to. It's rebased onto 4.20 and the PR now targets 4.20 (f7282a1). The commit applied cleanly, and VirtualRoutingResourceAggregationTest and VirtualRoutingResourceTest pass on 4.20. It will reach 4.22 and main through the usual forward merge.

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] 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]

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

[bhouse-nexthop]

The Simulator CI failure on the 4.20 retarget is the runner failing to resolve the Ubuntu apt mirror in "Install Build Dependencies" (every job failed at that step, before any test ran). The other workflows, including Build and the unit tests, passed. Could someone re-run Simulator CI when convenient?