Eden to evetest: rewrite networking, security, apps, lps and storage tests - #6244
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Claude finished @milan-zededa's task in 3m 45s —— View job Code review
I reviewed the production-code changes and the framework/library additions in depth, and scanned the test rewrites. This is a well-structured, thoroughly-commented PR. Production code (the parts with user-facing impact) look correct:
One latent bug I flagged inline (framework code): the OCI-datastore image-ref split in Other things I checked and consider fine:
No security or correctness concerns in the pillar changes. Nice work. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
593b7d5 to
c5ad210
Compare
|
Claude finished @milan-zededa's task in 2m 12s —— View job Re-review
The one finding from my previous pass is resolved and I found nothing new that blocks. Summary below. Fix confirmed — Production code (pillar) — re-verified correct:
Framework additions — checked, no issues:
Not re-raising: the CodeQL No security or correctness concerns. LGTM. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6244 +/- ##
==========================================
+ Coverage 24.12% 24.47% +0.35%
==========================================
Files 512 522 +10
Lines 93594 95310 +1716
==========================================
+ Hits 22576 23330 +754
- Misses 69232 70012 +780
- Partials 1786 1968 +182 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
7f6e110 to
79a4d78
Compare
81ffe76 to
09567f0
Compare
eriknordmark
left a comment
eriknordmark
left a comment
There was a problem hiding this comment.
Yetus reports
| -1 | golangcilint | 8m 17s | The patch generated 24 new + 538
| | | | unchanged - 0 fixed = 562 total (was
| | | | 538)
but the summary and annotations appear to be showing unchanged files with yetus issues. @rene I see this with yetus on other PRs as well - as soon as there is a -1 vote we end up with a summary and annotations which do not relate to the diffs in the PR.
Needs to be rebased and conflicts resolved - hence "changes requested".
Also, how soon can we get the running of these tests into the EVE CI? And can we make that be included in the code coverage so we don't show zero coverage as we do for this PR?
e65b78d to
e7b6f78
Compare
milan-zededa
changed the title
bcde1eb to
cf53db6
Compare
|
@eriknordmark @rene
We do not have enough hardware resources to run both eden and evetest. So we need to first complete the migration of tests from eden to evetest. The only remaining (after this PR) are smoke tests and one storage test, which Rene, Andrew and Paul are taking care of. Once that has completed, we can migrate to evetest. |
| // published to a real, reachable registry -- see | ||
| // PushDockerImageToLocalRegistry. | ||
| func newLocalRegistryHandler() http.Handler { | ||
| return registry.New() |
There was a problem hiding this comment.
registry.New() in v0.14.0 hardcodes the in-memory blob handler, so the pushed image (an lfedge/eve image for the OCI upgrade variant -- hundreds of MB) stays in the harness heap for the whole process. TestUpgradeSuite runs all variants under a single Init, so it is still resident during the later ones.
Bumping go-containerregistry to >= v0.19.1 gets you registry.WithBlobHandler(registry.NewDiskBlobHandler(dir)), which would put the blobs under th.imgServerDir instead. Probably worth doing anyway now that the module is a direct dependency -- v0.14.0 is from early 2023.
| // content tree) directly from evetest, without that image ever having been | ||
| // published to a real, reachable registry -- see | ||
| // PushDockerImageToLocalRegistry. | ||
| func newLocalRegistryHandler() http.Handler { |
There was a problem hiding this comment.
registry.New() defaults all of its loggers to log.New(os.Stderr, ...), so every manifest/blob request is printed outside logrus and outside the artifact dir, interleaved with go test output.
registry.Logger() exists in v0.14.0 -- passing one backed by the harness logger fixes it (needs a logger argument here).
There was a problem hiding this comment.
Now it logs like this:
HARNESS time="2026-08-06T12:55:18Z" level=debug msg="OCI Registry: GET /v2/lfedge/evetest-ubuntu-ctr/blobs/sha256:bfb639d8897b6a35afa63552a945cf1e821a2bd4722ae9c3f0d01f19e8e62664"
HARNESS time="2026-08-06T12:55:18Z" level=debug msg="OCI Registry: GET /v2/"
HARNESS time="2026-08-06T12:55:18Z" level=debug msg="OCI Registry: GET /v2/lfedge/evetest-ubuntu-ctr/blobs/sha256:ad8d8f2cc1c08a1b0f5ead34e57d34e28e7b9eb51e2a82c1cb8f3ef7bfb2d6a5"
HARNESS time="2026-08-06T12:55:18Z" level=debug msg="OCI Registry: HEAD /v2/lfedge/evetest-ubuntu-ctr/blobs/sha256:bfb639d8897b6a35afa63552a945cf1e821a2bd4722ae9c3f0d01f19e8e62664"
HARNESS time="2026-08-06T12:55:18Z" level=debug msg="OCI Registry: HEAD /v2/lfedge/evetest-ubuntu-ctr/blobs/sha256:1d2c5bc98e8170a8e5ce3776edba944097c26bc35fe7c52aa2a2ed905746325d"
| th := getTestHarness() | ||
| log := th.log.WithField("component", "local-registry") | ||
|
|
||
| repo, tag, ok := strings.Cut(imageName, ":") |
There was a problem hiding this comment.
strings.Cut splits at the first colon, whereas the code this replaces in UpgradeEVE used strings.LastIndex deliberately. A repo carrying a registry port (harbor.example.com:5000/lfedge/eve:1.2.3-kvm-amd64) now yields repo="harbor.example.com".
LastIndex alone would not be enough either -- the re-tag below would then produce 10.x.x.x:80/harbor.example.com:5000/.... Parsing with name.ParseReference and re-tagging from .Context().RepositoryStr() handles both, and name is already imported.
eriknordmark
left a comment
eriknordmark
left a comment
There was a problem hiding this comment.
Should we do something about the yetus issues against evetest? They might not have been introduced by this PR (and we have an issue with the summary/annotations being all over the place), but makes sense to take a look at the summary in https://github.com/lf-edge/eve/actions/runs/31085972149?pr=6244
Kick off regression tests for good measure.
cf53db6 to
ad8ef52
Compare
I will address them at some point, but in a separate PR (there are 140 issues raised for evetest, mostly |
|
The yetus issue is related to an error with bufcompat plugin:
We use
That's why 2 images are generated, the plugin only supports check against one... I will open a PR with the fix, we should point only to |
I tried to fix this some time ago, but could never avoid the -1 for bufcompat. Maybe your agent will do better job :) |
|
Yetus fix: #6276 |
|
@milan-zededa Yetus fix is merged. You can rebase if you want. |
ZInfoApp was not reporting all NTP servers for an application's network instance: NetworkInstanceConfig.NtpServers (the NI's own raw configured list) and NetworkInstanceStatus.NTPServers (the combined NI+port list zedrouter actually maintains) differed only by case, so the reporting code in zedagent silently picked up the wrong one via field promotion through the embedded config struct. Renamed the status field to CombinedNTPServers to make the two unambiguous by name, and fixed zedagent to publish it. Signed-off-by: Milan Lenco <milan@zededa.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
When a DHCP server's GatewayIPv4 is left nil (the documented way to make clients not install a default IPv4 route), the config renderer simply omitted the dhcp-option=option:router line instead of explicitly suppressing it. dnsmasq's own default, absent an explicit override, is to advertise its own listening address on that subnet as the router, so clients still received one -- defeating the purpose of leaving GatewayIPv4 unset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Milan Lenco <milan@zededa.com>
Add the EdgeDevice/EdgeCluster/harness capabilities needed by the eden-to-evetest test rewrites: - Flow-log streaming: AdamClient.IterateDeviceFlowLogs/ SubscribeToDeviceFlowLogs, the corresponding gRPC server iterator, and the `evetest eve flow-logs` CLI subcommand. - EdgeDevice.GetArch, PowerOff, PowerOn and WaitForClusterNodeIsReady. - EdgeDevice.DialViaSSH (an SSH-tunneled net.Conn, used for reaching the Kubevirt VNC proxy which only binds to the device's loopback). Also removes the now-unused SkipIfHypervisorKubevirt test-parameter helper; its call sites move to an inline `if hypervisor == evetest.HypervisorKubevirt` check in the test-suite rewrite commit that follows. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Milan Lenco <milan@zededa.com>
…pgrade) Port the corresponding eden E2E scenarios to the evetest Go framework across networking (flow logs, DPC fallback/failover, intermittent connectivity, mgmt traffic routed via an app-based NAT gateway, and several smaller fixes), security (AppArmor status, vcomlink TPM-over-vsock), apps (VNC console access, app purge), LPS and upgrade. Adds the TwoMgmtPortsWithPublicNTP and MgmtViaAppTopology network models used by the rewritten tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Milan Lenco <milan@zededa.com>
Rewrite eden's storage/volume test suites as evetest tests under
evetest/tests/storage/:
- tests/eclient/testdata/mount.txt -> TestMountedVolumes
- tests/eclient/testdata/disk.txt -> TestExtraDiskAttach
- tests/volume/testdata/volumes_test.txt -> TestVolumes
- tests/zfs/testdata/state_and_layout_check.txt -> TestZFSDiskLayout
- tests/volume/testdata/volume_sftp.txt -> merged into
TestSFTPDatastore (tests/networking/datastore_test.go), since it
overlapped with the new datastore tests below
Additionally add datastore tests eden has no equivalent for at all:
TestHTTPDatastore, TestHTTPSDatastore and TestContainerRegistry,
exercising EVE's HTTP/HTTPS/container-registry datastore code paths
end to end (download, checksum verification, and the corresponding
negative paths: bad checksum, bad credentials, untrusted certificate,
missing object/tag).
To support all of the above, extend the evetest framework itself:
- EdgeDeviceConfig gains a standalone (app-unreferenced) volume
lifecycle -- AddVolume/AddBlankVolume/DeleteVolume -- and a Mounts
field on ApplicationInstanceConfig to attach such volumes to an
app, either as a mounted directory or (with an empty MountDir) as
a raw block device.
- RequireEdgeDevice gains ExtraDisks to provision additional virtio
disks via the broker, needed for the ZFS layout test.
- evetest's built-in image server gains HTTPS and SFTP listeners
(harness.go, new sftpserver.go) alongside its existing plain HTTP
server, so the new datastore tests are fully self-contained.
- New CreateBlankImageFile/CreateRandomImageFile helpers generate
test image content (real qemu-img disk images and random-content
files, respectively) served by the built-in image server.
Signed-off-by: Milan Lenco <milan@zededa.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both BaseOSDatastoreOCI upgrades and TestContainerRegistry previously
required their target image to already be published to a real, externally
reachable registry (Docker Hub), so they broke for an unpublished local/dev
build and depended on the device having Internet access.
- Add an embedded OCI registry (go-containerregistry's pkg/registry,
in-memory), mounted at "/v2/" on the harness's existing image-server
listeners alongside the HTTP/HTTPS/SFTP servers -- no new port or
certificate needed (localregistry.go).
- PushDockerImageToLocalRegistry pulls an image into the local Docker daemon
if not already present, then republishes it there. EVE is pointed at the
registry's HTTPS listener, trusting the harness's own CA via a new
DockerContainer.TrustedCACertsPEM field (mirrors
HTTPStorage.HTTPSTrustedCACertsPEM, wired into DatastoreConfig.DsCertPEM).
- UpgradeEVE's OCI branch now publishes to this registry instead of assuming
the target image is already published elsewhere, and fails fast with a
clear error if BaseOSDatastoreOCI and the live-image transport
(EVETEST_EVE_LIVE_IMAGE) are both requested together -- one wants a
container image, the other a disk image, so the combination is
unsupported.
- TestContainerRegistry now publishes its test image
(lfedge/evetest-ubuntu-ctr:1.0) through the same registry instead of
pulling it from the real Docker Hub, and no longer needs
RequireInternetConnectivity{}.
- go.mod/go.sum: promotes go-containerregistry to a direct dependency
(already present transitively). Uses its tarball package plus the
already-used moby/moby/client for reading local images, rather than
go-containerregistry's own daemon package, which would have pulled in a
second, separate Docker client library and a much larger dependency bump.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
Each harness process gets its own img-cache-* directory under $HOME/.evetest, normally removed in Close(). A container killed before Close runs (SIGKILL, docker stop without a graceful shutdown) skips that cleanup and leaks it forever. Init now removes any such directory whose mtime is older than 3 days before creating its own. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Milan Lenco <milan@zededa.com>
Log Close/Remove errors instead of discarding them in adam.go, localregistry.go and sftpserver.go, and make Close() not return an unused error. Also fix a possible nil-pointer dereference flagged in buildMountRefs by adding explicit continue statements after Fatalf. Signed-off-by: Milan Lenco <milan@zededa.com>
- Store registry blobs on disk (th.imgServerDir) via
registry.NewDiskBlobHandler instead of keeping every pushed image
resident in the harness process's memory for the life of the test
binary. This matters most for TestUpgradeSuite, which runs every
upgrade variant under a single Init and pushes a full EVE rootfs
(hundreds of MB) each time. Bump go-containerregistry to v0.21.9 for
registry.WithBlobHandler/NewDiskBlobHandler (added in v0.19.1);
v0.14.0 was from early 2023, so go mod tidy also pulls in the
resulting transitive dependency updates.
- Route the registry's manifest/blob request logging through the
harness logger (debug level, since it logs every request) via
registry.Logger, instead of registry.New's default of printing
straight to stderr, outside logrus and outside the artifact dir.
- Parse PushDockerImageToLocalRegistry's imageName with name.NewTag
instead of splitting on the first ":", so a registry host that
carries its own port (e.g.
"harbor.example.com:5000/lfedge/eve:1.2.3-kvm-amd64") is still split
into the correct repository ("lfedge/eve") and tag, rather than
mistaking the port for the tag separator.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
ad8ef52 to
673c126
Compare
@rene Rebased |
|
Yetus passed. My bad, I forgot the small changes on pillar as well and just merged. @milan-zededa will keep an eye on the tests. |
5 participants
Description
Rewrites a batch of Eden E2E test scenarios (networking, security, apps, LPS, upgrade, storage)
as native Go tests in the
evetestframework, plus some framework/library enhancementsand one SDN bugfix needed to support them.
Commits
zedrouter, zedagent: rename NI status field to CombinedNTPServersNetworkInstanceConfig.NtpServers(the NI's own raw configured list) andNetworkInstanceStatus.NTPServers(the combined NI+port list zedrouteractually maintains) differed only by case, so zedagent's reporting code
silently picked up the wrong one via field promotion through the embedded
config struct. Renamed the status field to
CombinedNTPServersand fixedzedagent to publish it.
evetest/sdn: fix dnsmasq router option not being suppressedA DHCP server config with
GatewayIPv4left nil (the documented way tomake clients skip installing a default IPv4 route) simply omitted the
dhcp-option=option:routerline instead of explicitly suppressing it.dnsmasq's own default, absent an explicit override, is to advertise its
own listening address on that subnet as the router, so clients still
received one anyway.
evetest: framework/library enhancementsAdds the
EdgeDevice/EdgeCluster/harness capabilities the testrewrites below need: flow-log streaming (
IterateDeviceFlowLogs,SubscribeToDeviceFlowLogs, the gRPC server iterator, and theevetest eve flow-logsCLI subcommand),EdgeDevice:GetArch,PowerOff,PowerOn,WaitForClusterNodeIsReady,and
EdgeDevice.DialViaSSH(an SSH-tunnelednet.Conn, used to reachthe Kubevirt VNC proxy which only binds to the device's loopback).
Also removes the now-unused
SkipIfHypervisorKubevirthelper (all testsnow support running at-least single-node EVE-K).
evetest: rewrite eden test suites (networking, security, apps, lps, upgrade)The actual eden -> evetest ports:
of EVE networking.
TestAppArmorEnabled(kernel AppArmor status) andTestVCom(vcomlink TPM-over-vsock request/response from inside aguest VM).
TestVNC(console access, VM and container apps) and apppurge.
profile switching, radio silence).
evetest: port eden storage tests, add new datastore coverageRewrite eden's storage/volume test suites as evetest tests under
evetest/tests/storage/:
- tests/eclient/testdata/mount.txt -> TestMountedVolumes
- tests/eclient/testdata/disk.txt -> TestExtraDiskAttach
- tests/volume/testdata/volumes_test.txt -> TestVolumes
- tests/zfs/testdata/state_and_layout_check.txt -> TestZFSDiskLayout
- tests/volume/testdata/volume_sftp.txt -> merged into
TestSFTPDatastore (tests/networking/datastore_test.go), since it
overlapped with the new datastore tests below
evetest: run an embedded OCI registry for container-image tests
Both BaseOSDatastoreOCI upgrades and TestContainerRegistry previously
required their target image to already be published to a real, externally
reachable registry (Docker Hub), so they broke for an unpublished local/dev
build and depended on the device having Internet access.
evetest: sweep stale img-cache-* dirs left by killed containersThis leaves only the "smoke" test suite as not yet ported from eden to evetest.
How to test and validate this PR
Changelog notes
Fixed a bug where a network instance's device-level NTP servers (learned
from DHCP or configured directly on the port) were not included in the
NTP server list reported to the controller for apps attached to that
network instance -- only the network instance's own explicitly
configured NTP servers were. Apps now see the full, correct combined
list.
All other changes are test-only (evetest framework additions and eden
test-suite rewrites) plus one SDN test-harness bugfix; no other
user-facing EVE behavior changes.
PR Backports
Checklist