Repository navigation
fix: return error from installAgentFromImage when extraction fails - #6533
Merged
avorima merged 4 commits intoSep 7, 2026
Merged
Conversation
installAgentFromImage was logging the error from extractAgentBinariesFromImage but not returning it, so InstallAgent always got nil and skipped cleanup of targetDir - leaving a potentially partial installation that isAlreadyPresent would treat as valid on subsequent calls regression from a803e77 which switched to go-containerregistry and dropped the return by mistake
knQzx
requested review from
Edivion,
StefanHauth,
andriisoldatenko,
avorima,
gkrenn,
jdanieck and
svfoxat
May 1, 2026 10:19
|
|
avorima
previously approved these changes
May 27, 2026
avorima
left a comment
avorima
left a comment
Contributor
There was a problem hiding this comment.
Hi, thank you for your contribution.
This looks okay to merge. Omitting the error at this point somewhat obfuscated that multi-layer images are not supported. I've created a follow-up task on our end to improve reporting in that case.
After main was merged in, imageCacheDir is only a struct field on imagePullInfo, not a local variable, so the log line failed to compile
Contributor
Author
|
Pushed a build fix - after main was merged in, imageCacheDir became a field on imagePullInfo rather than a local, so the failure-log line stopped compiling. It now references the CacheDir constant, which is the exact value passed into imagePullInfo, so the log stays accurate. CI should go green again. @avorima the approval was dismissed on the update - could you take another look when you get a chance? |
jokuniew
added
the
community
avorima
removed their request for review
September 7, 2026 11:33
andriisoldatenko
approved these changes
Sep 7, 2026
avorima
enabled auto-merge
September 7, 2026 11:51
0sewa0
approved these changes
Sep 7, 2026
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
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.
description
installAgentFromImagelogs the extraction error but returns nil. soInstallAgentnever sees the error and skips theos.RemoveAll(targetDir)cleanupfor single-layer codemodule images this mostly hides itself - the symlink step fails afterwards on the missing dir and the error still surfaces, just with a misleading log
multi-layer is where it bites. layer 1 renames into targetDir successfully then layer 2 fails. targetDir has partial files. error swallowed. symlink created over the partial dir.
InstallAgentreturns(true, nil). next reconcile seesisAlreadyPresent=trueand skips install forever. silent broken install no auto recoveryroot cause
regression from a803e77 ("Make the image download function use the go-containerregistry library") - the refactor dropped the
return errors.WithStack(err)and changed the log message to "via proxy"fix
put the return back
how can this be tested?
pkg/injection/codemodule/installer/image/...tests pass