Repository navigation
feat(agent): adopt what the command seeded, and leave it until then (MK8S-434) - #26
ezekiel-alexrod wants to merge 12 commits into
Conversation
ezekiel-alexrod
added
agent
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
db2e1f4 to
36ea193
Compare
| // replaceable reports whether the swap may remove what is at final: only a | ||
| // directory with a sentinel, whoever wrote it. imagecachectl runs as root, so | ||
| // a wrong name or cache path must not become an rm -rf of someone else's data. |
There was a problem hiding this comment.
Agree with the comment but it shouldn't apply for the agent, I mean if there is a folder without sentinel files the agent should remove everything and replace the full folder content
Btw what happen in case of removal of the sentinel file ? To me it should "just" re-sync everything (of course you lose everything not managed by the agent but .... it's fine to me it's should be the actual behavior
There was a problem hiding this comment.
Agreed, it's done in 4a4744e. Before, if the sentinel was missing, the resource stayed pending until someone removes the directory by hand. Now the agent replaces the directory of its resource even without sentinel, so if you delete the sentinel, it re-syncs. But I did it only if the directory contains only regular *.tar files, like a cache image, not "remove everything": if the cache path is one level too high and a resource is named rpm, we don't want to remove the rpm database. imagecachectl still refuses: it's run by hand, so a wrong name is more a typo. Ok for you?
There was a problem hiding this comment.
Update: I went further than the *.tar rule, like you asked first. The cache path belongs to image-cache, so the agent now replaces anything at its resource path without sentinel: a directory with any content, a file, or a link (only the link, not what it points to). imagecachectl still refuses to replace anything without sentinel, it stops with an error. The history is rewritten, it's in 61fdf3b now.
| Digest string `json:"digest"` | ||
| Files []string `json:"files"` | ||
| Record | ||
| Files []string `json:"files"` |
There was a problem hiding this comment.
I'm not sure to get it so it means if a file exists we consider it's the right one ? What if someone edit/put a file by hand ? If the file has the right name it will be considered as the right one ?
That's not good to me we should hash every file and check it (most likely in another watcher + JSON file exactly like we do in metalk8s-registry-node-agent (maybe not part of this PR)
There was a problem hiding this comment.
Yes, today the completeness check looks only at the file names. So if someone edits a file by hand and keeps the name, it passes. To hash all the files at each pass is expensive, so I would do like metalk8s-registry-node-agent: a watcher and a JSON file with the checksums. To me it's out of this PR, I would open a ticket for it, with your label idea below, because the expected digests would come from there. Ok for you?
| // Layers are the image's layer diff IDs, as puller.Image reports them. | ||
| Layers []string `json:"layers,omitempty"` |
There was a problem hiding this comment.
Thinking about it maybe a better option could be to have a JSON in some labels in the meta image that would contains {<file name>: <hash>} and that would always be the source of truth and the "only" things pulled by default (it would be part of ConfigFile)
Example build of image (🤖 )
# Dockerfile
FROM scratch
COPY *.tar /
ARG CACHE_CONTENT
LABEL com.scality.image-cache.content="${CACHE_CONTENT}"content=$(for f in *.tar; do
printf '{"name":"%s","digest":"sha256:%s","size":%s}\n' \
"$f" "$(sha256sum "$f" | cut -d' ' -f1)" "$(stat -c %s "$f")"
done | jq -sc .)
docker build --platform linux/amd64 \
--build-arg CACHE_CONTENT="$content" \
-t "$ref" .Then we would no longer require those layers and this label would be used also to handle file that would have been updated by hand (even we this to me we still need the watcher that compute the checksum of file on disk like metalk8s-registry-node-agent as I mentioned in my other comment)
This JSON would be stored in the sentinel file as well, we can discuss about it if you want but I think it's a better option
There was a problem hiding this comment.
I like the idea, mostly for the integrity: we get the digest of each file from the config, without pulling a layer, and it survives a Docker to OCI conversion. Note that the adoption already pulls no layer today: it reads only the manifest and the config, where the diff IDs are. The cost is a new contract for the image: the MetalK8s buildchain must set the label, and we must decide what the agent does with an image without it. I prefer to do it in a follow-up, with the check on disk, than to make this PR bigger. We can discuss it if you want.
|
|
||
| // maxEventNames bounds the names an event lists. The API server refuses an | ||
| // event note longer than 1024 characters, and the recorder does not cut it. | ||
| const maxEventNames = 10 |
There was a problem hiding this comment.
Ten names don't keep the note under 1024 characters: an entry name can be 255 bytes, and the note also carries the cache path. With a few long foreign names (e.g. 10 × 100-character directories) the API server rejects the ForeignCacheEntries event and the recorder only logs it, so the warning this PR adds never shows on the Node. Bound the joined length too (stop adding names once the note would pass ~900 bytes, then say "and N more").
The agent and the command ran the same steps in the same order: check the cache path, pull the image, close the stream on the way out, extract it. They differed only in how they reported. The next changes touch what an extraction records, and two copies would have had to move together. Both now call internal/fill. Two small differences fall out of it. The agent refuses a cache path that is a regular file, as the command did, instead of failing later on the extraction. And it logs a failure to close the stream only once the extraction went through, as the command did: before that, the error it returns already says what happened. Relates-to: MK8S-434
One agent writes one cache path, given by --cache-path like imagecachectl. It defaults to /var/lib/image-cache. A path per resource made the agent keep in memory a set of paths to scan, so an orphan on a custom path survived a restart, and two spellings of one path had to be cleaned to the same key. With one path, garbage collection scans one directory. The agent image was never published, so no object needs migrating. Relates-to: MK8S-434
Pull returned the manifest digest only. It now returns an Image: the manifest digest and the diff IDs of the layers. Resolve returns the same without reading a layer: for a registry, the manifest and the configuration, a few kilobytes. The diff IDs do not depend on how the image is stored: an archive saved from the Docker daemon and the same image in a registry report the same ones, while their manifest and configuration digests differ (TestBothPullersAgreeOnTheLayers). The next commits record them and compare by them. Relates-to: MK8S-434
The sentinel listed the files and a digest nothing read back. It now also records the owner, the source the content was read from, and the diff IDs of the layers. The agent writes image-cache-agent as owner and the command imagecachectl, so that the agent can tell a directory it wrote from one seeded before it arrived. Relates-to: MK8S-434
imagecachectl runs as root and leaves root-owned 0700 directories, so the agent, UID 65532, could not read what it seeded. A chown at pod start would also miss anything seeded while the agent runs. The binary now carries cap_dac_override as a file capability, and the pod keeps DAC_OVERRIDE in its bounding set. Adding it to the pod alone does nothing: a non-root process starts with an empty effective set, measured on kind with containerd 1.7.18. DAC_READ_SEARCH is not enough, the agent writes and removes there. The chown init container goes. Relates-to: MK8S-434
Garbage collection removed every sentinel-bearing directory no resource claimed, and it runs on every pass, the first one included. At install the agent can land before the resources that name what imagecachectl seeded, so it removed that cache and pulled it again: about a gigabyte, with the node holding none of the images in between. The cache path now belongs to image-cache. Garbage collection removes every entry no resource keeps, whoever wrote it: a directory with no sentinel or a damaged one, a link (never its target), an interrupted extraction. Only these stay: - regular flat files: provisioning writes them, and the preload service restores from them; - a directory whose sentinel names another writer, or no owner; - lost+found, when the cache path is a filesystem of its own; - a directory whose sentinel cannot be read: the pass fails until it can be read. Relates-to: MK8S-434
A resource that claims a directory imagecachectl seeded took it as it was: nothing checked that it held the image the resource names. It now stays pending until the agent has checked. The agent resolves the resource's source, which reads no layer, and compares the diff IDs of the layers, in order, with the ones the command recorded: - the same image: the agent takes the directory over, rewriting only the owner in the sentinel, and pulls nothing; - another image: the directory is replaced like any other that does not hold what its resource asks for; - the source cannot be resolved: the directory is left as it is, the resource stays pending, and the next pass tries again. The sentinel is rewritten through a temporary file renamed over it, so a crash never leaves a half written sentinel. A complete directory whose sentinel cannot be read is left alone for that pass. Relates-to: MK8S-434
The store checked whether it could replace a resource's directory at the swap only, after the whole image had been pulled and extracted. A directory the agent could not take over was therefore pulled again on every pass: on a kind cluster, with a seeded directory the agent could not read, about 950 layer pulls in a minute and a half. Fill now asks the store first, through Store.Replaceable, and pulls nothing when the answer is no. Extract still checks at the swap. A sentinel that cannot be read is also reported as such, with its cause, instead of as a directory the store did not write: the refusal then points at who owns the directory, not at what it holds. Relates-to: MK8S-434
When the sentinel of a resource was deleted by hand, the resource stayed pending until someone removed the directory. The agent owns the cache path, so it now replaces whatever sits at its resource's path without a sentinel: a directory, a file or a link, never what the link points to. A file or a link loop there reads as an incomplete resource, not as an error. imagecachectl still refuses: it is run by hand, and a wrong name there is a typo. Relates-to: MK8S-434
remote.WithPlatform only picks a child out of an index. A tag that points at a single arm64 manifest was pulled and extracted on an amd64 node, and the sentinel said the resource was complete. The archive path already checked the configuration. The check now lives in identify, so both sources share it, adoption included. Relates-to: MK8S-434
GC removed every `.<name>.tmp-*` directory, even one an import was still writing. On a node where the agent runs, the import creates its temporary directory, the watcher wakes the agent, and the pass removes it: the import fails with "no such file or directory". The writer now holds a flock on its temporary directory until it is done, and GC and SweepTemporaries skip a locked one. The kernel drops the lock with the process, so a crashed run is still cleaned up. Relates-to: MK8S-434
eb00348 to
b86c1c3
Compare
The watcher watched the temporary directory of an extraction, and each write in it raised an event. While imagecachectl import extracted an image, an idle agent ran thousands of passes, one per write. An agent that extracts runs only one more pass, since its own pass holds the single worker meanwhile. A pass reads the sentinel and checks that the files it lists exist, so a write to any other file cannot change its result. The watcher now skips temporary directories and ignores writes, except to a sentinel. The rename into the final directory is still seen on the cache path. Relates-to: MK8S-434
3 participants
Component
agent, config, docs
Problem
At install the agent can land before the
ImageCacheresources. Its GC then deletes whatimagecachectlseeded, and the agent pulls the same gigabyte again. And when a resource claims a seeded directory, nothing checks it holds the right image.Fix
Read the commits in order: each one builds, passes and adds one step. 1 is a refactor, 2 replaces
spec.cachePathby a flag, 3 and 4 prepare (image identity, sentinel record), 5 to 7 are the core (permissions, GC, adoption), 8 to 12 are fixes found on the way. I'd mostly like eyes on the GC rules (GCinagent/internal/cache/store.go) and on the adoption (adoptinagent/internal/controller/node_reconciler.go). "Cache layout" and "Adopting a seeded directory" inagent/DESIGN.mddescribe both.spec.cachePathis gone. The agent takes its one cache path from--cache-path, likeimagecachectl. The agent image was never published, so no object needs migrating.lost+foundand a directory whose sentinel can't be read.TestBothPullersAgreeOnTheLayers).DAC_OVERRIDE, and the chown init container goes.imagecachectlstill refuses.flockon its temporary directory, and the GC skips a locked one.Test
go test -race,go vetandgolangci-lint(Go 1.26.0), on every commitmake -C agent test-e2eon kind: root-owned seeded directory kept, temporary, damaged sentinel and directories with no sentinel collectedOut of scope
README.md,agent/README.md,DESIGN.md,agent/DESIGN.md,CONTRIBUTING.md, whichever owns it.agent/DESIGN.md. The preload service only globs*.tarand never reads the sentinel, so onlyagent/DESIGN.mdchanges.Relates-to: MK8S-434