Add logMirrorChoice to ctx for log

[QiWang19]

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1881694
This BZ needs change the log level of the image pull location from the debug to info.
Add DockerLogMirrorChoice to types.SystemContext for keeping log level of the physical pull source of images.
Crio could set it to log the image source if it's from a mirror.

Signed-off-by: Qi Wang qiwan@redhat.com

[rhatdan]

LGTM

[TomSweeneyRedHat]

LGTM

[umohnani8]

LGTM

[mtrmac]

I get that this is an “easy quick improvement”, and I don’t have any similarly easy suggestion, but I’m not too happy about a library just logging like this without giving the caller much say in where the log goes, especially on such a frequent path that everyone is going to hit.

Conceptually this probably belongs into the copy.Options.Progress log stream, as a well-typed data structure — but getting the data there would involve changing the ImageSource interface.

[QiWang19]

Could you explain why it would be better to be in copy.Options.Progress log stream, I am not very familiar with it, what is copy.Options.Progress log stream, related with https://github.com/containers/image/blob/master/copy/progress_reader.go?
Will that log show up in crio logs?

[mtrmac]

Yes, that’s the major current producer (+ one event after TryReusingBlob succeeds). The point is to make these things observable by software as an API, not to produce a formatted text that’s impossible to work with because it can change any time.

Notably CRI-O is using that interface already: https://github.com/cri-o/cri-o/blob/8a05a29873456911db9a5c85a8d569e4a0db0ee3/server/image_pull.go#L171

It’s, for the most part, not the job of c/image to provide an UI, but it should allow callers to create an UI. (OTOH, admittedly, copy.Image does create progress bars, and it is expected to write something undefined to copy.Options.ReportWriter.)

“copy.Image is required to write important information to the log stream” just does not feel like a good fit in that context; it’s not a sustainable way to define or maintain an API.

Or maybe the mirror choice could go to ReportWriter, sure. That would make it user-readable but not software-consumable, not as good as Progress but better than just the indiscriminate log.

Either way we’re looking at some kind of API break to ImageSource, something that we do need to be able to do eventually but much more work than this is probably worth — the customers in this case could just bump CRI-O to debug mode for the time necessary to test mirroring (or, well, monitor the network externally), if I understand the situation correctly.

---

I suppose one way to shut me up is to gate this behind a types.SystemContext.DockerLogMirrorChoice opt-in or something like that, making it an ”obscure implementation aspect of the Docker transport that CRI-O specifically knows about”, rather than a design pattern in c/image. OTOH that also could become a very bad precedent.

[QiWang19]

@mtrmac PTAL. Change to use the DockerLogMirrorChoice to avoid send the sys back to cri-o.

[mtrmac]

LGTM. I’m not happy about this but blocking this small improvement on building infrastructure for ImageSource API changes is not proportionate.