Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revamp Libpod state strings for Docker compat #12684

Merged
merged 1 commit into from
Jan 18, 2022

Conversation

mheon
Copy link
Member

@mheon mheon commented Dec 22, 2021

Improve our compatibility with Docker by better handling the state strings that we print in podman ps. Docker capitalizes all states in ps (we do not) - fix this in our PS code. Also, stop normalizing ContainerStateConfigured to the "Created" state, and instead make it always be Created, with the existing Created state becoming Initialized.

I didn't rename the actual states because I'm somewhat reticent to make such a large change a day before we leave for break. It's somewhat confusing that ContainerStateConfigured now returns Created, but internally and externally we're still consistent.

[NO NEW TESTS NEEDED] existing tests should catch anything that broke.

I also consider this a breaking change. I will flag appropriately on Github.

Fixes RHBZ#2010432 and RHBZ#2032561

@mheon mheon added the breaking-change A change that will require a full version bump, i.e. 3.* to 4.* label Dec 22, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 22, 2021
@mheon
Copy link
Member Author

mheon commented Dec 22, 2021

I fully expect tests will be red initially, will fix whatever this breaks once I know what that happens to be.

@rhatdan
Copy link
Member

rhatdan commented Dec 22, 2021

LGTM

@mheon mheon force-pushed the remap_states branch 4 times, most recently from 1c10ded to 4e40b6a Compare December 22, 2021 19:49
@mheon
Copy link
Member Author

mheon commented Dec 22, 2021

I think I have the tests sorted now

@TomSweeneyRedHat
Copy link
Member

@mheon, I think you needed to cross your fingers tighter on the test wish, a lot of red there.
Will these changes cause any upgrade issues?

@mheon
Copy link
Member Author

mheon commented Dec 23, 2021

Potentially? This is a frequently machine-parsed field and we're making significant changes to it. The breaking-change label is there for that reason.

@mheon
Copy link
Member Author

mheon commented Dec 23, 2021

I might need help from @edsantiago on the system tests, which means they might have to wait until after Christmas break. I think the original failure is definitely in not ok 72 podman ps --external but it's not obvious to me what is actually going wrong.

@edsantiago
Copy link
Member

Looks like lower-case storage got changed to capitalized Storage?

@edsantiago
Copy link
Member

This seems to fix things, but I'm concerned, first because why did these things change, and second, tests seem to be running reeeeeally slowly. I can't take the time to look into it further right now.

diff --git a/test/system/040-ps.bats b/test/system/040-ps.bats
index 61b290415..8d0a405d2 100644
--- a/test/system/040-ps.bats
+++ b/test/system/040-ps.bats
@@ -110,7 +110,7 @@ EOF
     run_podman ps --external
     is "${#lines[@]}" "3" "podman ps -a --external sees buildah containers"
     is "${lines[1]}" \
-       "[0-9a-f]\{12\} \+$IMAGE *buildah .* seconds ago .* storage .* ${PODMAN_TEST_IMAGE_NAME}-working-container" \
+       "[0-9a-f]\{12\} \+$IMAGE *buildah .* seconds ago .* Storage .* ${PODMAN_TEST_IMAGE_NAME}-working-container" \
        "podman ps --external"

     # 'rm -a' should be a NOP
diff --git a/test/system/080-pause.bats b/test/system/080-pause.bats
index 857c8bbf4..4d9b7f889 100644
--- a/test/system/080-pause.bats
+++ b/test/system/080-pause.bats
@@ -26,7 +26,7 @@ load helpers
     is "$output" "paused" "podman inspect .State.Status"
     sleep 3
     run_podman ps -a --format '{{.ID}} {{.Names}} {{.Status}}'
-    is "$output" "${cid:0:12} $cname paused" "podman ps on paused container"
+    is "$output" "${cid:0:12} $cname Paused" "podman ps on paused container"
     run_podman unpause $cname
     run_podman ps -a --format '{{.ID}} {{.Names}} {{.Status}}'
     is "$output" "${cid:0:12} $cname Up .*" "podman ps on resumed container"

@mheon
Copy link
Member Author

mheon commented Dec 23, 2021

The paused vs Paused change is expected. The storage vs Storage was not, but looking at it, I can see how it happened. I think it's probably OK to leave it.

@edsantiago
Copy link
Member

Ack, thanks. But there are more problems: generate systemd tests are failing too. Again, I don't have time now to look into it deeper, sorry.

@Luap99
Copy link
Member

Luap99 commented Jan 13, 2022

@mheon friendly ping, since this is a breaking change we need should this before 4.0

@mheon
Copy link
Member Author

mheon commented Jan 13, 2022

Wait, this didn't land yet? Thought it merged before break.

I'll try and get it landed today.

Improve our compatibility with Docker by better handling the
state strings that we print in `podman ps`. Docker capitalizes
all states in `ps` (we do not) - fix this in our PS code. Also,
stop normalizing ContainerStateConfigured to the "Created" state,
and instead make it always be Created, with the existing Created
state becoming Initialized.

I didn't rename the actual states because I'm somewhat reticent
to make such a large change a day before we leave for break. It's
somewhat confusing that ContainerStateConfigured now returns
Created, but internally and externally we're still consistent.

[NO NEW TESTS NEEDED] existing tests should catch anything that
broke.

I also consider this a breaking change. I will flag appropriately
on Github.

Fixes RHBZ#2010432 and RHBZ#2032561

Signed-off-by: Matthew Heon <[email protected]>
@mheon
Copy link
Member Author

mheon commented Jan 17, 2022

Repushed, I think I got the last test

@mheon
Copy link
Member Author

mheon commented Jan 17, 2022

@containers/podman-maintainers PTAL, this is going green

@edsantiago
Copy link
Member

LGTM. I no longer remember what the problem was with the systemd tests, so, this has my blessing. I'd like someone more docker-experienced to give the final approval though.

@rhatdan
Copy link
Member

rhatdan commented Jan 18, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2022
@openshift-merge-robot openshift-merge-robot merged commit d6b0720 into containers:main Jan 18, 2022
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. breaking-change A change that will require a full version bump, i.e. 3.* to 4.* lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants