Repository navigation
Report what the controller is not being told - #6434
Conversation
5437a15 to
0c7b739
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6434 +/- ##
==========================================
+ Coverage 25.87% 26.25% +0.38%
==========================================
Files 513 523 +10
Lines 93751 95570 +1819
==========================================
+ Hits 24257 25095 +838
- Misses 67620 68414 +794
- Partials 1874 2061 +187 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // interfaces): agents like downloader/mgmtproxy can otherwise grow | ||
| // URLCounters without bound and exceed the pubsub message size limit. | ||
| // The actual count may transiently exceed this; see AgentMetrics. | ||
| const MaxURLCounters = 100 |
There was a problem hiding this comment.
We may need to lower this -- with the newly added fields, the combined size of 100 entries (for e.g. app image blobs) could now exceed the 64KB pubsub limit.
It also might make sense to add json:",omitempty" tag to error counters, which are likely to be at zero for most entries.
There was a problem hiding this comment.
Good catch, and the margin is tighter than 64KB: sendUpdate base64-encodes the value before measuring it, so the 65535-byte socket message leaves about 48KB for the JSON. The peak is also 115 entries rather than 100, since makeRoomForNewURL only evicts once the count is past MaxURLCounters+urlCountersWiggle.
Measured max entries that still fit, for a downloader-shaped entry (byte counters set, answer counters zero, one interface):
| URL length | master | with the new fields | with omitempty |
|---|---|---|---|
| 100 | 144 | 121 | 144 |
| 128 | 133 | 113 | 133 |
| 160 | 122 | 105 | 122 |
| 200 | 111 | 97 | 111 |
| 256 | 99 | 87 | 99 |
So the new fields moved the overflow point from ~165-character URLs down to ~120, and a registry blob URL is 126 characters just for https://<registry>/v2/<org>/<repo>/blobs/sha256:<64 hex>.
I took the omitempty suggestion and applied it to all three, DeliveredMsgCount included: they are written only by RecordAnswer, which is called only from controllerconn/send.go, so for downloader and mgmtproxy -- the two that can actually fill URLCounters -- all three stay zero forever. That restores the master column exactly, so I would rather leave MaxURLCounters at 100 than give up the entries. Nothing changes on the wire to the controller either, since the proto3 scalars already skip zeroes.
The second commit goes at what I think is the more durable problem: an entry cap cannot bound a byte budget while the URL keys are unbounded (metricsURL is the full DownloadURL, query string included). AgentMetrics.Publish now checks the encoded size and drops least-recently-used entries until the value fits, and returns an error rather than publishing if it still does not -- today an overflow is a Fatalf in the publishing agent, which is the failure your original commit was chasing.
Happy to lower MaxURLCounters too if you would rather have the margin.
44eb03c to
b5da0e0
Compare
milan-zededa
left a comment
milan-zededa
left a comment
There was a problem hiding this comment.
LGTM but there are merge conflicts to address.
Each metrics message now carries the state of the deferred queue: how many messages are still waiting for the controller, how long the oldest of them has been waiting, and how many were given up on since boot, split into the ones the controller rejected and the ones a later periodic publication supersedes. Only the two queues carrying controller traffic are counted; the local operator console queue holds nothing the controller is waiting for. The advertised API capability moves to DEFERRED_QUEUE_METRICS. Alongside it, every controller URL now records how its answers came out - accepted, refused with a status worth offering the payload against again, or rejected outright. The existing per-URL counters say only whether the controller was reachable, which a device whose reports are being turned away looks identical to. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Extends the controller fault suite with two cases which read back what the device says about its own queue. With the controller answering 503 to every info message, the metrics have to show a growing count of undelivered messages, the age of the oldest one, and refusals counted against the info endpoint while the metrics endpoint keeps counting accepted answers; the queue then has to drain to empty once the controller accepts info again. With the controller rejecting info messages instead, the drops have to be reported as rejections with the time of the last one, and none of them counted as superseded. The metrics message does not travel the deferred queue, so a fault on the info path leaves the device able to report what the fault is doing to it. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The controller re-serializes what a device sent using its own copy of the API, so fields newer than that copy never reach a test. 0.0.82 carries an eve-api recent enough to parse the deferred queue metrics, which the controller fault tests assert on. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The per-URL answer counters are written only for URLs carrying controller or local profile server traffic. The agents which can fill URLCounters to its cap -- downloader fetching image blobs, mgmtproxy per target -- never touch them, so on those agents the three counters serialize as zeroes on every entry and buy nothing. That is not free. pubsub base64-encodes a published value before measuring it against its 64KB socket message limit, leaving about 48KB for the JSON, and a map at its peak occupancy of 115 entries with registry blob URLs as keys sits close to that ceiling already; the zeroes cost about 10KB of it, and going over is a fatal in the publishing agent rather than a dropped message. Omitting them when zero puts a full map back at the size it was before the counters existed. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: eriknordmark <erik@zededa.com>
The URLCounters cap counts entries, but what it defends is the encoded size of the published map, and the length of the URL keys dominates that. A run of long URLs -- a registry blob URL, or a datastore URL carrying a query string -- puts the map over the limit at an entry count the cap still allows, and pubsub answers an oversized value with a fatal rather than an error, taking down an agent that was only reporting on itself. Ask before publishing, and give up the least recently used entries, counting each as redacted, until what is left fits. If even an empty set of URL counters does not fit, report that and skip the publication instead of letting it be fatal. Signed-off-by: eriknordmark <erik@zededa.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: eriknordmark <erik@zededa.com>
b5da0e0 to
01031c0
Compare
2 participants
Description
A device that cannot deliver a report to the controller says nothing about it
today: the controller sees an absence, which is indistinguishable from a device
with nothing to say. This adds that missing account.
Each metrics message now carries the state of the deferred queue -- how many
messages are still waiting for the controller, how long the oldest of them has
been waiting, and how many were given up on since boot, split into the ones the
controller rejected and the ones a later periodic publication supersedes. Only
the two queues carrying controller traffic are counted; the local operator
console queue holds nothing the controller is waiting for.
Alongside it, every controller URL records how its answers came out: accepted,
refused with a status worth offering the payload against again, or rejected
outright. The existing per-URL counters say only whether the controller was
reachable -- an answered 404 counts as a success there -- so a device whose
reports are being turned away looks identical to a healthy one. These three
counters are what separate them.
Reporting three more counters per URL costs bytes in a message that already
runs close to the pubsub size limit, because agents fetching many distinct URLs
-- downloader pulling image blobs -- hold a per-URL entry for each. The
counters are left out of the encoding while zero, which is always the case for
those agents, and the map is now measured before it is published, giving up the
least recently used entries until what remains fits. An oversized publication
is fatal to the publishing agent, so this trades a crash for the loss of the
oldest per-URL detail.
The advertised API capability moves to
DEFERRED_QUEUE_METRICS.PR dependencies
deferredQueueMetric,the three
urlcloudMetriccounters and the capability value this PR reports.Already satisfied by master, which pins eve-api at
v0.0.0-20260907084402-08ba01c79328; this PR carries no bump of its own. Noreplacedirective anywhere.end-to-end tests here cannot pass without it. Adam re-serializes what a device
sent using its own copy of the API, and protojson drops what that copy does not
carry, so against the previous adam every counter added by Removing hard coded count of TSC members #154 read as unset
however correctly the device reported it -- which presents as an EVE bug.
Satisfied by the published
lfedge/adam:0.0.82, which the third commit hereselects.
Behaviour also builds on #6304 (merged), which introduced the retriable/rejected
distinction in the deferred queue that these counters report, and the controller
fault injection the tests use.
How to test and validate this PR
Covered by automated tests.
Unit tests,
make -C pkg/pillar test: ten new cases acrosscontrollerconnand
typescovering the queue snapshot (backlog, message age, the two dropreasons, aggregation across queues), the answer classification, and the size
guard -- a map that fits publishes untouched, one that does not is trimmed
least recently used first until it does, and one that cannot fit at all is
reported rather than published. Each was validated against un-fixed code --
neutering the drop accounting, the message-age inheritance, the age entirely,
the answer classification, the omitted zero counters or the size check makes
the matching test fail on its assertion, and all pass again once restored.
End to end,
evetest/tests/controllerfaults, two new cases inTestControllerFaultsSuite:TestDeferredQueueBacklogReported-- with the controller answering 503 toevery info message, the metrics must report a growing backlog, the age of the
oldest held message, and refusals counted against the info endpoint while the
metrics endpoint keeps counting accepted answers; nothing may be counted as
given up on, and the queue must drain to empty once the controller accepts
info again.
TestDeferredQueueDropsReported-- with the controller rejecting infomessages with 404, the drops must be reported as rejections with the time of
the last one, and none of them counted as superseded.
Both require
EVETEST_CONTROLLER_FAULTS=true; they skip without it. Run with:Validated on amd64/KVM against an image built from this branch: the whole suite
passes, 5 of 5 subtests, with the two new ones at 111.98s and 112.06s. Not run
on arm64.
Note for whoever adds to this suite next: the subtests share one device, and
each one's setup allows
deviceApplyConfigTimeoutfor the previous one'sapplication to be reported gone, which does not cover finishing a container
shutdown as well as deleting it. A subtest returning while the application it
stopped is still halting therefore fails the next subtest's setup. The two
new tests wait for the application to be reported halted before returning, as
the two oldest ones already did, and
TestInfoDroppedOnRejectionKeepsDeviceReportingis ordered last because bydesign it cannot.
Changelog notes
The controller is now told what a device is holding back or has given up on
delivering to it: the number of undelivered messages, how long the oldest has
been waiting, and how many were dropped because the controller rejected them or
because a later report supersedes them. Per-URL counters additionally
distinguish an accepted answer from a refusal worth retrying and from an
outright rejection.
PR Backports
Checklist
eve-api (Removing hard coded count of TSC members #154); there is no pillar-side doc describing reported metrics.
architecture independent.
stablelabel.