Skip to content

Conversation

@flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Jun 26, 2021

Following PR allows podman system reset to remove all the external containers with a safety prompt when --force is not provided. This makes sure that podman system reset performs a complete nuke of all the containers.

Attempts a feature patch for issue: #10755

Example

flouthoc@system:~/src/stable/glibc$ podman system reset
WARNING! This will remove:
        - all containers
        - all pods
        - all images
        - all build cache
WARNING! The following external containers will be purged:
	- dbfc83909cbb (busybox-working-container)
	- c37fb04008bd (busybox-working-container-1)
Are you sure you want to continue? [y/N] y
  • --force silently removes everything without any prompt.
    Example 2
flouthoc@system:~/src/stable/glibc$ podman system reset --force

@flouthoc
Copy link
Collaborator Author

Added [NO TESTS NEEDED] for a check will add relevant tests after initial review if needed.

@flouthoc flouthoc force-pushed the system-reset-prune-external branch 3 times, most recently from b04a617 to bcf786c Compare June 26, 2021 14:04
@flouthoc
Copy link
Collaborator Author

I think failing tests are flake. I'll re-kick CI

@vrothberg
Copy link
Member

I have a weak preference to have only one prompt. In fact, to keep things as they are but in case there are external containers to display them as follows:

flouthoc@system:~/src/stable/glibc$ podman system reset

WARNING! This will remove:
        - all containers (5 external containers)
        - all pods
        - all images
        - all build cache
Are you sure you want to continue? [y/N] y

IMO, system reset is a "nuke all" but we already remove external containers with rmi -f $image.

@flouthoc flouthoc force-pushed the system-reset-prune-external branch from bcf786c to 86b80c9 Compare June 27, 2021 15:59
@flouthoc
Copy link
Collaborator Author

@vrothberg Sure sounds good to me. Just a slight doubt how would human-users know about which external containers are getting purged. Its fine if it is generally acceptable that podman system reset will nuke everything.

@rhatdan
Copy link
Member

rhatdan commented Jun 28, 2021

I agree only give one prompt, but output the external containers as well.

@mheon
Copy link
Member

mheon commented Jun 28, 2021

/approve
Concur that a single prompt would be fine - we should just list the external containers before the existing "proceed" prompt

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

I'm not sure why the release test is failing with dev shouldn't be in the version for the main branch.... @edsantiago ?

@flouthoc flouthoc force-pushed the system-reset-prune-external branch 2 times, most recently from ae7fa06 to 2c9e314 Compare June 29, 2021 06:46
@flouthoc
Copy link
Collaborator Author

@TomSweeneyRedHat Thanks resolved your feedback on latest commit.
@vrothberg @rhatdan @mheon Added a single prompt with list of external containers if any as suggested.

Example

flouthoc@system:~/src/stable/glibc$ podman system reset
WARNING! This will remove:
        - all containers
        - all pods
        - all images
        - all build cache
WARNING! Following External Containers will be purged
CONTAINER ID  NAME
c7d50cbfff74  busybox-working-container
3ab9923a9f8b  busybox-working-container-1
bd371cdddd29  busybox-working-container-2
Are you sure you want to continue? [y/N] y

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fmt.Println(`WARNING! The Following External Containers will be purged`)
fmt.Println(`WARNING! The following external containers will be purged:`)

@vrothberg
Copy link
Member

vrothberg commented Jun 29, 2021

Thanks, @flouthoc!

Visually, I would find it more appealing if the the two lists would have the same format. Maybe something as below?

flouthoc@system:~/src/stable/glibc$ podman system reset
WARNING! This will remove:
        - all containers
        - all pods
        - all images
        - all build cache
WARNING! The following external containers will be purged:
        - c7d50cbfff74  (busybox-working-container)
        - 3ab9923a9f8b  (busybox-working-container-1)
        - bd371cdddd29  (busybox-working-container-2)
Are you sure you want to continue? [y/N] y

Let's wait how the others think.

@edsantiago
Copy link
Member

I'm not sure why the release test is failing with dev shouldn't be in the version for the main branch.... @edsantiago ?

@TomSweeneyRedHat that happens when someone manually presses the "Trigger" button for that test, usually because they see that it didn't run and they believe it should run. It shouldn't. It's an optional test that doesn't actually run and shouldn't run. If you've read HHGTTG, it's the equivalent of a big button that, when pressed, lights up and says "do not press this button again".

@flouthoc
Copy link
Collaborator Author

@edsantiago Oh actually i triggered that test by mistake. 😢 😭

@rhatdan
Copy link
Member

rhatdan commented Jun 29, 2021

@flouthoc Make @vrothberg suggestion and then we can merge.

[NO TESTS NEEDED]

Signed-off-by: flouthoc <flouthoc.git@gmail.com>
@flouthoc flouthoc force-pushed the system-reset-prune-external branch from 2c9e314 to 2243b60 Compare June 30, 2021 03:34
@flouthoc flouthoc requested a review from vrothberg June 30, 2021 03:46
@flouthoc
Copy link
Collaborator Author

@vrothberg Updated with suggested changes in lastest commit

flouthoc@system:~/src/stable/glibc$ podman system reset
WARNING! This will remove:
        - all containers
        - all pods
        - all images
        - all build cache
WARNING! The following external containers will be purged:
	- dbfc83909cbb (busybox-working-container)
	- c37fb04008bd (busybox-working-container-1)
Are you sure you want to continue? [y/N] y

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 30, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, mheon, vrothberg

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

@rhatdan
Copy link
Member

rhatdan commented Jun 30, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 30, 2021
@openshift-merge-robot openshift-merge-robot merged commit 78a3605 into containers:master Jun 30, 2021
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 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. 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.

7 participants