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 ability to run the provisioner directly on the ZFS host. #133

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

jp39
Copy link

@jp39 jp39 commented Aug 19, 2024

The default is to run kubernetes-zfs-provisioner in a container and create datasets via SSH on a remote host.
To do that, the docker image is created with the zfs and update-permissions stubs that will both call commands on the remote host using SSH.

Allows running kubernetes-zfs-provisioner directly on the ZFS host by making the update-permissions script presence optional. The zfs stub is already optional because it merely replaces the command of the same name on the remote host. The provisionner now uses the command specified in the ZFS_UPDATE_PERMISSIONS environment variable, which is set to /usr/bin/update-permissions by default in the docker image, otherwise it falls back to chmod.

Fixes #130.

Copy link
Owner

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

Hi, thanks for you contribution, and sorry for the late response.

I kinda prefer a different solution: I'd rather adapt the script in a way that skips the SSH invocation. In a way, I initially intended it to be modifiable to suit people's need when it comes to changing permissions and thus offloaded the permission bits to a script. Otherwise I could have done this all without a script and directly in go.

What I also like is to ship this update script, the binary and a systemd-unit file in a deb and rpm package if we're supporting non-dockerized installations. We can easily achieve this with some goreleaser lines (see https://github.com/ccremer/paperless-cli/blob/master/.goreleaser.yml#L25 and https://github.com/ccremer/paperless-cli/tree/master/package).

One solution is to ship different update scripts for the docker image and the packages.: For dockerized, we use SSH, for packaging we invoke chmod directly. But that assumes that plain installations are also running on ZFS nodes, which makes deb+ssh more of a pain, as one needs to modify the script again to make it work with SSH.

The better solution of that idea is to adapt the script so it supports both dockerized and packaged installations. By introducing an environment variable in the script only, we can decide whether to invoke chmod wrapped in SSH or not. Something like

if [[ ! -z "${ZFS_SKIP_SSH}" ]]; then
  ssh ...
else
  chmod ...
fi

(untested, better impl and naming welcome).

In the systemd-unit file, we'll set the ZFS_SKIP_SSH variable by default, but can be modified in an override. That way, we don't need to change anything in go code.

All this also needs documentation.

What do you think?

Comment on lines 6 to 5
chmod_bin=${ZFS_CHOWN_BIN:-sudo -H chmod}
chmod_bin=${ZFS_CHMOD_BIN:-sudo -H chmod}
Copy link
Owner

Choose a reason for hiding this comment

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

I know I named it incorrectly, but renaming the Env var would be a breaking change for those people that already override it.
Maybe we can keep the old one as an alias instead, so both work?

@jp39
Copy link
Author

jp39 commented Sep 7, 2024

I'm not sure I agree with the solution you're proposing because I think having to install an extra script (even if it's packaged in an installable archive) whose only purpose is to invoke chmod seems overkill to me. I believe it's good software engineering practice to keep the number of moving parts low. It's also in the spirit of the go language to produce self-contained binaries that needs few or no dependencies.

Unless you're aware of actual users who need the ability to customize the update-permission script I would actually advocate not using an external script at all and move the whole logic in the main program.

@ccremer
Copy link
Owner

ccremer commented Sep 8, 2024

I see your point.
I initially chose the script-route because it's easier to debug a failing SSH connection that way. If there's a problem with dataset connection, one could just adapt the script and provide more args to SSH or ZFS and log the output. If everything's in the binary, it's either not possible or we need to support extra flags somehow. Because if a volume can't be provisioned, it's either something with ZFS (e.g. dataset existing) or SSH unable to connect.

No, I'm not aware of any users that need it customized. Most people give a star or fork it and never make a PR.

Feel free to move the whole logic to the main program, but please test the SSH part and the helm chart :)

@jp39
Copy link
Author

jp39 commented Sep 10, 2024

Why not going for an hybrid approach where the main program tries to execute update-permission if it exists in the PATH or else falls back to calling plain chmod if it does not?

@jp39 jp39 force-pushed the run-on-zfs-host branch 2 times, most recently from bb2046f to 70f330b Compare September 10, 2024 15:45
The default is to run kubernetes-zfs-provisioner in a container and
create datasets via SSH on a remote host.
To do that, the docker image is created with the zfs and
update-permissions stubs that will both call commands on the remote host
using SSH.

Allows running kubernetes-zfs-provisioner directly on the ZFS host by
making the update-permissions script presence optional. The zfs stub is
already optional because it merely replaces the command of the same name
on the remote host. The provisionner now uses the update-permissions
executable if it's present in the current PATH, otherwise it falls back
to executing chmod g+w directly.

Fixes ccremer#130.
@ccremer ccremer merged commit f7ad02d into ccremer:master Sep 11, 2024
3 checks passed
@ccremer
Copy link
Owner

ccremer commented Sep 11, 2024

Thanks for your PR!

@jp39 jp39 deleted the run-on-zfs-host branch September 17, 2024 17:43
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.

Ability to run the provisioner on the ZFS host.
2 participants