Repository navigation
Conversation
…e it runs The score export's services gain max_points: what the team would hold had every one of its checks of the service passed (the sum of points over its stored checks). A consumer scaling service scores no longer has to estimate it from points and uptime. For that sum to be right, every stored check has to carry its points. The runner copied them from the check's answer, so a check still running at the round deadline had 0, and was stored that way whenever the engine was still collecting as its result arrived. The runner now sets the points before the check runs.
…ntegration target The runner sets a result's points from the check definition and no longer reads them from the check's answer, so Service.Run and the ping, ssh and winrm checks setting them wrote a value nothing read. The runner's deadline test needs Redis and skips under -short, which every CI unit run uses; make test-integration now includes ./runner/... so CI runs it.
3 of 5 tasks
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
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.
Summary
GET /api/engine/export/scoresgives each service's earned points, but not what the team could have earned. A consumer that scales service scores against a maximum has to estimate it. The WRCCDC portal does this and depends on this change: wccomps/wccomps-portal#310 replaces its estimate (points divided by uptime) withmax_points.Changes:
max_points: what the team would hold had every one of its stored checks of the service passed, the sum ofpointsover those checks (GetServiceScores,ServiceScoreData.MaxPoints). A team checked in fewer rounds, or with a service disabled for some rounds, reports less for that service.Pointsfrom the check definition before the check runs (Runner.GetPoints), instead of copying it from the check's answer. The checks no longer setPointsthemselves; nothing read it after that.make test-integrationalso runs./runner/..., so the runner test below runs in CI.Why the runner change: checks cut off at the round deadline were stored with 0 points
For the sum to be right, every stored check has to carry its points. A failed check earns nothing, so this never showed in scores. But a check still running at the round deadline never answers, and unless an earlier attempt had answered, the runner's result for it had
Points: 0.Such a result can still be stored. The engine collects with
BLPop(timeoutCtx, time.Until(NextRoundStartTime)): go-redis rounds a wait under one second up to one second, and a BLPOP already sent ignores the context, becauseContextTimeoutEnabledis unset. So when the engine's last wait starts with under a second left, it blocks past the deadline, and a cut-off result pushed in that window is collected. The round is then stored if that was the last missing result. Those checks were stored with 0 points, andSUM(points)under-counted the maximum. With the points set before the check runs, a cut-off check is stored with its points like any other failed check.Open question
The stored points still come from the runner's result, so the exported maximum depends on every runner running this version, and on trusting what a runner reports. The engine could instead stamp each result's points from its own check config when it stores the round (
processCollectedResults), and stop trusting the runner's value. The engine tests feedprocessCollectedResultsresults for services no config names and expect their points stored, so that would change who owns the value; I left it as is. Would you rather the engine own it?Found while looking, not changed here
go-redis cuts the collection wait down to whole seconds, and an empty BLPOP discards the whole round (
engine.go:566-570). So collection ends anywhere from a second before the deadline to a second after it, and on-time results pushed in the last second can lose a round. Measured against Redis 7: with 2.7s left and a result pushed 300ms before the deadline, the wait came back empty 665ms before it and the round was discarded. A fix would retry an empty wait while the deadline hasn't passed. Happy to send it separately.Notes
max_pointsshort by those rows. Deploying between competitions avoids it (a score reset truncates the table).checks.Resultis both what a check answers and what the runner pushes, so a check can still set fields the runner owns (as ping, ssh and winrm did withPoints). A narrower answer type forRunner.Runwould rule that out; I kept that refactor out of this change.Verification
TestHandleTaskKeepsPointsWhenRoundEndsFirst(runner/runner_test.go): a check that outlives the round deadline is pushed with the service's points. It fails with the runner change reverted.TestGetServiceScoresReportsEarnedAndPossiblePoints(tests/integration/service_scores_test.go):GetServiceScoresreturnsMaxPointsas the sum of points over a team's stored checks, passed or not, and a team checked in fewer rounds reports less. It fails withmax_pointssummed over passed checks only.ExportScorescopies the value intomax_pointsunchanged.golangci-lintv2.15: 0 issues.gosecwith the CI exclusions: no findings.go test -race -short ./engine/... ./www/... ./runner/...: pass.go test -race -p 1 ./tests/integration/... ./engine/... ./runner/...(thetest-integrationtarget) against the compose test Redis and Postgres: pass.