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 Full ZFS Dataset Support #118

Merged
merged 8 commits into from
Apr 14, 2024

Conversation

templehasfallen
Copy link
Contributor

New PR superseding #113

Added Full ZFS Dataset Support:

  • The script will now create a ZFS dataset for each jail if the 'jailmaker' directory is a ZFS dataset
  • The script will create the 'jails' directory as a dataset if the 'jailmaker' directory is a ZFS dataset
  • The script will now remove the ZFS dataset (including snapshots) when deleting the jail
  • Dual mode: For legacy use without datasets, it will continue to work as previously
  • The script now lists jails based on the config file existing rather than the directory

Tested on 23.10.2 and 24.04-RC1

Added Full ZFS Dataset Support:

- The script will now create a ZFS dataset for each jail if the 'jailmaker' directory is a ZFS dataset
- The script will create the 'jails' directory as a dataset if the 'jailmaker' directory is a ZFS dataset
- The script will now remove the ZFS dataset (including snapshots) when deleting the jail
- Dual mode: For legacy use without datasets, it will continue to work as previously
- The script now lists jails based on the config file existing rather than the directory

Tested on 23.10.2 and 24.04-RC1
@Jip-Hop
Copy link
Owner

Jip-Hop commented Apr 11, 2024

Thanks! I had a quick look.

The script now lists jails based on the config file existing rather than the directory

Is this change required for the ZFS dataset feature?

@templehasfallen
Copy link
Contributor Author

Thanks! I had a quick look.

The script now lists jails based on the config file existing rather than the directory

Is this change required for the ZFS dataset feature?

No, it is not, I previously implemented it so that the script checks for config files rather than directories.
For functionality it helps with the case where user has created unrelated/empty/temp directories/datasets inside the jails directory that don't contain actual jailmaker jails.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Apr 12, 2024

Unfortunately I haven't been able to properly test it yet. I did make some code changes (untested!). What do you think? By the way I haven't made up my mind about this change:

The script now lists jails based on the config file existing rather than the directory

Seems to me like extra complicating things. A jail dir with a config file without a rootfs dir is still useless. And I don't want to encourage users to manually create directories inside the jails dir. Also, this change is unrelated to dataset support.

@templehasfallen
Copy link
Contributor Author

Unfortunately I haven't been able to properly test it yet. I did make some code changes (untested!). What do you think? By the way I haven't made up my mind about this change:

The script now lists jails based on the config file existing rather than the directory

Seems to me like extra complicating things. A jail dir with a config file without a rootfs dir is still useless. And I don't want to encourage users to manually create directories inside the jails dir. Also, this change is unrelated to dataset support.

Looks much better with relative paths and clean code :)
Tested it on 23.10.2 and 24.04-RC1 and it works fine (creating/deleting jails, creating jails dataset)

I don't think checking for a config file instead of a directory is overcomplicating things. If anything its straightforward and rules out lots of cases for errors - a directory can be created from someone for lots of reasons, even by accident, a config file is much more specific.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Apr 12, 2024

a directory can be created from someone for lots of reasons, even by accident

I think I prefer those directories to be recognized as jails so they show up in the output of jlmkr list and can be removed with jlmkr remove instead of them being hidden from management by jailmaker.

@Jip-Hop
Copy link
Owner

Jip-Hop commented Apr 14, 2024

I hadn't mentioned it before, but good job on implementing the ZFS dataset feature request! 🎉 I think this PR is now ready to be released as jailmaker version 1.1.4. Could you confirm one last time that it's still working as intended? I undid the changes unrelated to #80...

@Jip-Hop Jip-Hop changed the base branch from develop to main April 14, 2024 09:39
@Jip-Hop
Copy link
Owner

Jip-Hop commented Apr 14, 2024

P.S. it would be awesome if you could write a small migration guide to help users coming from the current plain directories setup transition to the ZFS dataset setup. I'll include it in the release notes.

Added a guide to migrate from using directories to using ZFS datasets
@templehasfallen
Copy link
Contributor Author

I hadn't mentioned it before, but good job on implementing the ZFS dataset feature request! 🎉 I think this PR is now ready to be released as jailmaker version 1.1.4. Could you confirm one last time that it's still working as intended? I undid the changes unrelated to #80...

Thanks a lot :)
I can confirm, just tested it on both 23.10.2 and 24.04-RC1 just now.

P.S. it would be awesome if you could write a small migration guide to help users coming from the current plain directories setup transition to the ZFS dataset setup. I'll include it in the release notes.

Its done :)

@Jip-Hop Jip-Hop merged commit a7c4b9d into Jip-Hop:main Apr 14, 2024
@NicholasFlamy
Copy link

Thank you so much everyone. I've been using Jailamaker since v1.0.0 or v1.0.1 (I feel like it has been longer but all the older releases are deleted. I know I used it before Cobia was stable, that's for sure.) It's been great and things like this just keep making it better. Maybe one day iX will integrate it into TrueNAS and create a GUI for it even.

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.

None yet

3 participants