-
-
Notifications
You must be signed in to change notification settings - Fork 186
Detach previous workers deleted loop devices #403
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
base: master
Are you sure you want to change the base?
Conversation
Make configmap more generic Signed-off-by: David Rozé <droze@baylibre.com>
Detach deleted loop devices from previous worker instances. It should be done when the worker stops but it's not. Space isn't released, leading the node space to fill up quickly. This will always happen upon a worker crash or forcing a pod to terminate Signed-off-by: David Rozé <droze@baylibre.com>
Concourse process may no longer have PID 1 if some conditions are met eg shareProcessNamespace is used Signed-off-by: David Rozé <droze@baylibre.com>
templates/worker-deployment.yaml
Outdated
| - name: teardown | ||
| image: busybox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the extra image being pulled in like this. It should be obvious which images the chart uses, which is currently concourse/concourse and postgres. This could also fail if being deployed in an environment where Docker hub is not available. I'm going to be very cautious about bringing in another dependency like this into the chart.
I was going to propose using the concourse/concourse image, but sadly not all the tools used in the scripts you added are part of the pre-7.14 images.
Since 7.14 concourse/concourse is based on wolfi which comes with the busybox binary, so your scripts would work fine on the latest images.
We could move forward with this change, but it would be a major bump of the chart since the scripts won't work on <7.14 versions of Concourse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concourse/concourse is pretty big for a sidecar container that just waits. wolfi is perfect, tiny, and does not require many changes.
Just tested and pushed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having to install losetup while booting the sidecar container but it's not present on the wolfi docker image, even in the busybox binary.
Concourse does contain all the required tools (losetup/lsof/pgrep) so this looks like the best option to me. As for the image size, main and sidecar containers run on the same node so this isn't an issue.
I'll update the PR if you're ok with this @taylorsilva
Add sidecar container teardown Monitor Concourse processes post shutdown and detach main container loop devices left alone. Disk space is released on the node from the dead pod. Signed-off-by: David Rozé <droze@baylibre.com>
afcbffa to
bde2bb7
Compare
Existing Issue
Fixes #374 .
Contributor Checklist
master