update c/common to add some netns cleanup fixes

[Luap99]

Does this PR introduce a user-facing change?

None

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[edsantiago]

No joy, sorry. debian root, f40 root, and other jobs still running

[Luap99]

No joy, sorry. debian root, f40 root, and other jobs still running

I still would call this progress. At least the error message is less cluttered with unimportant messages. But yes it seem like whatever root cause there is is not fixed (not surprising as I don't understand the cause at all)
Anyhow I will try to get #23487 ready which should provide me with better logs

[Luap99]

@mheon @edsantiago PTAL

cdcef62 is needed after #23553 bc it doesn't log the netns error otherwise just that it failed to stop containers.

[Luap99]

@mtrmac Looks like your changes broke the tests here, containers/image@f49cb62?

time="2024-08-09T06:35:05-05:00" level=warning msg="Compression using zstd:chunked is not beneficial for encrypted layers, using plain zstd instead"

The test expected an empty stderr. Do we need to fix the test to force zstd?

[mtrmac]

@Luap99 Thanks for pointing me at this.

Per https://github.com/containers/podman/blob/1ab9dd156a35ed1ca7648379d74d7cfb9615699a/vendor/github.com/containers/common/pkg/config/default.go#L358 and https://github.com/containers/podman/blob/1ab9dd156a35ed1ca7648379d74d7cfb9615699a/vendor/github.com/containers/common/pkg/config/containers.conf#L442 , I’d expect this to be defaulting to gzip. So I thought this test failure newly appears only when changing the configuration, I’m sorry.

Are the tests, perhaps, running against the system’s containers.conf? (https://src.fedoraproject.org/rpms/containers-common/tree/rawhide does show rawhide enabling zstd:chunked).

---

If the above is all expected, yes, either change https://github.com/containers/podman/blob/1ab9dd156a35ed1ca7648379d74d7cfb9615699a/test/e2e/pull_test.go#L612 to use non-chunked zstd, or ignore the warning like 0adf8d2 (from #23397 ). The former would be a bit clearer because other warnings would still cause failures.

[Luap99]

Are the tests, perhaps, running against the system’s containers.conf? (https://src.fedoraproject.org/rpms/containers-common/tree/rawhide does show rawhide enabling zstd:chunked).

Yes we use system configs except for the cases where explicitly overwrite certain settings (i.e. composefs on rawhide).

If the above is all expected, yes, either change

Sure fixing the test is easy but if we enable zstd:chunked as default for newer Fedoras than this will throw visible warnings for all users that push encrypted images (likely not many) and you basically force them to overwrite system default settings just to get rid of this warning. Is a warning really needed, should it be dropped to an info level?

IMO "expected" configurations should not throw warnings. There is the case of setting --compression-format explicitly in which case a warning sounds appropriate to me but this isn't something c/image wouldn't know about I guess? So I think we would need to decide which of the two cases we want? Or we move such a warning into the podman fronted where such information is available.

[mtrmac]

In short, I’m leaning towards c/image lowering this to a debug-level log message.

(Reordered)

There is the case of setting --compression-format explicitly in which case a warning sounds appropriate to me but this isn't something c/image wouldn't know about I guess?

Yes, the difficulty is that c/image can’t tell the difference between the setting coming from the CLI, and from containers.conf.

IMO "expected" configurations should not throw warnings.

I agree that it’s undesirable to warn users about the system-shipped default configuration. We might want to warn about a user-customized configuration, but that’s less important (and we can’t tell user’s intent from an unchanged field in any case). A very weak counterargument is that “encrypting images is not ‘expected’”… and that we there is a low, hypothetical, chance that we would add support for the combination in the future.

---

you basically force them to overwrite system default settings just to get rid of this warning.

Yes, that’s the impact; manually use --compression-format zstd to silence.

Is a warning really needed, should it be dropped to an info level?

I think typically users don’t care about compression that much, but zstd:chunked comes with performance promises (“only pulling the changed files”) that users might want. I don’t know.

To confirm, the effect of using “info” level would be to hide it from Podman’s output, wouldn’t it? (Skopeo does include “info” by default.)

If the intent is to hide this, I’d rather go all the way to “debug”. If the intent is to keep it visible, I have no opinion on the severity level of the log message. (If I imagine myself in the users’ shoes, I’d either just ignore the warning, or I’d want to silence it, whether it is called a “warning” or “info”.)

---

So I think we would need to decide which of the two cases we want? Or we move such a warning into the podman fronted where such information is available.

I think if the warning is valuable, it should trigger also for non-Podman callers which haven’t been explicitly updated.

We could over-engineer this, have the caller provide the default compression (in addition to the current “desired compression”) as a new field … but that seems to me to be too much complexity, both increasing the API surface and IIRC Podman / Buildah don’t have a truly centralized location where the c/image SystemContext is created, so we would be playing whac-a-mole.

---

For reference: containers/image#2485

[rhatdan]

Alternatively should podman just not use the format when pushing encrypted images, then we can just document that encrypted images are ignore compression-format?

[mtrmac]

Interesting … there already is a precedent for hard-coding compression decisions in c/common/libimage: https://github.com/containers/common/blob/d93f74f43223d0d20bd7e4d4b29b762ed981b04c/libimage/push.go#L90-L95
I don’t know how I feel about hard-coding these things, but a precedent and consistency are, to me, quite important.

But, then again, libimage, just like c/image, can’t tell the difference between a CLI option and a containers.conf default, because Podman’s CLI layer has a habit of using the containers.conf values as option defaults, erasing the difference:

podman/cmd/podman/images/push.go

Line 140 in 48e727f

flags.StringVar(&pushOptions.CompressionFormat, compFormat, compressionFormat(), "compression format to use")

---

Completely ignoring the config and falling back to gzip is plausible. Ignoring CLI options and not allowing to create zstd-compressed encrypted images at all seems hard to accept (but, ultimately, up to Podman maintainers, not me). So I think this is also blocked by the inability to tell the difference between options on the CLI and in the config file.

[Luap99]

We could over-engineer this, have the caller provide the default compression (in addition to the current “desired compression”) as a new field … but that seems to me to be too much complexity, both increasing the API surface and IIRC Podman / Buildah don’t have a truly centralized location where the c/image SystemContext is created, so we would be playing whac-a-mole.

Agree, I fix the tests to work around that. I guess we will see eventually if users complain or not about this message.

[rhatdan]

Can't we just change the options to conflict. Podman CLI can tell if the user actually changed the CLI. So if
CLI states `encryted and --compression-format we throw an error, otherwise we just document the behaviour.

[Luap99]

Can't we just change the options to conflict. Podman CLI can tell if the user actually changed the CLI. So if CLI states `encryted and --compression-format we throw an error, otherwise we just document the behaviour.

I don't think they conflict, only zstd:chunked conflicts with means we have to do a very narrow specific check

If we have such a warning on the cli level you likely would have to patch buildah and skopeo the same way and then not let c/image log this. But then any other other c/image user faces the problem where c/image drops the compression format without notice which isn't nice either depending on the context.

Regardless none of this blocks this PR as this does a simple vendor and I just fix the test for now. If someone wants to change this we can do so later

[Luap99]

@containers/podman-maintainers PTAL

[mheon]

LGTM

[rhatdan]

/lgtm