RPMAnalzyer: download absent remote:// images
#164
Conversation
|
Ping @nkubala |
differs/rpm_diff.go
Outdated
| } | ||
| if err := client.PullImage(pullOpts, godocker.AuthConfiguration{}); err != nil { | ||
| return packages, fmt.Errorf("Error pulling remote image: %s", err) | ||
| } |
There was a problem hiding this comment.
should we do a logrus.Debug(buf.String()) here? otherwise why pass a buffer to the OutputStream?
| type ImageType int | ||
|
|
||
| const ( | ||
| ImageTypeTar ImageType = iota |
differs/rpm_diff.go
Outdated
| @@ -123,6 +123,20 @@ func rpmDataFromContainer(image pkgutil.Image) (map[string]util.PackageInfo, err | |||
|
|
|||
| defer client.RemoveImage(imageName) | |||
| defer logrus.Infof("Removing image %s", imageName) | |||
| } else if image.IsCloud() { | |||
| // if it's a remote image and not in the daemon, we need to pull it | |||
| if _, err := client.InspectImage(imageName); err != nil { | |||
There was a problem hiding this comment.
if err == nil here, we should probably log and exit yeah? otherwise we have an invalid image and this will fail somewhere else down the line
There was a problem hiding this comment.
Could you elaborate on why the image would be invalid in such case? If err == nil then I would expect the tool to use the already present image. I agree that it should be logged.
update: Another option could be to pull unconditionally, which would update the local images in case they're outdated. I can imagine this to be especially useful in automation. What do you think?
There was a problem hiding this comment.
Ah sorry, I confused myself with the error cases here. Disregard my original comment.
I think it might make sense to specify an option to users (through a flag or something) to always pull, but I don't think we want to do that by default. One of the major use cases is comparing locally built images with their remote counterparts, and I don't think we should assume that users will always use image tags appropriately :) either way this is outside the scope of this PR imo, but feel free to file a separate issue to track this
The RPMAnalyzer runs all analyses in a container, which requires not only to unpack the tarball, but also to pull the referenced image into the daemon in case it's not present. However, we cannot blindly pull a remote image as the Docker daemon would overwrite a potentially present image with the same tag. Instead, pull the image via its sha256 digest, which is computed via the OCI libs, just as Skopeo does. Signed-off-by: Valentin Rothberg <vrothberg@suse.com> Fixes: #162
|
@nkubala I went a slightly different path. Now, we can unconditionally pull a remote image. To avoid a tag/name conflict with an already present image, we cannot use However, I am still not a 100 percent satisfied with the current state, as we are now downloading an RPM remote image two times. Once for unpacking it into |
| // log PullImage() output in debug mode | ||
| if logrus.GetLevel() == logrus.DebugLevel { | ||
| // log each line separately for consistency | ||
| for _, out := range strings.Split(buf.String(), "\n") { |
There was a problem hiding this comment.
does the output stream not already have newlines in it? just wondering why this is necessary.
There was a problem hiding this comment.
Yes, the output stream has newlines. The issue here is that logrus will not prepend the log prefix to a new line, so only logging buf.String() would look like:
[level][counter] first line
second line
third line...
I find that very confusing as it looks broken, so I usually split strings to print each array entry separately.
|
@vrothberg thanks for all your work on this. Pulling all images by digest should be fine as far as tags in the local daemon go, so this approach looks good. I agree that pulling the image twice for RPM diffs isn't optimal, but I don't think it'll be too much an issue right now; let's go ahead and open an issue to rework the image pulling at a later date. I suspect we should be able to use the containers/image library (what skopeo is built on and what container-diff uses in a few places) to do this. |
|
@nkubala Thanks, I totally agree with you. |
The RPMAnalyzer runs all analyses in a container, which requires not
only to unpack the tarball, but also to pull the referenced image into
the daemon in case it's not present.
Signed-off-by: Valentin Rothberg vrothberg@suse.com
Fixes: #162