Skip to content

EVE-k: fix stray-daemonset storage block and report readiness truthfully - #6240

Open
eriknordmark wants to merge 6 commits into
lf-edge:masterfrom
eriknordmark:evek-storage-readiness
Open

EVE-k: fix stray-daemonset storage block and report readiness truthfully#6240
eriknordmark wants to merge 6 commits into
lf-edge:masterfrom
eriknordmark:evek-storage-readiness

Conversation

@eriknordmark

@eriknordmark eriknordmark commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Description

Two independent robustness fixes for how an EVE-k node decides, and reports, whether cluster storage is usable. Both found while running kvm→EVE-k conversion tests.

A stray DaemonSet in longhorn-system blocks every volume on the node. checkLonghornReady required every DaemonSet in that namespace to have a Running-and-Ready pod here, using its list of expected ones (longhorn-manager, longhorn-csi-plugin, engine-image) only to confirm those three exist. Anything else sharing the namespace that never becomes ready therefore gates storage permanently, with app volumes stuck in CREATING_VOLUME as the only visible symptom. EVE's own collect-info reaches this by leaving a SupportBundle DaemonSet behind — see #6230, where it caused 7/7 test failures on a device and 16 consecutive passes once the object was removed. Now only Longhorn's own DaemonSets gate readiness, which also drops a latent false failure where a DaemonSet whose node selector excludes this node counted as missing.

VolumeMgrStatus.Initialized was set regardless of whether storage came up. volumemgr waits up to 40 minutes for cluster storage and then published success unconditionally, so a node whose Longhorn or CDI never converged was indistinguishable from a healthy one outside volumemgr's own log. Initialized now reflects the outcome, and a new UnmetCondition carries the outstanding gate — reusing the sub-condition the kubernetes wait already computes and currently discards. Non-EVE-k nodes have no such gate and are Initialized from the start, as before. Volumes are unaffected either way: they are gated separately and retry until storage appears.

The point of reporting the outcome is access from outside volumemgr. The status lands in /run/volumemgr/VolumeMgrStatus/volumemgr.json, so a test can ssh in and assert on it and collect-info captures it in the bundle. diag now subscribes as well and prints a warning naming the outstanding gate, so an operator at the console can see why applications are waiting for storage rather than having to correlate agent logs.

Deliberately not fixed here: volumemgr publishes nothing at all during those 40 minutes, because pubVolumeMgrStatus and the inputs to getRemainingDiskSpace are created after the waits — so "converging" cannot be told from "wedged". That needs a startup reorder, and any early publish must carry a real RemainingSpace, since nodeagent treats 0 as grounds for MaintenanceMode. Separate PR.

How to test and validate this PR

Every change here has a unit test that fails without it, each confirmed by reverting the change and re-running:

go test -tags k -run TestCheckLonghornReady ./kubeapi/
go test -tags k -run TestVolumeMgrStatus ./cmd/volumemgr/
go test -run TestDiag ./cmd/diag/

TestCheckLonghornReadyIgnoresStrayDaemonset fails with the production symptom (longhorn daemonset:longhorn-support-bundle-agent missing on this node) while the other three keep passing. TestVolumeMgrStatusReportsUnmetCondition fails if the reported readiness goes back to being a constant. TestDiagReportsUnmetStorageCondition fails without the console warning; the two tests beside it pin the quiet cases — storage usable, and volumemgr not having published yet.

Testing the reported readiness needed one production change: generateAndPublishVolumeMgrStatus computes the remaining disk space from /persist before publishing, which no test environment has, so the function returned before publishing anything. The status construction is now a separate method and the publishing path is otherwise untouched.

On an EVE-k device, first fix: with Longhorn healthy, create any DaemonSet in longhorn-system that cannot get a ready pod, then deploy an app with a volume. Before this change the app sits in CREATING_VOLUME and volumemgr logs longhorn not ready: longhorn daemonset:<name> missing on this node; after it, the app deploys. Through the real path instead: run collect-info, confirm the leftover SupportBundle DaemonSet, then deploy.

