feat(computing-unit): pre-pull curated images onto every node - #8485
Open
tanishqgandhi1908 wants to merge 7 commits into
Open
feat(computing-unit): pre-pull curated images onto every node#8485tanishqgandhi1908 wants to merge 7 commits into
tanishqgandhi1908 wants to merge 7 commits into
Conversation
An administrator registers an image reference from a public registry, and a computing unit can then be started from it. Off by default until the UI to manage these ships. Texera reads the image's manifest and config blob -- a few kilobytes, never the layers -- to check its start command runs computing-unit-master, which means it was built FROM the Texera computing-unit image, and to resolve the digest behind the reference. A misspelled, private or unsuitable image is refused in seconds, in front of the administrator, rather than when a user's unit will not start. The row records owner/name@sha256:..., and that is what units run, so a tag its owner moves later cannot change what already ran. Nothing is copied and no registry is added: units pull the reference the same way the deployment's own image is already pulled. Uniqueness is enforced by the database, not only checked in the service. Two administrators registering the same link at the same moment both pass a read-then-write check and produce two rows for one image. A curated image was supplied by an administrator and reviewed by nobody, so a unit started from one runs as a non-root user with no privilege escalation and no capabilities. Curated images only -- the deployment's own image is its operator's choice, and one that has replaced it with an image needing root would break on upgrade. The first unit on each node waits for the image to download, about 80 seconds for a 3 GB one, while later units there start at once. Pre-pulling ready images onto nodes is a follow-up.
The first unit on a node waits for the whole image, about 80 seconds for a 3 GB one, while every later unit there starts at once. Each ready image now gets a DaemonSet whose init container is the image and whose command does nothing, plus a pause container so the node does not reclaim it. Created when an image becomes ready, repointed when a refresh resolves a new digest, and removed with the image. Best-effort: if it cannot start, the image still works and the first unit pays for the pull.
github-actions
Bot
added
ddl-change
Contributor
Author
|
/request-review @aicam |
Contributor
Automated Reviewer SuggestionsBased on the
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 363 | 0.222 | 25,601/40,455/40,455 us | 🔴 +15.1% / 🔴 +150.8% |
| 🔴 | bs=100 sw=10 sl=64 | 775 | 0.473 | 126,052/184,247/184,247 us | 🔴 +30.0% / 🔴 +69.5% |
| ⚪ | bs=1000 sw=10 sl=64 | 897 | 0.548 | 1,108,690/1,195,852/1,195,852 us | ⚪ within ±5% / 🔴 +15.2% |
Baseline details
Latest main ded7ba1 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 363 tuples/sec | 421 tuples/sec | 755.28 tuples/sec | -13.8% | -51.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.222 MB/s | 0.257 MB/s | 0.461 MB/s | -13.6% | -51.8% |
| bs=10 sw=10 sl=64 | p50 | 25,601 us | 22,245 us | 12,957 us | +15.1% | +97.6% |
| bs=10 sw=10 sl=64 | p95 | 40,455 us | 35,425 us | 16,134 us | +14.2% | +150.8% |
| bs=10 sw=10 sl=64 | p99 | 40,455 us | 35,425 us | 20,333 us | +14.2% | +99.0% |
| bs=100 sw=10 sl=64 | throughput | 775 tuples/sec | 811 tuples/sec | 980.1 tuples/sec | -4.4% | -20.9% |
| bs=100 sw=10 sl=64 | MB/s | 0.473 MB/s | 0.495 MB/s | 0.598 MB/s | -4.4% | -20.9% |
| bs=100 sw=10 sl=64 | p50 | 126,052 us | 120,442 us | 101,894 us | +4.7% | +23.7% |
| bs=100 sw=10 sl=64 | p95 | 184,247 us | 141,678 us | 108,718 us | +30.0% | +69.5% |
| bs=100 sw=10 sl=64 | p99 | 184,247 us | 141,678 us | 122,482 us | +30.0% | +50.4% |
| bs=1000 sw=10 sl=64 | throughput | 897 tuples/sec | 919 tuples/sec | 1,011 tuples/sec | -2.4% | -11.3% |
| bs=1000 sw=10 sl=64 | MB/s | 0.548 MB/s | 0.561 MB/s | 0.617 MB/s | -2.3% | -11.2% |
| bs=1000 sw=10 sl=64 | p50 | 1,108,690 us | 1,088,724 us | 996,422 us | +1.8% | +11.3% |
| bs=1000 sw=10 sl=64 | p95 | 1,195,852 us | 1,161,033 us | 1,037,670 us | +3.0% | +15.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,195,852 us | 1,161,033 us | 1,072,152 us | +3.0% | +11.5% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,550.31,200,128000,363,0.222,25600.64,40454.90,40454.90
1,100,10,64,20,2579.17,2000,1280000,775,0.473,126052.00,184247.45,184247.45
2,1000,10,64,20,22287.85,20000,12800000,897,0.548,1108690.04,1195852.49,1195852.49
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8485 +/- ##
============================================
- Coverage 93.94% 93.35% -0.59%
+ Complexity 4827 4826 -1
============================================
Files 1209 1213 +4
Lines 49705 50201 +496
Branches 6074 6135 +61
============================================
+ Hits 46695 46865 +170
- Misses 1522 1835 +313
- Partials 1488 1501 +13
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review found the pre-pull never ran. The pool namespace has a ResourceQuota on requests.cpu and requests.memory, quota admission checks init containers too, and the prepuller declared neither -- so every pod was refused while the DaemonSet was still created, leaving the service logging success and pulling nothing. The chart's own two pre-pullers escape this only by living in the release namespace. Pre-pulls move there with them, with a Role of their own, and both containers now state requests. A pod's request is the larger of its init containers and the sum of the rest, so this stays 1m/8Mi. Also from review: - an image leaving READY now loses its pre-pull, instead of holding gigabytes on every node for something no unit can start from - the reconcile pass compares the reference, not just the id, so a repoint that failed once is retried rather than left pointing at a superseded digest - a listing that failed is no longer read as "nothing is pre-pulled", which had every read fire a doomed create per ready image - delete removes the row before the pre-pull, closing a window where a concurrent read re-created one nothing would reap - turning pre-pulling off removes the pre-pulls already made, which is what frees the disk the setting talks about - no tolerations: a computing-unit pod declares none, so tolerating everything only put images on nodes no unit can be scheduled onto
The previous commit fixed the quota rejection twice over -- it declared requests on both containers and moved the pre-pulls to the release namespace. The requests are what fixed it. The move was the half that did damage: the release namespace holds the privileged hostPath mounter, so permission to write DaemonSets there is permission to run as root on every node. Pre-pulls go back to the pool namespace, whose quota the requests satisfy and which holds nothing privileged, and the grant goes back to being one rule on the Role that was already there. Also from review: - limits are on the pause container only. A limit is enforced per container and never maxed across them, so sharing one object put an 8Mi cap on the image's own shell -- bash, on the Python bases these are built from. It would OOMKill, crash-loop, never reach pause, and leave the pulled image reclaimable, while the DaemonSet reported itself created. - the reconcile pass now removes pre-pulls no ready image wants, rather than only adding. Every other removal path can be interrupted between the database write and the cluster call, and nothing revisited a row that was gone or not ready, so an orphan was unreapable. This also ends the collection delete that ran on every read with pre-pulling off: it is driven by what the listing found, so it stops once there is nothing left.
The reaper added last time kept only READY rows, but a refresh moves a healthy image through VALIDATING first. So refreshing anything deleted its pre-pull from every node and rebuilt it moments later -- and the admin page polls this endpoint for as long as a check is running, so that read was certain to land inside the window. A row still being checked now keeps its pre-pull; only a row that is gone or FAILED is reaped. Also: - a create that fails is not retried for five minutes, keyed by the reference so a new digest is still tried at once. Reconciling runs on every read of the list, by any signed-in user, so a failure that will not clear on its own -- the Role not reapplied after an upgrade -- was one doomed call and one stack trace per ready image per page load, indefinitely. - the reference reaches finishValidation from the row the reconcile already read, rather than a second query. A row deleted in between left pinnedRef building "@sha256:..." with no repository at all, and a DaemonSet that could never pull it. - deletePrepull states its propagation policy, as the validation client's deletes already do. Were the default ever Orphan, the pods would stay on every node holding the image, and the reconcile pass lists DaemonSets, so nothing would find them again.
Two from review, plus a pass over the comments. A repoint inherited the default rolling update, one node at a time, each waiting for a full pull -- hours on a large cluster, for a pod with no availability to protect. maxUnavailable is now 100%. Refreshing is the only remedy the page offers for a pre-pull that could not be created, but with the digest unchanged the cooldown swallowed it, so the button appeared to do nothing for five minutes. Starting a validation now clears the image's recorded failure. Comments trimmed throughout: they had grown into accounts of what an earlier version did wrong rather than what the code does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this PR?
The first unit on a node waits for the whole image — about 80 seconds for a 3 GB one — while every later unit there starts at once. The same action takes seconds or minutes depending only on which node it landed on.
Each ready image now gets a DaemonSet: an init container that is the image, with a command that does nothing, plus a pause container so the node does not reclaim what was just pulled. The same mechanism the chart already uses for the deployment's own image, built in code because a curated image is registered while the cluster is running, so a template cannot describe it.
One per image, keyed on the image id, so a refresh that resolves a moved tag repoints the existing pre-pull rather than adding a second one holding bytes nothing runs. Removed when the image is.
Three moments, because the service has no background threads: an image reaching READY, an image being deleted, and a read of the list — which also gives a pre-pull to any ready image that has none, since nothing else revisits a row once it is ready.
Best-effort throughout. A pre-pull that cannot be created is logged and ignored: the image still works, and the first unit on each node just pays for the pull.
Costs node disk, since every node holds every ready image, so
curatedImages.prepull.enabledturns it off.Any related issues, documentation, discussions?
Closes #8469
Part of #8466
Stacked on #8475, which this calls into, so GitHub shows that commit here too until it merges.
How was this PR tested?
Seven new tests for the DaemonSet the code builds, plus the object applied to a real cluster.
IfNotPresent— a digest cannot resolve to different bytes laterExists, so tainted nodes are not the ones left waitingapponly — a DaemonSet's selector is immutable, so it must hold nothing that changeshelm templaterenders the manager Role gainingdaemonsetsand the two new environment variables.On minikube, applying the object the code actually builds:
The refresh case is the one worth having: a DaemonSet's selector cannot be changed after creation, so a selector carrying anything mutable would make every refresh fail permanently.
Also confirmed against the cluster rather than assumed: deleting a pre-pull that does not exist returns an empty result instead of throwing, so an image that never reached READY is a silent no-op rather than a warning on every delete.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)