bhouse-nexthop opened a new pull request, #14640:
URL: https://github.com/apache/cloudstack/pull/14640
### 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)
- [x] 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
- [x] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] 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 `@Ignore`d 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
