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

Added simple logic for dry run on prune containers #41449

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

kaushik94
Copy link

@kaushik94 kaushik94 commented Sep 14, 2020

- What I did
Implemented a simple dry run mode for containers

- How I did it
When checking for containers that are stale, moby does a daemon.ContainerRm. In dry run mode, I simply disabled this line and returned the container id and size to be removed.

- How to verify it
do
docker run hello-world
docker containers prune --dry-run

use this branch for cli side changes: docker/cli#3226

- Description for the changelog

changed:
container/prune.go -> accept a new parameter dryRun
container_routes.go -> Post a url with dryRun param to backend

TODO: Add tests

- A picture of a cute animal (not mandatory but encouraged)
cute-cat-photos-1593441022

daemon/prune.go Outdated
@@ -26,6 +26,7 @@ var (
"label": true,
"label!": true,
"until": true,
"dryRun": true,
Copy link
Member

Choose a reason for hiding this comment

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

I would probably promote dryRun to a full option rather than a filter argument.

Copy link
Author

Choose a reason for hiding this comment

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

@cpuguy83 when I remove this line, it is complaining me invalid parameters.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably because you are passing dryRun as a filter rather than a configuration option.
It looks like we don't currently accept any options on prune other than filter.

This would need to be added at the API level.

Copy link
Author

Choose a reason for hiding this comment

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

@cpuguy83 can you give an example of any top level options. Like --all? Can you show me the file to add this.

Copy link
Member

Choose a reason for hiding this comment

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

It will need to be decoded here:

func (s *containerRouter) postContainersPrune(ctx context.Context, w http.ResponseWriter, r *http.Request, vars map[string]string) error {
if err := httputils.ParseForm(r); err != nil {
return err
}
pruneFilters, err := filters.FromJSON(r.Form.Get("filters"))
if err != nil {
return errdefs.InvalidParameter(err)
}
pruneReport, err := s.backend.ContainersPrune(ctx, pruneFilters)
if err != nil {
return err
}
return httputils.WriteJSON(w, http.StatusOK, pruneReport)
}

The backend interface definition will need to be updated here:

ContainersPrune(ctx context.Context, pruneFilters filters.Args) (*types.ContainersPruneReport, error)

The options will need to be encoded here:

func (cli *Client) ContainersPrune(ctx context.Context, pruneFilters filters.Args) (types.ContainersPruneReport, error) {
var report types.ContainersPruneReport
if err := cli.NewVersionError("1.25", "container prune"); err != nil {
return report, err
}
query, err := getFiltersQuery(pruneFilters)
if err != nil {
return report, err
}
serverResp, err := cli.post(ctx, "/containers/prune", query, nil, nil)
defer ensureReaderClosed(serverResp)
if err != nil {
return report, err
}
if err := json.NewDecoder(serverResp.body).Decode(&report); err != nil {
return report, fmt.Errorf("Error retrieving disk usage: %v", err)
}
return report, nil
}

daemon/prune.go Outdated
}
if cSize > 0 {
rep.SpaceReclaimed += uint64(cSize)
if (!dryRunMode) {
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicated in the else block.
I think this can be:

Suggested change
if (!dryRunMode) {
if !dryRunMode {
if err := daemon.ContainerRm(c.ID, &types.ContainerRmConfig{}); err != nil {
logrus.Warnf("failed to prune container %s: %v", c.ID, err)
continue
}
}
if cSize > 0 {
rep.SpaceReclaimed += uint64(cSize)
}
rep.ContainersDeleted = append(rep.ContainersDeleted, c.ID)

Note: Don't use GH's UI to accept the change, as the commit won't be signed and it will be rejected by CI.

changed:
daemon/prune.go

TODO: Add tests

Signed-off-by: Kaushik Varanasi <kaushik.varanasi1@gmail.com>
Signed-off-by: Kaushik Varanasi <kaushik.varanasi1@gmail.com>
@kaushik94
Copy link
Author

@cpuguy83 when I try to make a change to the existing API, it complains with the following error:

# github.com/docker/cli/vendor/github.com/docker/docker/client
vendor/github.com/docker/docker/client/interface_stable.go:10:5: cannot use &Client literal (type *Client) as type APIClient in assignment:
	*Client does not implement APIClient (wrong type for ContainersPrune method)
		have ContainersPrune(context.Context, filters.Args) (types.ContainersPruneReport, error)
		want ContainersPrune(context.Context, filters.Args, bool) (types.ContainersPruneReport, error)

This is probably because docker implements a ContainerPrune declaration already. How do I build the moby with my CLI?
I used to just make previously but that doesn't work. Can you please tell me steps to compile my own CLI and then attach that CLI to moby as described here

2. Added dry run option to ContainersPrune in prune.go

Signed-off-by: Kaushik Varanasi <kaushik.varanasi1@gmail.com>
@kaushik94 kaushik94 force-pushed the feature/dry-run-prune-containers branch from 327ebd0 to 66e1d02 Compare July 29, 2021 23:05
2. Added comments

Signed-off-by: Kaushik Varanasi <kaushik.varanasi1@gmail.com>
@kaushik94
Copy link
Author

@cpuguy83 I was able to build the moby code by hacking the vendor/github.com directory of docker/cli. But the tests are still failing on the CLI PR. Can you please help me with understanding. Thanks a ton.

@kaushik94
Copy link
Author

Please don't close this PR. I have started working on this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants