Make inspect compatible with docker v1.44 API #21601

flobz · 2024-02-10T11:39:17Z

Hello,
Small changes to make podman compatible with last change in docker API.

Does this PR introduce a user-facing change?

config.entrypoint in inspect command result return an array instead of a string
config.StopSignal in inspect command result return a string instead of int

packit-as-a-service · 2024-02-10T12:06:28Z

Cockpit tests failed for commit aa3b6c4dd519f8cbd7e7aff0d80df3781e7d9847. @martinpitt, @jelly, @mvollmer please check.

rhatdan · 2024-02-10T12:10:29Z

@flobz Thanks

Could you add a test to make sure the new format sticks.
test/system/030-run.bats for example.

martinpitt · 2024-02-10T14:46:23Z

Even though through my uninitiated eye the patch here shouldn't directly affect {{.NetworkSettings.Ports}}, this breaks the cockpit-podman tests:

AssertionError: '5000/tcp:[{ 6000}]' not found in 'map[5000/tcp:[{0.0.0.0 6000}] 5001/udp:[{127.0.0.1 6001}] 8001/tcp:[{0.0.0.0 45355}] 9001/tcp:[{127.0.0.2 41347}]]\n'

At least the recent landed and other open PRs were all green, so it's plausible that this change is the cause. Of course that needs adjustment on our side.

containers/podman#21601 changes the output for "bind port to all addresses" from "" to "0.0.0.0". Adjust the test to accept either format.

martinpitt · 2024-02-10T15:09:35Z

Hang on, the difference is exactly the "0.0.0.0" for "all addresses", so it indeed exactly matches the change here. I sent cockpit-project/cockpit-podman#1565 to fix this.

This PR needs at least one more force push anyway to fix the "Validate" test, by then the c-podman fix is hopefully merged.

Thanks!

containers/podman#21601 changes the output for "bind port to all addresses" from "" to "0.0.0.0". Adjust the test to accept either format.

martinpitt · 2024-02-10T15:53:13Z

The cockpit-podman test adjustment landed, so please either force-push or retry the "cockpit-revdeps" tests. Thanks!

flobz · 2024-02-10T16:58:50Z

@rhatdan what do you think of 6e0e9e9c956035361c6fb4cf339c8c151ec7d2d7 ?

Luap99

Can you link to any docker v1.44 changes that this addresses? These changes here directly touch the podman inspect output and AFAICT these differences are not new in v1.44.

The entrypoint and signal change seem simple enough but it will break compatibility between v4 and v5 which is fine given v5 contains breaking changes but it will make the API much less compatible.

Also the hostip change breaks a lot of tests and I am not sure if it is really needed?

Luap99 · 2024-02-12T10:44:56Z

test/python/docker/compat/test_containers.py

@@ -302,3 +309,33 @@ def wait_and_start():
        finally:
            ctr.stop()
            ctr.remove(force=True)
+
+    def test_container_inspect_compatibility(self):


This is not really an API test as you check podman inspect output against the docker schema which is not a bad idea at all but the API test is checking the output/behavior of our API but I am also not sure if there is a better place right now.

Do you need to do this in python? I think if we want to validate this it would be better in a go unit test.

Apparently there is a module for go: https://github.com/go-openapi/validate.
But it's not compatible with openapi v3, so if docker change the format from v2 to v3 it wont work anymore.
If you think it's mandatory to do this test in go can you tell me where I should add it ?

Luap99 · 2024-02-12T10:45:53Z

test/python/docker/compat/test_containers.py

+        ctr = self.docker.containers.create(image="alpine", detach=True)
+        try:
+            spec = yaml.load(
+                requests.get(f"https://docs.docker.com/reference/engine/v{DOCKER_API_COMPATIBILITY_VERSION}.yaml").text,


loading this every time via network is flaky, you can get this from vendor/github.com/docker/docker/api/swagger.yaml I think.

Yes, please use the vendored copy. You may want to add a comment (and perhaps even an assertion) on spec['info']['version'] being 1.44, to help maintainers track down possible future failures

edsantiago

Thank you for your PR!

A few concerns inline. And still many broken tests, I assume you can track those down.

I'm a ~~little~~ lot alarmed that our validation tests didn't catch the changes in tools/vendor. Have added a TODO item for me to investigate.

edsantiago · 2024-02-12T12:36:23Z

libpod/util.go

@@ -235,8 +235,12 @@ func makeInspectPorts(bindings []types.PortMapping, expose map[uint16][]string)
 			for i := uint16(0); i < port.Range; i++ {
 				key := fmt.Sprintf("%d/%s", port.ContainerPort+i, protocol)
 				hostPorts := portBindings[key]
+				var host = port.HostIP


even though the scope here is minuscule and unambiguous, perhaps hostIP would be a clearer variable name

edsantiago · 2024-02-12T12:44:26Z

test/e2e/create_test.go

@@ -482,7 +482,7 @@ var _ = Describe("Podman create", func() {
 		inspect.WaitWithDefaultTimeout()
 		data := inspect.InspectContainerToJSON()
 		Expect(data).To(HaveLen(1))
-		Expect(data[0].Config).To(HaveField("StopSignal", uint(15)))
+		Expect(data[0].Config).To(HaveField("StopSignal", "SIGTTIN"))


I'm not sure where this came from? SIGTTIN is 21, which is 0x15, but I still don't understand this change?

I fix that, it was SIGTERM not SIGTTIN. The new api return signal as string not int.

edsantiago · 2024-02-12T12:51:13Z

test/python/docker/compat/test_containers.py

+        ctr = self.docker.containers.create(image="alpine", detach=True)
+        try:
+            spec = yaml.load(
+                requests.get(f"https://docs.docker.com/reference/engine/v{DOCKER_API_COMPATIBILITY_VERSION}.yaml").text,


Yes, please use the vendored copy. You may want to add a comment (and perhaps even an assertion) on spec['info']['version'] being 1.44, to help maintainers track down possible future failures

packit-as-a-service · 2024-02-12T19:34:03Z

Ephemeral COPR build failed. @containers/packit-build please check.

flobz · 2024-02-20T12:58:50Z

I could use some help to fix last tests.
Machine test doesn't works on my computer so it's hard to fix them...

Luap99 · 2024-02-21T15:55:18Z

I could use some help to fix last tests. Machine test doesn't works on my computer so it's hard to fix them...

The issue is that you hard break a stable API type (which is fine as it is 5.0), however this breaks new clients that talk to a v4 server (which is the case here). We tried to avoid breaking the API this time around so breaking this is really undesirable.
At the very least we should add a custom Unmarshaller interface for both breaking fields to ensure the old format can still be parsed by the new client.

mheon · 2024-02-21T17:00:44Z

Sorry, but I think this is going to miss our internal 5.0 breaking change deadline (tomorrow AM). This wasn't communicated publicly; in the future, we'll try to get this posted on Github.

We can still consider this but it will likely have to wait until 6.0 (early 2025)

flobz · 2024-02-22T06:34:55Z

At the very least we should add a custom Unmarshaller interface for both breaking fields to ensure the old format can still be parsed by the new client.

That's what docker-py does for docker, it's a good idea.

We can still consider this but it will likely have to wait until 6.0 (early 2025)

That's a pity in podman should be as much as possible compatible with docker to be able to replace it, so incompatibility with new docker API is worst than incompatibility with podman v4 in my opinion.
I faced this issue using testcontainer a well known test framework, so it's mean that until 2025 I should use docker for this use case.
I assume that a lot of other docker dependant project won't work too with podman...

rhatdan · 2024-02-22T10:25:08Z

Can you add the unmarshal call @Luap99 asked for?

flobz · 2024-02-22T20:59:16Z

@Luap99 @rhatdan it's done, I add a custom Unmarshaller.

flobz · 2024-02-23T08:22:36Z

Also the hostip change breaks a lot of tests and I am not sure if it is really needed?

For test container it's, because the lib need to know if the container listen on ipv4 or v6

Luap99

The issue is that it still break compatibility with v4 clients talking to a v5 server, i.e.. the inspect API will return the wrong type even if the /v4 endpoint is called by the client.

I think like the unmarshal hack we would need the same hack for marshal and then actually do a version check in the API endpoint to return the correct thing.

However this change is very late in the cycle a we sort of committed to no more breaking changes as @mheon mentioned so I am not sure if we actually want to go through with this for 5.0.

Luap99 · 2024-02-26T10:24:11Z

libpod/define/container_inspect.go

+
+	switch entrypoint := aux.Entrypoint.(type) {
+	case string:
+		insp.Entrypoint = []string{entrypoint}


this would need to at least split the string by spaces, just dumping everything into argv0 is wrong. Not that splitting by spaces would be 100% correct either but it should be better.

Luap99 · 2024-02-26T10:30:39Z

pkg/signal/signal_common.go

+	if err != nil {
+		signalStr = strconv.FormatUint(uint64(s), 10)
+	}
+	return fmt.Sprintf("SIG%s", signalStr)


this looks odd, in case of an error it would return SIG2 for example. Is this really what docker does? To me it the more logical thing would be to return just the number without the prefix.

I don't know how docker handle unknown signal. I change this to return only the number as you ask.

Signed-off-by: Florian Bezannier <[email protected]>

…r API v1.44 Signed-off-by: Florian Bezannier <[email protected]>

Signed-off-by: Florian Bezannier <[email protected]>

flobz · 2024-02-28T18:54:15Z

Well then this change is incorrect. No listen ip == we listen on dualstack assuming the network supports that. Blindly adding 0.0.0.0 to say it only listens on v4 makes no sense then and would be incorrect. I know podman ps also does this today but it really is not correct if you interpret the value like that.

Yes I agree, on computer with Ipv6 only it won't work but it's a marginal scenario and it's better to support one scenario than none ? You can see this as a default value but the ip shouldn't be empty in the first place don't you think ? I can create an issue for that if you want ?

The issue is that it still break compatibility with v4 clients talking to a v5 server, i.e.. the inspect API will return the wrong type even if the /v4 endpoint is called by the client.

There is no test for that right now right ? It seems to be a too big task for me. Furthermore as I said earlier incompatibility with new docker API is worst than incompatibility with podman v4 in my opinion.

packit-as-a-service · 2024-02-28T19:41:06Z

Cockpit tests failed for commit 2b2207e39e3473d495cbfbce80f3d11921410dda. @martinpitt, @jelly, @mvollmer please check.

martinpitt · 2024-02-29T07:51:36Z

The failure above was once again the podman build eternal hang in issue #21504. But that was on F40, which still has kernel 6.8.0-0.rc4.20240212git716f4aaa7b48.35.fc40.x86_64, and this is apparently only fixed in rc6. As F40 will be frozen for a while, we'll have to endure these failures until then 😢

rhatdan · 2024-02-29T16:51:34Z

LGTM

TomSweeneyRedHat · 2024-02-29T18:04:42Z

@Luap99 are you still around to take another peak at this?

TomSweeneyRedHat · 2024-02-29T18:05:22Z

@mheon OK to hit the "Approve and Run" button here at this point?

mheon · 2024-02-29T18:39:20Z

After discussion with the team, we're going to merge this. This will be the last breaking change for 5.0.

mheon · 2024-02-29T18:39:25Z

/lgtm

mheon · 2024-02-29T18:39:35Z

/approve

openshift-ci · 2024-02-29T18:39:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flobz, mheon

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~~ [mheon]

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

openshift-ci bot added the release-note label Feb 10, 2024

martinpitt mentioned this pull request Feb 10, 2024

test: Adjust for podman inspect change for "all addresses" cockpit-project/cockpit-podman#1565

Merged

flobz force-pushed the feature/docker_v1.44_compat branch from 87d798d to 6e0e9e9 Compare February 10, 2024 16:53

Luap99 reviewed Feb 12, 2024

View reviewed changes

edsantiago suggested changes Feb 12, 2024

View reviewed changes

flobz force-pushed the feature/docker_v1.44_compat branch from 93ec6a5 to f50f520 Compare February 12, 2024 19:27

Luap99 added breaking-change A change that will require a full version bump, i.e. 3.* to 4.* 5.0 labels Feb 13, 2024

flobz force-pushed the feature/docker_v1.44_compat branch from b5d80ae to 5fc08db Compare February 22, 2024 20:57

flobz force-pushed the feature/docker_v1.44_compat branch 2 times, most recently from 77f12e8 to 718fe56 Compare February 23, 2024 07:53

flobz force-pushed the feature/docker_v1.44_compat branch 3 times, most recently from b75f487 to eabf932 Compare February 24, 2024 09:37

Luap99 requested changes Feb 26, 2024

View reviewed changes

flobz force-pushed the feature/docker_v1.44_compat branch from 042fe2c to 2b2207e Compare February 28, 2024 18:44

flobz added 8 commits February 28, 2024 19:45

feat: make inspect compatible with docker v1.44

de845a5

Signed-off-by: Florian Bezannier <[email protected]>

tests: add docker api compatibility test

2cd72c2

Signed-off-by: Florian Bezannier <[email protected]>

chore: rename var host hostIP

23d6377

Signed-off-by: Florian Bezannier <[email protected]>

tests: fix after make podman inspect cmd return compatible with Docke…

facad62

…r API v1.44 Signed-off-by: Florian Bezannier <[email protected]>

feat: allow compatibility with podman v4 machine

585af03

Signed-off-by: Florian Bezannier <[email protected]>

ci: fix module not found

1de94dc

Signed-off-by: Florian Bezannier <[email protected]>

refacto: unknown signal return signal number without prefix

90e6478

Signed-off-by: Florian Bezannier <[email protected]>

fix: split string Entrypoint by space

77bcf23

Signed-off-by: Florian Bezannier <[email protected]>

flobz force-pushed the feature/docker_v1.44_compat branch from 2b2207e to 77bcf23 Compare February 28, 2024 18:45

flobz requested review from Luap99 and edsantiago February 28, 2024 18:54

openshift-ci bot assigned mheon Feb 29, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 29, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 29, 2024

openshift-merge-bot bot merged commit 98a9aee into containers:main Feb 29, 2024
93 checks passed

edsantiago mentioned this pull request Mar 4, 2024

minikube flakes #21931

Closed

stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 30, 2024

stale-locking-app bot locked as resolved and limited conversation to collaborators May 30, 2024

Make inspect compatible with docker v1.44 API #21601

Make inspect compatible with docker v1.44 API #21601

Conversation

flobz commented Feb 10, 2024

Does this PR introduce a user-facing change?

packit-as-a-service bot commented Feb 10, 2024

rhatdan commented Feb 10, 2024

martinpitt commented Feb 10, 2024

martinpitt commented Feb 10, 2024

martinpitt commented Feb 10, 2024

flobz commented Feb 10, 2024 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Feb 12, 2024

flobz commented Feb 20, 2024

Luap99 commented Feb 21, 2024

mheon commented Feb 21, 2024

flobz commented Feb 22, 2024 • edited Loading

rhatdan commented Feb 22, 2024

flobz commented Feb 22, 2024

flobz commented Feb 23, 2024 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flobz commented Feb 28, 2024 • edited Loading

packit-as-a-service bot commented Feb 28, 2024

martinpitt commented Feb 29, 2024

rhatdan commented Feb 29, 2024

TomSweeneyRedHat commented Feb 29, 2024

TomSweeneyRedHat commented Feb 29, 2024

mheon commented Feb 29, 2024

mheon commented Feb 29, 2024

mheon commented Feb 29, 2024

openshift-ci bot commented Feb 29, 2024

flobz commented Feb 10, 2024 •

edited

Loading

flobz commented Feb 22, 2024 •

edited

Loading

flobz commented Feb 23, 2024 •

edited

Loading

flobz commented Feb 28, 2024 •

edited

Loading