andrijapanicsb opened a new pull request, #14527:
URL: https://github.com/apache/cloudstack/pull/14527
### Description
This PR fixes #14522.
#12409 added a cleanup to the async-job GC task that moves networks stuck in
Implementing to Shutdown, so that a
network left in Implementing after a crash or a lost job can be used again.
It treats a network as orphaned when it
is older than `job.expire.minutes` and no pending job of instance type
`Network` refers to it. A network that a VM
start or deployment is implementing has no such job (the work runs inside
the VM's job), so a cleanup run that lands
during the implementation (for an isolated network, while its virtual router
boots) shuts the network down under the
implementing thread, and the start fails with "Network is not yet fully
implemented". The start and deploy APIs retry
once, which usually hides it. Instance boot group starts (#13907, on main)
start each VM once and fail outright; HA
restarts also start once (not tested here). Details and a log trace are in
#14522.
The thread that implements a network
(`NetworkOrchestrator.implementNetwork`) holds the network's lock-table lock for
the whole time the network is in Implementing. So a network in Implementing
is orphaned only if that lock is free.
This PR:
1. In `AsyncJobManagerImpl.cleanupOrphanedNetworks()`, for each candidate
that passes the existing age check:
- try the network's lock once, without waiting (`acquireInLockTable(id,
0)`); if it is held, skip the network;
- otherwise use the row re-read under the lock: skip unless it is still
Implementing, then apply the existing
pending-job check and the existing transition to Shutdown;
- release the lock in `finally`.
A network that is really stuck (for example after a management server
crash, whose locks are released when the
server leaves the cluster or restarts) is still cleaned up as before.
2. **This also changes the lock-table class in framework/db,
`Merovingian2`.** In `acquire(key, timeInSeconds)`, a
timeout of 0 (or less) now makes exactly one attempt and returns at once,
without sleeping and without the
"Timed out on acquiring lock" WARN. Callers with a positive timeout run
the unchanged loop.
- Why it is needed: today the wait loop is `while (elapsed <
timeInSeconds * 1000) { try; sleep 5 s }`. For 0 the
body never runs, so the lock is never tried: `acquireInLockTable(id,
0)` always returns null, and a WARN with a
stack trace is logged. The cleanup could then never clean anything up,
including a network that is really stuck.
- Why not a 1 s timeout: on a held lock, `acquire(key, 1)` tries, sleeps
5 s, and can try and sleep once more,
because the loop's clock (`InaccurateClock`) only ticks every 10 s.
Each busy network would block the GC task
for 5-10 s and log the timeout WARN with a stack trace. That would
happen on every implementation of an old
network, and many networks implemented at once (a mass start after
maintenance) would stall the job GC for
minutes.
- No current caller passes 0. How this was checked on 4.20:
- `Merovingian2.acquire` is only called from `TransactionLegacy.lock`,
which is only called from
`GenericDaoBase.acquireInLockTable` and `lockInLockTable`.
- All 78 call sites of `acquireInLockTable`/`lockInLockTable` (`grep
-rnE "acquireInLockTable\(|lockInLockTable\("`
outside tests) were reviewed. The one-argument form uses the DAO's
`lock.timeout`, which defaults to 300 s and
is not set anywhere in the code base. The explicit timeouts are
literals (10, 30, 1200) or three settings:
`network.lock.timeout` (default 600), `storage.pool.max.waitseconds`
(3600) and `vm.job.lock.timeout` (1800).
- `GlobalLock` (MySQL `GET_LOCK` through `DbUtil.getGlobalLock`) is a
separate mechanism and is not affected.
- So 0 can only come from an administrator setting one of those three
to 0. Today that makes every acquisition
fail at once (for example, no network could be implemented). With
this change it means a single attempt.
No schema change, no new setting.
### 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
- [ ] 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?
Unit tests:
- `AsyncJobManagerImplTest`: lock held → no transition; lock free, still
Implementing, old enough → transition to
Shutdown on the re-read network, then the lock is released; lock free but
the network was implemented meanwhile →
no transition; a pending Network job → no transition; a young network →
the lock is not tried; the forced Shutdown
fallback runs under the lock.
- `Merovingian2AcquireTest` (lock table calls mocked, no database): a zero
timeout takes a free lock with a single
attempt, returns false at once on a lock held elsewhere, re-enters a lock
held by the same thread, and gives up
after one attempt when the insert loses a race; a positive timeout still
waits and retries (held on the first
try, free on the second: acquired after about 5 s). The framework/db
module excludes `com/cloud/utils/db/*Test*`
from surefire (those are database-backed tests, like `Merovingian2Test`),
so this test was run with
`-Dtest=Merovingian2AcquireTest`.
- Negative control: with the new tests on the old code (only
`cleanupOrphanedNetworks` made `protected` so the tests
compile), 9 of the 11 new tests fail. The two that pass are the
young-network and positive-timeout cases, whose
behaviour this PR does not change.
Tested before and after the patch on a single-host KVM test environment
(MySQL 8.0, isolated networks on
DefaultIsolatedNetworkOfferingWithSourceNatService, default
`job.expire.minutes`, `network.gc.wait` and
`network.gc.interval`). The test environment ran the main nightly
(24.0.0-SNAPSHOT.20261027), with the two changed
classes, built from main with the same change, swapped into the management
server jar for the "after" run; the
changed code is identical on 4.20, and the unit tests above ran on this
branch, which is based on 4.20. To make a new
test network "old" without changing `job.expire.minutes` (which would also
expunge the job history), its `created`
column was moved back by two days.
| Case | Before (original jar) | After (patched jar) |
|---|---|---|
| Deploy a VM into a new network older than `job.expire.minutes` | Cleanup
fired 8 s into the implementation (`Found orphaned network ... age: 2880
minutes`); the first start attempt failed with "Network is not yet fully
implemented"; the deploy's retry passed | No orphan line; `Network <id> in
Implementing state is locked (being implemented) ... skipping cleanup` on 5
cleanup runs; one start attempt, passed |
| Instance boot group start (no retry) on such a network | Cleanup fired 10
s in; **the boot group start failed** ("Failed to start a VM in boot group
...") | Skipped on 3 runs; the boot group start succeeded on its single attempt
|
| Stop the VM, let the network GC shut the network down, start the VM again
| Cleanup fired 4 s in; the first attempt failed, the retry passed | Skipped on
4 runs; one start attempt, passed |
| Network older than the threshold set to Implementing in the DB, no lock
held (really stuck) | Moved to Shutdown within 10 s | **Still moved to Shutdown
within 10 s**; no lock left behind |
With the patch, no `Timed out on acquiring lock` line was logged, and the
only orphan line was the one for the really
stuck network. A bytecode comparison of the swapped classes against the
nightly's showed that only
`AsyncJobManagerImpl.cleanupOrphanedNetworks`, the new
`cleanupNetworkIfOrphaned` and `Merovingian2.acquire` differ.
#### How did you try to break this feature and the system with this change?
- The stuck case above: a network in Implementing with no lock holder must
still be moved to Shutdown.
- The cleanup holds a network's lock only for the few milliseconds of the
re-read and transition, and only when the
lock was free; an implementation that starts in that window waits for the
lock as it would for any other holder.
Thank you @Pearl1594 for #12409. This keeps its cleanup as it is and only
narrows what it treats as orphaned.
--
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]
