evetest: cache EVE disk image templates and support locally built live images - #6260
evetest: cache EVE disk image templates and support locally built live images#6260rucoder wants to merge 4 commits into
Conversation
|
Claude finished @rucoder's task in 5m 20s —— View job Review completeI focused on the broker-side image-template cache, GPT/config-partition handling, and the local-live-image upload path. Overall this is careful, well-documented code with the trust boundaries (tar path traversal, sha256 verification, hex/ref-name validation, GPT bounds/overflow checks) already handled. Two things worth a look, posted as inline comments:
Findings
Everything else — the single-flight build, tar staging/rename atomicity, config-partition injection mirroring |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| diskHasher := sha256.New() | ||
| tr := tar.NewReader(f) | ||
| for { | ||
| hdr, err := tr.Next() |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| return nil | ||
| } | ||
| dir := c.templateDir(key) | ||
| if err := os.RemoveAll(dir); err != nil { |
There was a problem hiding this comment.
The refcount that guarantees "a template backing a live VM is never deleted" is not actually mutually excluded against reference acquisition. evict, hasRefs, candidates, addRef and removeRef never take c.mutex (only loadInstalled does), yet evict runs on the cleanup goroutine while addRef runs concurrently on BuildImage gRPC handlers.
So there is a TOCTOU window:
ensureTemplatereturns a cachedref(template currently unreferenced).- cleanup calls
evict(key)→hasRefsreads an emptyrefs/dir. makeDeviceImagecallsaddRef→ writes the ref marker.evictproceeds toos.RemoveAll(dir), deleting the marker anddisk.qcow2thataddRefjust returned success for.
makeDeviceImage then builds the overlay backed by a now-deleted template disk → the device's VM has a missing backing file. The re-check in evict closes the common ordering but not the interleaving where the WriteFile lands between hasRefs's ReadDir and RemoveAll.
The window is tiny (cleanup fires every 30 min) and this is a test broker, so severity is low, but the strongest safety claim of the design rests on this being race-free. Consider serializing addRef/removeRef/hasRefs/evict under c.mutex.
| return nil, false | ||
| } | ||
| } | ||
| meta.LastUsed = time.Now() |
There was a problem hiding this comment.
loadInstalled performs a synchronous file write (meta.save, to update LastUsed) on every cache lookup, and it runs while c.mutex is held, so a slow disk write serializes all concurrent template lookups. It is also reached from hasTemplate, which reads as a side-effect-free predicate (called in the BuildImage precondition) but in fact rewrites meta.json and mutates on-disk LastUsed. Minor for a handful of devices, but the write-under-lock and the hidden mutation in a has… check are both surprising; consider updating LastUsed outside the lock (or only from the acquire path, not the probe).
|
Thanks — both findings were real. Fixed in 0827bbd. 1. Refcount vs. the cleanup sweep. Confirmed, and worth separating from a window an earlier review had already looked at. That review closed the stale snapshot case by having It is not theoretical. The regression test added here ( The fix uses a dedicated 2.
|
0827bbd to
9d5d0b4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6260 +/- ##
==========================================
+ Coverage 24.13% 24.48% +0.34%
==========================================
Files 512 522 +10
Lines 93537 95253 +1716
==========================================
+ Hits 22575 23322 +747
- Misses 69177 69961 +784
- Partials 1785 1970 +185 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude i addressed your previouse comments. Review agin |
|
Claude finished @rucoder's task in 1m 31s —— View job Re-review completeI re-reviewed the two fixes in response to your update.
One minor residual (inline): The CodeQL "Zip Slip" flag on |
| if c.hasRefs(key) { | ||
| log.Warnf("EVE image template %q is unusable (%s) but is still referenced by a "+ | ||
| "working copy; leaving it in place", key, reason) | ||
| return | ||
| } | ||
| dir := c.templateDir(key) | ||
| if err := os.RemoveAll(dir); err != nil { |
There was a problem hiding this comment.
Residual (minor) of the same race the evict fix closed: discardUnusable does hasRefs (takes+releases refsMutex) and then os.RemoveAll(dir) outside refsMutex, so an addRef can still land between the check and the delete. Unlike evict, this is far lower risk — discardUnusable only fires on a template deemed corrupt/incomplete, which wouldn't normally be handed back to makeDeviceImage as a usable ref for a concurrent addRef — but the check-then-delete pattern here isn't guarded the way the design's "never deleted while referenced" invariant otherwise now is. For consistency, consider folding the hasRefs check + RemoveAll into one refsMutex critical section (hasRefsLocked), exactly like evict.
refsMutex, the touch param, and the documented lock ordering all look correct otherwise.
| return fullCapabilitySet() | ||
| caps := fullCapabilitySet() | ||
| if p.DiskImageStrategy() != DiskImageLegacyBuild { | ||
| caps = append(caps, api.Capability_CAPABILITY_LOCAL_LIVE_IMAGE) |
There was a problem hiding this comment.
I do not think this belong to the provider.
Provider does not build the image, it is done in the provider-agnostic part of the broker.
And actually, does it even make sense to use this capability when libvirt or proxmox providers are used? They (usually) run on a remote machine, away from your eve repo. In my opinion, it makes sense to enable this only for the qemu mode (when everything including broker is inside the evetest container)
There was a problem hiding this comment.
@milan-zededa yes, it does. this is exactly how i use libvirt broker today: I build local live image - it is faster than make eve , and then upload local image to broker
| } | ||
|
|
||
| // DiskImageStrategy returns DiskImageLegacyBuild. Proxmox can never use | ||
| // DiskImageOverlay because uploadDiskImages ships the image to the PVE node, |
There was a problem hiding this comment.
For proxmox we could also just save the template as a file, then run uploadDiskImages only for config-overlayed disks.
After all, providers do not prepare the VM image. This is done by the provider-agnostic part of the broker. So when provider receives call to setup a new device, the qcow image is already prepared, including the config partition (and then proxmox provider calls uploadDiskImages).
There was a problem hiding this comment.
@milan-zededa but what if we want to use native proxmox template feature? it cannot be provider agnostic
There was a problem hiding this comment.
So do we want to use the proxmox template feature? What is the added value as opposed to having one solution for all providers? Let's make that decision before expanding the provider API. I intentionally want to keep it minimal and avoid 3 different solutions for one problem if possible.
| }) | ||
| } else { | ||
| // No EVE container image need exist on this broker at all on the live | ||
| // path -- that is the point of the feature -- so its content ID is |
There was a problem hiding this comment.
Please remove these AI-generated "-- that is the point of the feature --" comments (there are two instances at least)
| | `EVETEST_BROKER_DOCKER_IMAGE_RETENTION` | How long, in minutes, an unused, evetest-managed Docker image (one the broker itself pulled or built) is kept before the broker's periodic cleanup removes it | `10080` (7 days) | | ||
| | `EVETEST_BROKER_DOCKER_DISK_USAGE_THRESHOLD` | Disk usage percent (on the filesystem backing Docker's storage) at or above which the broker aggressively evicts the oldest unused, evetest-managed Docker images, regardless of the retention setting above | `80` | | ||
| | `EVETEST_BROKER_TEMPLATE_RETENTION` | How long, in minutes, an unused EVE disk-image template (see [The EVE Image Template Cache](#the-eve-image-template-cache)) is kept before the broker's periodic cleanup removes it. Deliberately generous, since templates let consecutive runs against the same EVE version skip the image build entirely; zero or negative disables age-based eviction, but disk-usage-based eviction still applies regardless. A template still backing a live VM is never removed regardless of this value | `10080` (7 days) | | ||
| | `EVETEST_BROKER_TEMPLATE_DISK_USAGE_THRESHOLD` | Disk usage percent (on the filesystem backing the broker's image directory) at or above which the broker evicts the oldest unreferenced EVE image templates, regardless of the retention setting above. Deliberately higher than `EVETEST_BROKER_DOCKER_DISK_USAGE_THRESHOLD`: broker hosts routinely idle above 80% (the reference host sits at 83% with 150 GB free), so an 80% threshold would evict every unreferenced template on every pass and the cache would never stay warm; templates are also the wrong thing to give up first -- one is 1-2 GB, where the Docker image store is tens of GB | `90` | |
There was a problem hiding this comment.
Please remove "(the reference host sits at 83% with 150 GB free)"
| | `EVETEST_NAME` | Test or suite name to run (**required**) | -- | | ||
| | `EVETEST_OUTPUT_FORMAT` | `go test` output format: `json` (machine-readable, for `gotestfmt`) or `quiet` (compact, no `-v`); default is verbose (`-v`). **Do not combine `quiet` with `EVETEST_PAUSE_ON_FAILURE` or `EVETEST_PAUSE_ON_CHECKPOINT`** — without `-v`, `go test` buffers all output until the test completes, so a pause appears frozen with no visible output. | -- | | ||
| | `EVETEST_EVE_VERSION` | EVE version to test | current repo HEAD | | ||
| | `EVETEST_EVE_LIVE_IMAGE` | Test against a locally built live image instead of an EVE container image. Empty/unset uses the container path; the literal `current` resolves to `dist/<arch>/current/live.qcow2`; any other value is a path to a qcow2. See [Testing a Local EVE Build](#testing-a-local-eve-build) | -- | |
There was a problem hiding this comment.
Does this override EVETEST_EVE_VERSION?
What is the effect on for example TestEVEUpgrade. Does this override only the target image, while we continue the legacy image built for EVETEST_INITIAL_EVE_VERSION?
There was a problem hiding this comment.
@milan-zededa good question. I'll double check
There was a problem hiding this comment.
For now I suggest to:
- not override EVE version explicitly requested by a test. Apply the local image override only when this
eveVersionis empty string: https://github.com/lf-edge/eve/blob/master/evetest/setup.go#L96-L99 - add a check to
TestEVEUpgrade(in the area where we fetch the parameter values) and mark the test as failed ifEVETEST_EVE_LIVE_IMAGEis defined with error like "Live image override is not supported for the upgrade test..."
|
Good catch — I checked, and it is broken. Worse than just overriding
eveVersion := dev.requirement.WithEVEVersion // per-device, explicit
if eveVersion == "" {
eveVersion = viper.GetString(constants.EVEVersionEnv)
}and my block then does
So with
To your questions directly: yes it overrides The minimum correct behaviour is that a local live image must not hijack a device that explicitly asked for a particular EVE version — such a device wants a specific released build and the local one is not it. I will make that device fall back to the container path rather than fail, so Making the local build serve as the upgrade target is the genuinely useful version of this, and is what an image reference in I will fold this in with your other points. |
ca0df2e to
852bfdc
Compare
687ed8d to
20e7177
Compare
|
|
||
| // DiskImageStrategy returns DiskImageLegacyBuild. The qemu provider attaches | ||
| // local files and could use DiskImageOverlay, but that has not been validated | ||
| // here yet. |
There was a problem hiding this comment.
Should be easy to validate, just run any test without broker IP defined.
qemu and proxmox are the priority for us. The qemu provider is used for running tests locally, while proxmox will be used for CI.
The broker built one EVE disk image per device by running the EVE container image: roughly four minutes and ~2 GB of work each, serialized. A three-node cluster test paid that three times, every run. Build a configuration-independent template once per distinct (docker image content ID, disk size, installer flag, arch), cache it under templates/<key>/, and give each device a qcow2 copy-on-write overlay backed by it. The device's own 5 MiB FAT config partition is assembled and written into the overlay afterwards, reproducing what pkg/eve/runme.sh does with mcopy onto /bits/config.img, so device configuration never enters the cache key and a warm cache serves every device. Templates are reference counted, so one backing a live VM is never deleted; an image-directory flock makes a second broker sharing the directory read-only for housekeeping; and unused templates are evicted by age and by disk pressure. Providers that have not been validated against this keep the per-device build path, selected by a new per-provider disk image strategy. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
Teardown matched a bare "<nvram>" tag, but libvirt emits the element with attributes -- format='raw' on this host -- so the check never fired, DOMAIN_UNDEFINE_NVRAM was never passed, and every undefine failed with "cannot undefine domain with nvram". The domain was destroyed but its definition survived. Device names are stable within a suite so that consecutive tests reuse the same VMs, so the next test failed to create them with "already exists"; 32 leaked definitions had built up on the shared broker before this was noticed. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com> (cherry picked from commit d5079ed)
Running a test required an EVE container image: ~6.6 GB pushed to the broker and a ~4 minute container run there to produce a disk. But local EVE development already produces that disk -- make live writes dist/<arch>/current/live.qcow2 alongside installer/config.img and the OVMF firmware, which is everything a template needs. EVETEST_EVE_LIVE_IMAGE=current points evetest at that build. The harness content-hashes the image and uploads it, with config.img and firmware, only when the broker does not already hold that hash; the tar is streamed ungzipped because qemu-img convert -c has already compressed the qcow2's clusters. The broker installs it through the template cache's existing builder parameter, so single-flight, atomic install, reference counting, overlays, config injection and eviction are shared with the container path rather than duplicated. Per-device disk sizing is applied by resizing the overlay, leaving the shared backing file untouched. Measured on one broker running TestSingleNodeCluster: 2.1 GB transferred in 19 s and a template ready in 9 s, against 6.62 GB and 3 m 56 s for the container path; an unchanged image on a rerun transfers nothing. Whether an uploaded live image can be consumed is the broker's determination, not a device provider's -- a provider attaches disks and runs VMs, and never builds or receives an image. The broker composes the advertised capability set, deriving CAPABILITY_LOCAL_LIVE_IMAGE from the provider's disk image strategy, so an outdated broker or an unsuitable provider fails clearly instead of silently building from a different EVE image. A device requesting a specific version via RequireEdgeDevice.WithEVEVersion keeps taking the container path: it wants a particular released build, and TestEVEUpgrade depends on that field to boot its pre-upgrade version while applying the upgrade target through a separate path. An installer image cannot be produced from a live qcow2, so that combination is rejected rather than falling back. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
The traversal guard checked the archive entry name; checking the joined destination against the target directory instead rules out escape for certain and is what static analysis can verify. Signed-off-by: Mikhail Malyshev <mike.malyshev@gmail.com>
20e7177 to
65f7a64
Compare
3 participants
Description
The evetest broker built one EVE disk image per device by running the EVE container image: roughly four minutes and ~2 GB of work each, serialized. A three-node cluster test paid that three times, every run.
This adds two related things.
An EVE image template cache. The broker now builds a configuration-independent template once per distinct (docker image content ID, disk size, installer flag, arch), caches it under
$EVETEST_BROKER_IMAGE_DIR/templates/<key>/, and gives each device a qcow2 copy-on-write overlay backed by it. The device's own 5 MiB FAT config partition is assembled and written into the overlay's CONFIG partition afterwards, reproducing exactly whatpkg/eve/runme.shdoes withmcopy -o -i /bits/config.img -s /in/* ::/, so device configuration never enters the cache key. Templates are reference-counted so one backing a live VM is never deleted, guarded by an image-directoryflockso two brokers sharing a directory cannot destroy each other's state, and evicted by age and disk pressure. Providers that have not been validated against it (qemu, proxmox) keep the existing per-device build path unchanged.Support for a locally built EVE live image. During EVE development
make livealready produces the disk the broker spends four minutes recreating.EVETEST_EVE_LIVE_IMAGE=currentpoints evetest atdist/<arch>/current/live.qcow2; the harness content-hashes it and uploads it — withinstaller/config.imgandinstaller/firmware/*— only when the broker does not already hold that hash. The broker installs it as a template through the same machinery as above, so overlays, config injection, reference counting and eviction are shared with the container path rather than duplicated.Measured on the same broker running
TestSingleNodeCluster:A locally built qcow2 cannot produce an installer flow, so a request combining
make_installerwith a live image is rejected rather than silently falling back to a container build — as is a provider that builds per device. Support is advertised asCAPABILITY_LOCAL_LIVE_IMAGE, so an outdated broker fails with a clear message instead of quietly testing a different EVE build.Additional fix: NVRAM detection in libvirt teardown
The third commit fixes a separate, pre-existing bug in the libvirt provider's teardown that this work made impossible to ignore.
libvirt.godecided whether a domain had NVRAM withstrings.Contains(xmlDesc, "<nvram>")— a bare tag. Libvirt emits the element with attributes (<nvram format='raw'>/…/OVMF_VARS.fd</nvram>on the test host), so the check never fired,DOMAIN_UNDEFINE_NVRAMwas never passed, and every undefine failed withcannot undefine domain with nvram.The domain was destroyed but its definition survived. Device names are deliberately stable within a suite so consecutive tests reuse the same VMs, so the next test failed to create its devices:
How to test and validate this PR
Container path (unchanged behaviour):
Expect
Building EVE image templateon the first device,Reusing cached EVE image templateon subsequent devices with the same image, and one template directory under the broker's image dir.Local live image path:
Expect one upload and
Installing EVE image template ... from an uploaded live image. Rerun the same command: expect zero uploads andReusing cached EVE image template. Rebuild withmake liveand rerun: the new content hash produces exactly one new upload.Both paths were validated end to end against a remote libvirt broker: device boots from the overlay and onboards into the Adam controller, which exercises the injected config partition (server address and onboard certificate) rather than merely proving the image boots.
Changelog notes
No user-facing changes. Test-framework only (
evetest/); nothing underpkg/or the EVE rootfs is touched.PR Backports
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
I've checked the boxes above, or I've provided a good reason why I didn't check them.
Not tested on arm64: the change is broker-side image plumbing with no architecture-specific code paths, and the validation broker is amd64.