Relocate the run image to the builder repo #1533
Conversation
tomkennedy513
commented
Feb 21, 2024
tomkennedy513
commented
- Also make sure the builder is tagged with the name and kind if a tag is not provided
tomkennedy513
changed the title
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #1533 +/- ##
==========================================
- Coverage 67.19% 67.14% -0.06%
==========================================
Files 140 140
Lines 8827 8868 +41
==========================================
+ Hits 5931 5954 +23
- Misses 2389 2404 +15
- Partials 507 510 +3 ☔ View full report in Codecov by Sentry. |
| @@ -70,7 +71,6 @@ func (bb *builderBlder) AddStack(baseImage v1.Image, clusterStack *buildapi.Clus | |||
| bb.os = file.OS | |||
| bb.baseImage = baseImage | |||
| bb.stackId = clusterStack.Status.Id | |||
| bb.runImage = clusterStack.Status.RunImage.Image | |||
There was a problem hiding this comment.
Is clusterStack.Status.RunImage.Image fully qualified?
There was a problem hiding this comment.
Ya it's the resolved image
There was a problem hiding this comment.
It looks like it just the spec runImage which is not necessarily fully qualified.
kpack/pkg/cnb/remote_stack_reader.go
Lines 71 to 74 in 03acfbb
There was a problem hiding this comment.
I think that was intentional. The run image shouldn't necessarily be fully qualified. (E.g if you are using the builder with pack and the run image was updated it is useful to make use of that run image)
There was a problem hiding this comment.
I suppose here, it needs to be relocated by kpack so, it is not necessarily useful.
There was a problem hiding this comment.
Which I guess exposes the downside with this approach. It is only really useful if the source of the stack is not available otherwise, it provides a worse experience.
There was a problem hiding this comment.
Ya I guess the trade off is full build reproducibility though which a floating tag doesn't guarantee
| if err != nil { | ||
| return buildapi.BuilderRecord{}, err | ||
| } | ||
| runImage, _, err := r.RegistryClient.Fetch(stackKeychain, clusterStack.Status.RunImage.LatestImage) |
There was a problem hiding this comment.
Would it be possible to only save the image if it is in a different registry than the builder?
There was a problem hiding this comment.
I was thinking you might still run into a case where the pull rules could be different for a different repo
There was a problem hiding this comment.
Fair. There is a trade-off from having the run-image be relocated everywhere, though.
There was a problem hiding this comment.
I think I'm ok with just checking the registry and saying you need to be able to pull both. Maybe putting the original image in the run image mirror would also help as a backup
There was a problem hiding this comment.
Thinking more about this, i'm seeing less of an issue with always relocating. In the event that the run image is in the same registry we will just cross repo blob mount. It will also tag the run image to prevent it from being being cleaned up. If the run image is in a different repo the value is much more apparent since you don't have to worry about run image access.
There was a problem hiding this comment.
I was thinking the downside is more about more confusing provenance. It is useful to see that the run image is my-registry.com/my-stack/specific-stack-name/run vs my-registry.com/some-team/custom-builder/run.
but, I agree it is not a fatal flaw.
There was a problem hiding this comment.
ya that is definitely a concern, but I was hoping the tags on the relocated run image would simplify this. It also makes registry management a bit easier so you can easily see the run image associated with a particular builder
- Also make sure the builder is tagged with the name and kind if a tag is not provided Signed-off-by: Tom Kennedy <ktom@vmware.com>
tomkennedy513
force-pushed
the
relocated-run-image
branch
from
581ec0a to
948eb18
Compare
