[RAPTOR-19519] fix(workload): stop rebuilding the image for runtime-only changes - #854
[RAPTOR-19519] fix(workload): stop rebuilding the image for runtime-only changes#854wojtekwdr wants to merge 1 commit into
Conversation
|
🎫 Jira: |
0975fc8 to
0fb8403
Compare
damjandr
left a comment
There was a problem hiding this comment.
Looks good, put some minor comments.
A couple of them (the nil lastBuiltVersionId default, the untested primaryCodeVersionID) are worth a look before merge — the rest are cosmetic.
| // sync last pushed. A write that fails costs the next deploy a build it might | ||
| // have skipped, which is what every earlier release did anyway, so it must not | ||
| // fail this one. | ||
| func TestRecordBuiltVersion(t *testing.T) { |
There was a problem hiding this comment.
The loop over nil/error writes asserts the same thing both times, so the error arm proves nothing beyond not panicking.
There was a problem hiding this comment.
Restructured into a table that also covers the two states with nothing to record. The failing-write row does still assert the same value, and deliberately: the claim is that the write is attempted and its answer swallowed, so the config has to be captured before the error is returned. recordBuiltVersion returns nothing, so there is no stronger assertion available short of asserting the caller keeps going, which the roll-level test does. Happy to change it if you had something else in mind.
Code OwnershipWorkload Cli
Review requested from the teams above. Labels will be removed automatically upon approval. |
9543358 to
a21f3e3
Compare
…nly changes Changing an environment variable retriggered a full image build. Env vars are runtime configuration: the container reads them when it starts, so the change wants a restart, not a new image. The rebuild was the CLI's own doing. Any artifact spec edit made the run a roll, a roll minted a fresh artifact, and a fresh artifact has no image, so it always built. A port, a probe or a route cost a build the same way. A version that changes only fields a container reads at start is now copied from the one now serving, inheriting its image, the code it was built into and its repository, with the file's spec written over the copy. Those fields are an allow-list rather than a list of build inputs to exclude: the compiler passes keys it does not recognize straight through, so a field the platform grows later would otherwise read as runtime and inherit a stale image. Copying is the only route the platform supports, since it drops a client-supplied imageUri from any container it knows how to build and then refuses to deploy the result. The spec written over the copy carries the copy's own codeRef back in, because the manifest states none and the platform refuses a container that says how to build itself but not from what. The copy is then read back rather than assumed: one that did not take the change, or lost its image to the write, is removed again rather than promoted. Project state gains lastBuiltVersionId, the code version the last image was built from. Nothing on the platform records it, and without it a deploy that inherits an image cannot tell whether `dr artifact code sync` moved the code out from under it. Absent, as it is for every project until its next build, the deploy behaves as before.
a21f3e3 to
34ec46e
Compare
There was a problem hiding this comment.
LGTM — no issues found.
Details
The PR implements a significant optimization: runtime-only changes (env vars, ports, probes, routes) no longer trigger a full image rebuild. Instead, the CLI clones the running artifact version and writes the file's spec over the copy, inheriting the image.Key verification points
- Nil safety:
PrimaryContainerInDocumentcan return nil; all callers handle it safely (Go nil-map reads return zero values, comma-ok type assertions return false).imageStaleandrecordBuiltVersionboth check*stringpointers for nil before dereferencing. - Copy error handling:
copiedVersionrecords the copy ID before the spec update so failures can name and delete the orphan.discardCopyhandles the case where deletion also fails, returning a "left behind" error.copyUnsupportedcorrectly distinguishes 404/405/501 (old platform, fall back to create) from other errors (don't fall back). - Read-back verification:
carriedreads the artifact back after the spec write to verify the change landed and the image survived. A failed read is treated conservatively (no code, no image, matches=true) which triggers a safe rebuild. - Code reference preservation:
keepCodeRefre-injects the copy's owncodeRefinto the spec before writing, because the manifest states none and the platform refuses a container that says how to build itself but not from what. runtimeOnlyallow-list: Uses structuredKeyssegments (not renderedPathstrings) to identify container fields, correctly handling container names containing brackets. The allow-list approach means unknown fields default to rebuild — safe for future platform additions.descendslice isolation:slices.Clone+ append prevents sibling key slices from sharing backing arrays (verified byTestSubset_SiblingKeysDoNotShareStorage).InheritsImagecorrection: After execution,roll.gocorrectsresult.Plan.InheritsImagetofalseif a build happened or an error occurred, so the JSON envelope reports what actually happened rather than what was planned.
All tests pass across all 10 workload packages. Five parallel review passes covered all 23 changed files.
RATIONALE
Changing an environment variable on a workload retriggered a full image build. Env vars are runtime configuration: the container reads them when it starts, so the change wants a restart, not a new image.
The rebuild was ours rather than the server's. Any artifact spec edit made the run a roll, a roll minted a fresh artifact, and a fresh artifact has no image, so it always built. A port, a probe or a route cost a build the same way.
CHANGES
A version that changes only fields a container reads at start is now copied from the one now serving, inheriting its image, the code it was built into and its repository, with the file's spec written over the copy. Everything else still builds. An env-var deploy goes from about 45 seconds to about 25, with no build at all, and the same holds for locked production and for a workload that was stopped.
Three calls worth arguing about.
The runtime fields are an allow-list rather than a list of build inputs to exclude. The compiler passes keys it does not recognize straight through, so a field the platform grows after this release ships would read as runtime under a deny-list, inherit a stale image and deploy clean. Under an allow-list it reads as a rebuild, which costs a build and is always survivable.
Project state gains
lastBuiltVersionId. Nothing on the platform records which code an image was built from: a build row does not name its catalog pair, and an artifact's build pointer is cleared both by a re-sync and by being copied, so the two cannot be told apart. Without it,dr artifact code syncbetween deploys would let a later env-var change inherit an image built from the code before that sync and report success. Absent, which it is for every existing project until its next build, the deploy behaves as it did before.A copy inherits artifact-spec fields the file never mentions instead of dropping them as a create would. That is the one-directional contract
Subsetalready implements, but it is now a difference between the two tracks.Verified against staging across twenty scenarios, including locked production, a stopped workload, an out-of-band
dr artifact code sync,--detach,--force-build, a service-to-agent type change and a manifest bound by artifact id.Note
Medium Risk
Changes the core roll/deploy path (artifact minting, build skipping, and production/locked behavior) with new platform API dependencies; mitigated by conservative runtime allow-list, read-back validation, and broad integration tests.
Overview
dr workload upno longer triggers a full image build when a roll only changes runtime configuration (environment variables, ports, probes, routes, and similar fields read at container start). Those deploys clone the running artifact, PATCH the manifest spec onto the copy (preserving image and code reference), and roll without calling the builder.The deploy planner gains
RebuildsImage/InheritsImageusing an allow-list of runtime fields and structuredChange.Keysso rebuild vs copy is not inferred from rendered paths.Looknow surfaces the live image URI and catalog version; projectlastBuiltVersionIdrecords which code the last successful build used so env-only rolls still rebuild afterdr artifact code sync. New workload API helpersCloneArtifact,UpdateArtifactSpec, andSpecWithPrimaryCodeRefsupport the copy path, with fallback to create-and-build when clone is unsupported or the copy cannot be updated. Plans and JSON output exposekeepsImage/ dry-run messaging when no rebuild is expected.Reviewed by Cursor Bugbot for commit 7dc420d. Configure here.