Second fix: on a node where cluster storage never comes up, /run/volumemgr/VolumeMgrStatus/volumemgr.json should show "Initialized": false with "UnmetCondition" naming the gate, where it previously showed true. On a healthy node, and on any non-EVE-k device, true with an empty UnmetCondition. In the failing case the console — and /run/diag.out — additionally carries WARNING: cluster storage did not become ready at startup: <condition>, and nothing extra in the healthy case.

A new evetest package covers the healthy path:

make evetest NAME=TestDiagOutput

Nothing tested diag's output before — it reaches no EVE API message, so a lost section on a healthy device went unnoticed. The test deploys an application and reads the summary the way a consumer does, through GET /eve/v1/diag on the metadata server, requiring the device to report itself online and connected, its management port up, and the deployed application running. It is also the first coverage of msrv's diag handler. Unlike the unit tests above it is general coverage rather than a regression test for this change: it asserts the healthy path, which looks the same before and after.

Changelog notes

Fixed a leftover DaemonSet in the longhorn-system namespace — for example one left behind by collect-info — preventing a Kubernetes-enabled node from creating any application volume, leaving applications waiting for storage. The device now also reports accurately whether its cluster storage became usable, instead of always reporting success.

PR Backports

The template's list does not yet include 17.0-stable, which is where these belong.

  • 17.0-stable: To be backported.
  • 16.0-stable: No.
  • 14.5-stable: No.
  • 13.4-stable: No.

Checklist

  • I've provided a proper description
  • I've added the proper documentation
  • I've tested my PR on amd64 device
  • I've tested my PR on arm64 device
  • I've written the test verification instructions
  • I've set the proper labels to this PR

And the last but not least:

  • I've checked the boxes above, or I've provided a good reason why I didn't
    check them.

pkg/pillar/docs/volumemgr.md now describes VolumeMgrStatus and its three fields,
which it did not mention at all; the fields themselves are documented in the type.

The second fix is verified on an amd64 EVE-k device (QEMU under Eden) in both
directions across a seven-topology conversion sweep. Where cluster storage came up,
volumemgr published "Initialized":true with an empty UnmetCondition; on a run
where it did not, the same status carried
"UnmetCondition":"timed out waiting for the condition (last unmet condition: longhorn not ready: daemonset:longhorn-manager not running on node)" with
"Initialized":false — a state that was reported as success before this change, and
that took device-log archaeology to identify. The first fix is covered by the unit
tests; its device reproduction needs a stray DaemonSet planted by hand, and the steps
are above. TestDiagOutput passes against an amd64 kvm device built from this branch,
retrieving the summary below through the metadata endpoint — note the header, which shows
the new subscription driving the print, and the absence of any cluster-storage warning on
a node whose storage is usable from the start:

INFO: updated diag information at 2026-08-02T12:16:28.911588547Z due to Appinstance
WARNING: device: online attest: Complete vault: DISABLED unlock:no-tpm pcr: DISABLED
INFO: applications: 0 starting, 1 running
INFO: Summary: Connected to EV Controller and onboarded
INFO: Using highest priority DevicePortConfig key zedagent
INFO: Have 1 total ports. 1 ports should be connected to EV controller
INFO: Port eth0: Mac: da:39:78:d0:19:f2 link: up use: mgmt 172.20.20.123
INFO: App diag-reader uuid 945c1a47-1abc-4915-bdd2-707612475b06 state RUNNING

Not run on arm64: everything here is architecture-independent Go in pillar.

eriknordmark and others added 2 commits July 28, 2026 07:00
An EVE-k node reports cluster storage as unready, and every app volume
stays in CREATING_VOLUME, whenever any DaemonSet in the longhorn-system
namespace lacks a Running-and-Ready pod on this node -- including
DaemonSets that Longhorn does not own and that are never expected to
become ready. The readiness check iterated over every DaemonSet in the
namespace and required each one to be healthy, consulting its list of
expected DaemonSets only afterwards to confirm those three exist.

In practice this is reached through EVE's own collect-info, which leaves
a SupportBundle agent DaemonSet behind; the node then refuses to serve
volumes for as long as that object exists, with no way for an operator
to tell why. Restrict the per-node health requirement to the DaemonSets
Longhorn is expected to run, and skip anything else sharing the
namespace.

