fix: two inert defects, before something copies them - #76
Merged
Conversation
RUNPOOL_SETTLE_SECS was defined outside the snapshot-source-restore block, so a value in the config file beat one in the environment, the opposite of every other setting and of what the documentation says. Nothing outside scheduler.sh reads it and nobody sets it twice, so this never bit. It is fixed because it is the nearest neighbour of any setting added next: designing the stuck-queue guard, the obvious move was to copy it. The health check did not consult the reconfiguration lock, so a check landing while registrations were being rewritten could call a deliberate operation an outage, at critical. Unreachable for an org pool, whose registration count spans the whole organisation, and now unreachable full stop. Both cases fail without their fix. Closes #68 Closes #69
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #68 and #69. Neither has ever fired; both are fixed because they are cheap and because one of them has already tried to propagate.
#69, precedence.
RUNPOOL_SETTLE_SECSwas defined outside the snapshot-source-restore block, so a value in the config file beat one in the environment, inverting the documented order and every other setting's behaviour. Nothing outsidescheduler.shreads it and nobody sets it twice, so it never bit. It matters as a pattern: it is the nearest neighbour of the next setting anybody adds, and while designing the stuck-queue guard the obvious move was to copy it.#68, the health check. It did not consult the reconfiguration lock, so a check landing while
set-count,reregisterorrenamewas rewriting registrations could report a deliberate operation as an outage, atcritical, to the person holding the lock. Unreachable for an org pool, whoseregcount spans the whole organisation, and reachable only for a repo pool in the seconds between a deregister and its re-register. Now skipped with the same predicateautoscaleandupalready use.Verification
Two cases added to
tests/pool-settling-window.sh, both offline. The precedence one sources the library in a subshell per case, since precedence is decided once at load. The health one asserts by API call count, because the point is that GitHub is not asked at all rather than asked and forgiven; it holds the lock with a real live pid, since$$reads as our own lock and an unsignallable pid reads as a dead holder.Reverting either library change fails its case.
bash -n,shellcheck --severity=warningand all ten tests pass.