evetest: return errors from device file and publication reads - #6279
Conversation
scpFromEVE and scpToEVE returned a bare exec error, so a caller could not tell a file that does not exist yet from an unreachable device -- both surfaced as "exit status 1". The distinction matters to anything polling for a file EVE has not produced yet. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
ReadFile and ReadAllPublications called t.Fatalf on any failure, which ends the test goroutine outright -- the wrong contract for state EVE produces asynchronously, since a caller polling for something that has not appeared yet dies on the first attempt instead of retrying. Two call sites poll today: dns_test.go reads the mgmt dnsmasq servers file inside an Eventually while nim is still writing it, and zvol_provisioned_size_test.go reads volumemgr's AppDiskMetric the same way. ReadAllPublications was racy in a second way -- it lists the topic directory and then reads each file, so an unpublish in between was fatal. ReadPublication already supported polling, but by reporting absence as a bool after a FileExists probe. That costs an extra SSH round trip per read, races between the probe and the read, and collapses "not published yet" and "device unreachable" into the same false. An error covers both and says which. This also matches RunShellScript, the other way tests reach into a device. Fatalf stays for harness faults, where no test can proceed. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
find prints one path per line, and pubsub keys become file names, so a key containing a space -- types.DefaultVaultName is "Application Data Store" -- was torn into several nonexistent paths by strings.Fields. Splitting on whitespace happened to work only because every key read so far has been a UUID, "global", or a device path. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
ReadFile, ReadPublication and ReadAllPublications changed signature. Tests under tests/ are bind-mounted into the evetest container, but the framework they link against is compiled into the image, so a test written for the new signatures does not build until the image is rebuilt -- which only happens for a version the local docker daemon does not already have. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
0c1254e to
f79c7c4
Compare
scp speaks the SFTP protocol by default since OpenSSH 9.0, and evetest passes no -O to select the legacy one, so no remote shell ever sees the path. Quoting it therefore made the quotes part of the file name and every transfer failed, whether or not the path contained a space: scp: '/etc/resolv.conf': No such file or directory Spaces were the reason the quoting was added, but they are already safe: exec runs scp directly with no local shell, so the path reaches scp as a single argv element either way. Verified against a throwaway sshd with the same OpenSSH 9.9 client the evetest image ships, invoking scp with no shell so the quotes survive into argv exactly as they do here: the quoted form fails for both a spaced and a plain path, the raw form succeeds for both. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
|
@milan-zededa heads-up on a bug this turned up in current master, beyond the scope I opened the PR for.
The clearest demonstration comes from It affects |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6279 +/- ##
==========================================
+ Coverage 24.12% 24.49% +0.36%
==========================================
Files 512 522 +10
Lines 93653 95369 +1716
==========================================
+ Hits 22598 23359 +761
- Misses 69267 70040 +773
- Partials 1788 1970 +182 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Good, I just ran into this on #6257 |
Description
The evetest read helpers --
EdgeDevice.ReadFile,ReadPublicationandReadAllPublications-- ended the test throught.Fatalfon any failure.That is the wrong contract for state EVE produces asynchronously: a caller
polling for a file or a publication that has not appeared yet dies on its
first attempt instead of failing one
Eventuallyiteration and retrying.Two call sites depend on polling today:
tests/networking/dns_test.goreads/run/nim/dnsmasq.mgmt.serversinsidean
Eventuallywhile nim is still writing it.tests/storage/zvol_provisioned_size_test.goreads volumemgr'sAppDiskMetricthe same way.ReadAllPublicationswas racy in a secondway: it lists the topic directory and then reads each file, so an unpublish
in between killed the test.
ReadPublicationalready supported polling, but reported absence as aboolafter a separate
FileExistsprobe. That costs an extra SSH round trip perread, races between the probe and the read, and collapses "not published yet"
and "device unreachable" into the same
false. An error covers both and sayswhich -- and matches
RunShellScript, the other way tests reach into adevice.
Fatalfstays for harness faults, where no test can proceed.ReadFileis broken on master todayWhile validating the above,
ReadFileturned out to fail for every path,not just as an edge case.
scpFromEVE/scpToEVEshell-quote the remote path,but scp has spoken the SFTP protocol by default since OpenSSH 9.0 and evetest
passes no
-Oto select the legacy one, so no remote shell ever sees thepath. The quotes become part of the file name:
The clearest demonstration is from
TestZVolProvisionedSizeReported:ReadAllPublicationsrunsfindon the device, which locates/run/volumemgr/AppDiskMetric/dev-zvol-....json, and scp then reports thatexact path -- quoted -- as missing.
Spaces were the reason the quoting was added (
types.DefaultVaultNameis"Application Data Store"), but they are already safe: exec runs scp directlywith no local shell, so the path reaches scp as a single argv element either
way. Verified against a throwaway sshd with the same OpenSSH 9.9 client the
evetest image ships, invoking scp with no shell so the quotes survive into
argv exactly as they do here:
This affects
dns_test.godirectly andvault_trim_test.go/pciback_error_test.gothroughReadPublication. It was previously masked:scpFromEVEreturned a bareexit status 1, andReadFileturned that intoa
Fatalfthat named no cause.Also in this PR
scpFromEVEandscpToEVEnow wrap scp's own stderr, which is what makes"missing file" distinguishable from "unreachable device" -- and what made
the bug above legible in the first place.
ReadAllPublicationssplitfindoutput withstrings.Fields.findprints one path per line, and pubsub keys become file names, so a key
containing a space was torn into several nonexistent paths. Splitting on
whitespace happened to work only because every key read so far has been a
UUID,
"global", or a device path.evetest/VERSION1.1 -> 1.2.tests/is bind-mounted into the evetestcontainer, but the framework it links against is compiled into the image,
so a test written for the new signatures does not build until the image is
rebuilt -- which only happens for a version the local docker daemon does
not already have.
How to test and validate this PR
Both tests were run against a locally built live image from this branch
(
make live, thenEVETEST_EVE_LIVE_IMAGE=true). That matters: both assertrecent pillar fixes (
24ef31510zvol volsize,2e739c88emgmt dnsmasq) thatare absent from
17.0.0-lts, so against a published release they fail beforereaching any of this code.
make live EVETEST_EVE_LIVE_IMAGE=true make evetest NAME=TestZVolProvisionedSizeReported # PASSES EVETEST_EVE_LIVE_IMAGE=true make evetest NAME=TestDNSFunctionalityTestZVolProvisionedSizeReportedpasses, exercisingReadAllPublicationsend to end (error return and the newline split).TestDNSFunctionalitygets through Phase 1 and Phase 2 -- Phase 2 is allthree changed
ReadFilecalls, one of them inside anEventually-- andthen fails in Phase 3 on an unrelated assertion about EVE rejecting a
broken-DNS DPC and falling back to the previous one. That phase reads
device info from the controller and touches none of this code. I have not
investigated it; flagging it rather than claiming this PR green.
Before the scp fix, both tests failed at the changed calls with the quoted
path error above, so the fix is what the runs actually demonstrate.
Changelog notes
No user-facing changes. Test-framework only.
PR Backports
Checklist
And the last but not least:
check them.
Documentation:
evetest/README.mdis updated for the new signatures. Nottested on arm64 -- the change is arch-independent and the local live image is
amd64.