Restricting the loop also removes a second false failure: a DaemonSet
whose node selector legitimately excludes this node reported zero pods
here and was treated as missing.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
On an EVE-k node volumemgr waits up to 40 minutes for the cluster to be
able to serve a volume, and then reports Initialized regardless of how
that wait ended. A node whose Longhorn or CDI never came up is therefore
indistinguishable, from the outside, from a healthy one -- the only
difference is a line in volumemgr's own log. Anything consuming the
status, an operator inspecting it, or a test asserting on it is misled
in precisely the case that matters.

Report the outcome instead: Initialized now reflects whether cluster
storage became usable, and a new UnmetCondition carries the gate that
was still outstanding, reusing the sub-condition the kubernetes wait
already computes ("longhorn not ready: ...", "kubevirt not ready: ...").
Nodes that are not EVE-k have no such gate and are Initialized from the
start, as before.

Volumes are unaffected either way: they are gated separately and defer
and retry until storage appears.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@eriknordmark eriknordmark added the stable Should be backported to stable release(s) label Jul 28, 2026
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 25.00000% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 23.36%. Comparing base (5d69266) to head (dc8490c).
⚠️ Report is 26 commits behind head on master.

Files with missing lines Patch % Lines
pkg/pillar/cmd/diag/diag.go 10.71% 25 Missing ⚠️
pkg/pillar/cmd/volumemgr/volumemgr.go 0.00% 6 Missing ⚠️
pkg/pillar/cmd/volumemgr/handlediskmetrics.go 66.66% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6240      +/-   ##
==========================================
+ Coverage   22.93%   23.36%   +0.43%     
==========================================
  Files         510      520      +10     
  Lines       93473    95230    +1757     
==========================================
+ Hits        21440    22254     +814     
- Misses      70292    71039     +747     
- Partials     1741     1937     +196     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eriknordmark
eriknordmark marked this pull request as ready for review July 28, 2026 20:34
@eriknordmark
eriknordmark requested review from milan-zededa and rene and removed request for OhmSpectator and zedi-pramodh July 31, 2026 18:10
eriknordmark added a commit to eriknordmark/eve that referenced this pull request Jul 31, 2026
Record the 2026-07-30 replay of the lf-edge#5971 segment and drop the local CDI CR
fix, which the PR now handles itself: CDI is split into cdi-operator and cdi
components joined by a CDIOperatorReady signal, and a BestEffort timeout hands
the component to a retry loop that applies the CR without re-awaiting the
signal. Three local fixes remain. Also list lf-edge#6240, which the branch already
carried.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
eriknordmark added a commit to eriknordmark/eve that referenced this pull request Jul 31, 2026
Record the 2026-07-30 replay of the lf-edge#5971 segment and drop the local CDI CR
fix, which the PR now handles itself: CDI is split into cdi-operator and cdi
components joined by a CDIOperatorReady signal, and a BestEffort timeout hands
the component to a retry loop that applies the CR without re-awaiting the
signal. Three local fixes remain. Also list lf-edge#6240, which the branch already
carried.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@andrewd-zededa andrewd-zededa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we subscribe to longhorn readiness in the future and republish VolumeMgrStatus if there are future longhorn control plane issues?

Name: agentName,
Initialized: true,
Initialized: ctx.storageReady,
UnmetCondition: ctx.storageUnmet,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anyone using this field yet, is there a consumer coming in another PR?

@eriknordmark eriknordmark Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No consumer — and Initialized right above it has never had one either; nodeagent is the only subscriber and reads just RemainingSpace. The motivation is making the state available to tests and diagnosis: the status lands in /run/volumemgr/VolumeMgrStatus/volumemgr.json, so a test can ssh in and assert on it, and collect-info picks it up in the bundle.

Adding diag as a consumer was easy, so I've done it here — it prints a warning naming the outstanding gate when cluster storage didn't become usable.

Nothing reports this to the controller today: ZInfoClusterNode carries only the node_status enum and pillar doesn't populate it, so that's an eve-api change and a separate PR. Let's discuss what else we'd want in the API in this area. One thing to design around: these two fields are a one-shot startup outcome, not a live condition — volumemgr decides once after its wait and republishes the same value.

An EVE-k node whose cluster storage never converged looks healthy from the
console: applications that need a volume simply sit waiting, and nothing in
the diag summary says why. Now that volumemgr reports that outcome, diag
subscribes to VolumeMgrStatus and prints a warning naming the gate that was
still outstanding when storage failed to become usable at startup. A healthy
node, and any device that is not EVE-k, prints nothing extra.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
diag is the only operator-facing summary of device health -- controller
connectivity, attestation and vault state, applications, cluster storage --
and none of it reaches the EVE API, so nothing noticed when it lost a
section, went silent, or filled with errors on a healthy device.

Add a test that deploys an application and reads the summary the way a
consumer does, through the metadata server at GET /eve/v1/diag, asserting
that a healthy onboarded device reports itself online and connected, lists
its management port as up, and lists the deployed application as running.
It also cross-checks the storage state against volumemgr's own publication.
This is the first coverage of msrv's diag handler as well.

A device that reaches the controller takes the short path through the port
section, so the per-port detail and the "all management ports passed"
verdict are absent from a healthy summary; the assertions match the lines
that path actually emits.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@eriknordmark
eriknordmark force-pushed the evek-storage-readiness branch from 7adbdb8 to ef79fba Compare August 2, 2026 12:25
eriknordmark and others added 2 commits August 2, 2026 14:31
The evetest coverage of the diag summary asserts the healthy path, where
the cluster-storage warning is absent -- which is equally true of the code
before this warning existed, so it cannot tell the two apart.

Drive printOutput directly instead. Leaving the network state unset stops
it right after the storage line, so no subscription is needed, and the
three cases that matter can be asserted: a node whose storage never became
usable is named along with the outstanding gate, a node with usable storage
is not warned about, and neither is one whose volumemgr has not published
yet. The first of these fails without the warning in place.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing failed when volumemgr reported storage as usable regardless of the
outcome, which is what let the original bug stand. A test has to see the
published status, and the publishing path first computes the remaining disk
space from /persist -- absent in any test environment, so the function
returns before publishing anything.

Split the status out of the publish, and assert on it directly: a node whose
storage never came up reports that, along with the gate it was waiting on,
and a node with usable storage reports success and no gate. The first fails
if the field goes back to being a constant.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
eriknordmark added a commit to eriknordmark/eve that referenced this pull request Aug 3, 2026
An EVE-k node reports cluster storage as ready while it still cannot
attach any volume, so the first app deployed after a conversion or a
fresh install sits in volume creation for minutes while its CDI upload
pod repeats "volume ... is not ready for workloads". Readiness gated
only on the Longhorn DaemonSets, but a volume's engine and replica
processes run inside the per-node instance-manager pod, which is owned
by an InstanceManager CR and so was never examined. That pod is
typically still pulling its ~440 MB image when the DaemonSets go ready:
9m51s on one device here, and over twenty minutes on slower storage.

Require a running InstanceManager for this node in both the pillar and
the shell readiness paths. Longhorn creates the CR during node setup
rather than on first volume request, so waiting on it cannot deadlock
against a volume whose own creation is gated on storage readiness.

Cherry-picked from lf-edge#6259, with one branch-local adjustment: the
two positive daemonset tests stub the instance-manager gate, since a fake
clientset cannot supply the kubeconfig the real gate builds its Longhorn
client from. Those tests arrive with lf-edge#6240, which this branch also carries,
so the interaction only shows up here.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
eriknordmark added a commit to eriknordmark/eve that referenced this pull request Aug 3, 2026
An EVE-k node reports cluster storage as ready while it still cannot
attach any volume, so the first app deployed after a conversion or a
fresh install sits in volume creation for minutes while its CDI upload
pod repeats "volume ... is not ready for workloads". Readiness gated
only on the Longhorn DaemonSets, but a volume's engine and replica
processes run inside the per-node instance-manager pod, which is owned
by an InstanceManager CR and so was never examined. That pod is
typically still pulling its ~440 MB image when the DaemonSets go ready:
9m51s on one device here, and over twenty minutes on slower storage.

