Skip to content

Conversation

@djs55
Copy link
Contributor

@djs55 djs55 commented Jul 24, 2020

- What I did

Extended trim-after-delete to run the command after a container destroy and volume destroy.

- How I did it

We already watch for events, so it's a simple matter to handle the additional container and volume events.

- How to verify it

I run

trim-after-delete -- /sbin/fstrim /var/lib/docker

and then docker rm a container. I see the following output:

2020/07/24 13:08:10 A container has been deleted: will run the action at least once more
2020/07/24 13:08:20 Running /sbin/fstrim /var/lib/docker

If I docker image rm an image I see the following output:

2020/07/24 13:08:41 An image has been deleted: will run the action at least once more
2020/07/24 13:08:51 Running /sbin/fstrim /var/lib/docker

If I docker volume rm a volume I see the following output:

2020/07/24 13:25:25 A volume has been removed: will run the action at least once more
2020/07/24 13:25:35 Running /sbin/fstrim /var/lib/docker

- Description for the changelog

  • trim-after-delete: run the command also after container destroy and volume destroy.

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

iu

@djs55 djs55 force-pushed the trim-after-delete-container branch 2 times, most recently from 358054e to f8b9934 Compare July 24, 2020 13:26
@djs55 djs55 changed the title trim-after-delete: run the command after container destroy trim-after-delete: handle containers and volumes as well as images Jul 24, 2020
Comment on lines +139 to +145
log.Printf("An image has been removed: will run the action at least once more")
action.AtLeastOnceMore()
} else if event.Action == "destroy" && event.Type == "container" {
log.Printf("A container has been removed: will run the action at least once more")
action.AtLeastOnceMore()
} else if event.Action == "destroy" && event.Type == "volume" {
log.Printf("A volume has been removed: will run the action at least once more")
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if this will become very noisy (perhaps not a problem, just curious if every even should be logged here, or logging should also be delayed (or only in debug mode)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not noticed whether it's a problem in practice or not... but I've not been looking. I'll keep an eye out!

@thaJeztah
Copy link
Contributor

(also opened moby/moby#41259 to allow checking for "prune" events, which include pruning the BuildKit build cache)

@justincormack
Copy link
Member

LGTM

@djs55 djs55 force-pushed the trim-after-delete-container branch from b684ed8 to 830d70d Compare July 28, 2020 08:05
thaJeztah
thaJeztah previously approved these changes Jul 28, 2020
Copy link
Contributor

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, but feel free to squash my suggestion commit @djs55

@justincormack
Copy link
Member

Thanks I built the package can you update the examples to use linuxkit/trim-after-delete:6258aec13d91259379289b577f8a79754819777e in a new commit and will merge.

djs55 and others added 2 commits October 16, 2020 16:56
We already run the command after an image delete but

- a container delete
- a volume delete

will also free space on the filesystem.

Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
@djs55 djs55 force-pushed the trim-after-delete-container branch from 55e670b to 1866957 Compare October 16, 2020 15:59
@djs55
Copy link
Contributor Author

djs55 commented Oct 16, 2020

I pushed a separate commit with the example update. Per @thaJeztah 's request I squashed his update into my original patch, hope this is ok!

Copy link
Contributor

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rn rn left a comment

Choose a reason for hiding this comment

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

Somehow this was never merged. merging now

@rn rn merged commit e48d529 into linuxkit:master Apr 5, 2021
@djs55
Copy link
Contributor Author

djs55 commented Apr 5, 2021

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants