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

Add support for custom rclone options and fix defuct processes #85

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

Conversation

ppacher
Copy link

@ppacher ppacher commented Dec 14, 2022

Hi and thanks for your work on this!

This PR includes the following changes:

Mounter Parameters

This PR adds support for custom mounter parameters (although it is only implemented for rclone as of now). I needed to be able to specify additional parameters to rclone as the --s3-provider=AWS flag broke support for mounting MinIO buckets. I'm not sure why the tests didn't catch that (I guess thedev-full is not updated) but with this PR it's possible to overwrite the flags. The additional mounter parameters must be prefixed with the mounter name so it's more obvious what those parameters are for.

An example would be:

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: csi-s3-existing-bucket
provisioner: ch.ctrox.csi.s3-driver
parameters:
  mounter: rclone
  bucket: some-existing-bucket-name
  rclone-s3-provider: Minio
  rclone-no-modtime: "true"
  rclone-allow-non-empty: "true"
  rclone-file-perms: "0600"

Build Tags

It's now possible to include/exclude support for mounters from the binary. The mounters are still installed in the Docker image. I basically needed this since I wasn't able to get the go build with goofys to work as it always errors with some strange Go runtime linking in github.com/kahing/fusego.

By default, the docker image will contain support for all mounters but it's now possible to specify it:

make container TAGS="rclone,s3fs"

Defunct Processes

Additionally, this PR fixes the issue with defunct processes.

We're running in the container as PID-1 which gets some special treatment by the kernel. In particular, if a process in the container terminates and there are still active child processes, the kernel will move those orphaned processes to be child processes of PID-1.

Init-systems are expected to handle this case by reaping those "orphan" processes once they exit. The kernel notifies the then active parent process (either the real parent or PID-1 in case the real parent terminated) by sending a SIGCHLD.

Since all available mounters are instructed to daemonize, we need to reap the daemonized processes since their parent (the mounter) exists once the daemon is running.

pruiz added a commit to pruiz/csi-s3 that referenced this pull request Mar 10, 2024
// Since all available mounters are instructed to daemonize, we need to reap
// the daemonized processes since their parent (the mounter) exists once the daemon
// is running.
go func() {

Choose a reason for hiding this comment

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

I don't like this resolution. You assume you're running as PID-1 which you never check. And you globally reap all children which would mess up any code waiting for a process to exit. IMO the proper resolution would be to reap the child after you signal it to exit.

@ppacher
Copy link
Author

ppacher commented Oct 28, 2024

Hi, since I'm not using this anymore for some time now, and the PR is already 2y old, I don't have the time to improve on this. Close as you like

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.

defunct processes, the possible explanation
2 participants