Require a running InstanceManager for this node in both the pillar and
the shell readiness paths. Longhorn creates the CR during node setup
rather than on first volume request, so waiting on it cannot deadlock
against a volume whose own creation is gated on storage readiness.

Cherry-picked from lf-edge#6259, with one branch-local adjustment: the
two positive daemonset tests stub the instance-manager gate, since a fake
clientset cannot supply the kubeconfig the real gate builds its Longhorn
client from. Those tests arrive with lf-edge#6240, which this branch also carries,
so the interaction only shows up here.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@eriknordmark

Copy link
Copy Markdown
Contributor Author

Heads-up on an interaction with 6259 that neither PR sees on its own.

#6240 adds pkg/pillar/kubeapi/longhornready_test.go, whose
TestCheckLonghornReadyHealthy and TestCheckLonghornReadyIgnoresStrayDaemonset
call checkLonghornReady with a fake.NewSimpleClientset. #6259 makes
checkLonghornReady end with instanceManagerReady(ctx, nodeName), and the real
gate builds its Longhorn client from the on-device kubeconfig. Under a fake
clientset that file does not exist, so both tests fail with:

longhorn instance-manager: kubeconfig: stat /run/.kube/k3s/k3s.yaml: no such file or directory

No merge order is required — both are based on master and touch disjoint
lines, so they merge cleanly in either order. But whichever lands second must
carry the test fix, or master goes red. instanceManagerReady is a package-level
var, so stubbing it is enough:

// stubInstanceManagerGate satisfies the instance-manager check for tests that
// exercise the daemonset logic: the real gate builds a Longhorn client from the
// on-device kubeconfig, which a fake clientset cannot supply.
func stubInstanceManagerGate(t *testing.T) {
	saved := instanceManagerReady
	t.Cleanup(func() { instanceManagerReady = saved })
	instanceManagerReady = func(context.Context, string) error { return nil }
}

called at the top of both tests (plus a "context" import).

Found on an integration branch carrying both PRs; with the stub applied,
go test -tags k ./kubeapi/... passes.

eriknordmark added a commit to eriknordmark/eve that referenced this pull request Aug 3, 2026
Record lf-edge#6259 and the two source PRs the table never listed (lf-edge#6240, lf-edge#6242),
note that lf-edge#6063 is now 17 commits, and mark the CDI upload-pod teardown
logging as upstream in master rather than branch-local. Add the lf-edge#6240/lf-edge#6259
test interaction and the rebase-due note: master is 63 commits ahead of the
shared merge-base.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eriknordmark added a commit to eriknordmark/eve that referenced this pull request Aug 3, 2026
Record lf-edge#6259 and the two source PRs the table never listed (lf-edge#6240, lf-edge#6242),
note that lf-edge#6063 is now 17 commits, and mark the CDI upload-pod teardown
logging as a pre-merge copy of an upstream commit. Add the lf-edge#6240/lf-edge#6259 test
interaction and the rebase-due note: master is 63 commits ahead of the shared
merge-base.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eriknordmark added a commit to eriknordmark/eve that referenced this pull request Aug 3, 2026
Record lf-edge#6259, note that lf-edge#6063 is now 17 commits, refresh lf-edge#6240's head, and mark
the CDI upload-pod teardown logging as a pre-merge copy of an upstream commit.
Spell out why lf-edge#6242 is not replayed here and that its NAD-CRD race is still
open in the Go ApplyMultusCNI. Add the lf-edge#6240/lf-edge#6259 test interaction and the
rebase-due note: master is 63 commits ahead of the shared merge-base.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eriknordmark added a commit to eriknordmark/eve that referenced this pull request Aug 3, 2026
Record lf-edge#6259, note that lf-edge#6063 is now 17 commits, refresh lf-edge#6240's head, and mark
the CDI upload-pod teardown logging as a pre-merge copy of an upstream commit.
Spell out why lf-edge#6242 is not replayed here and that its NAD-CRD race is still
open in the Go ApplyMultusCNI. Add the lf-edge#6240/lf-edge#6259 test interaction and the
rebase-due note: master is 63 commits ahead of the shared merge-base.

Signed-off-by: eriknordmark <erik@zededa.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stable Should be backported to stable release(s)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants