Added support for ZFS datasets as jail directories

[templehasfallen]

Added support for using existing ZFS datasets as jail directories.

• Allows creation of jails in existing folders/directories that can be ZFS datasets
• The script now checks if a jail exists by checking for the config file rather than the directory
• The script now lists jails also based on the config file existing rather than the directory
• When deleting and the directory is a ZFS dataset, it will delete the config file and rootfs directory rather than deleting the directory

[Jip-Hop]

Cool! Can you please edit the pull request to merge into the develop branch? I'll try to look at this on Sunday.

[Salvoxia]

One question though:
The last item reads like the script will only not delete the directory if it is a dataset when deleting the jail. But if the script now allows creating jails in existing directories, is it really a good idea to delete the whole directory and everything in it that may have been there before the jail was created (in case the directory is not a dataset)?

[templehasfallen]

Cool! Can you please edit the pull request to merge into the develop branch? I'll try to look at this on Sunday.

Done

One question though: The last item reads like the script will only not delete the directory if it is a dataset when deleting the jail. But if the script now allows creating jails in existing directories, is it really a good idea to delete the whole directory and everything in it that may have been there before the jail was created (in case the directory is not a dataset)?

That's a valid point.
Then again, this should be specifically for the "jails" folder in the dataset used specifically for jailmaker, its a stretch to assume some has critical data in a folder already existing in jailmaker/jails/appname and then installs an app into it and deletes it.

Alternatively one could implement a check to see if the folder is not a dataset during jail creation and store it as a flag somewhere and only delete jail specific content during deletion... but I think that's overcomplicating things for no reason.

[Jip-Hop]

Thanks for providing this Pull Request :)

It's a good start but I was hoping you could make it a little more complete. Would you be able to add the following?

Pseudo code during jail create:

If the "jails" directory does not exist:
    if "jailmaker" directory is a ZFS dataset:
        create a new zfs dataset for "jails"
    else:
        create a new "jails" directory

...

# The "jails" dir shoud now exist
# Either as plain directory (e.g. legacy users) or a zfs dataset (new users)

If the "jails" directory is a zfs dataset:
    create a new zfs dataset for under "jails" (e.g. "jailmaker/jails/jailname")
else:
    create a new directory (e.g. "jailmaker/jails/jailname")

Then during cleanup you can check once more if "jailmaker/jails/jailname" is a zfs dataset. If it is you can destroy this dataset, else just shutil.rmtree() like the code currently does.

What do you think?

[templehasfallen]

Thanks for providing this Pull Request :)

It's a good start but I was hoping you could make it a little more complete. Would you be able to add the following?

Pseudo code during jail create:

If the "jails" directory does not exist:
    if "jailmaker" directory is a ZFS dataset:
        create a new zfs dataset for "jails"
    else:
        create a new "jails" directory

...

# The "jails" dir shoud now exist
# Either as plain directory (e.g. legacy users) or a zfs dataset (new users)

If the "jails" directory is a zfs dataset:
    create a new zfs dataset for under "jails" (e.g. "jailmaker/jails/jailname")
else:
    create a new directory (e.g. "jailmaker/jails/jailname")

Then during cleanup you can check once more if "jailmaker/jails/jailname" is a zfs dataset. If it is you can destroy this dataset, else just shutil.rmtree() like the code currently does.

What do you think?

Hey,

Yeah, this sounds very similar to how I initially wanted to do it. The reason I didn't and said "let the user manually create/delete datasets" is because while creating a ZFS dataset is easy, deleting may get complicated because you need to decide how snapshots are going to be handled during deletion. In order to destroy (delete) a dataset, you need to first delete snapshots or delete the dataset with snapshots recursively.
If someone is using separate datasets for jails, its most likely driven by wanting to use snapshots so it would apply to most cases.
Do you delete all snapshots with the dataset recursively? Do you ask the user? Do you add a flag to delete the ZFS dataset?

My suggestion is and this is probably how I'll implement it (probably next week unless someone beats me to it):

• Add a flag to the creation like --zfs-dataset or --create-zfs-dataset to create the dataset if the conditions you mentioned are true
• Add a flag to the deletion --zfs-dataset or --delete-zfs-dataset to delete the dataset. If the program detects that the user is trying to delete a jail which is a dataset, it could be implemented with 1. Deleting contents and asking user to delete dataset manually or 2. Ask user to pass the --delete-zfs-dataset flag and do nothing. Probably would go with 2.

[Jip-Hop]

Glad to hear you want to work on this feature some more :)

delete the dataset with snapshots recursively.

I think this is what needs to happen. If the user wants to keep snapshots, then just don't remove the jail.

The jlmkr remove command already requires user confirmation before removing a jail. So this is an opportunity to warn the user that all jail snapshots will be removed too. The user can then abort if this is not indented.

What do you think?

[Jip-Hop]

By the way I don't think the user should manually create or delete datasets. I think the default, for new users, should be to create the "jails" dir as a dataset automatically. And then for each jail a new dataset should be created automatically and this dataset should be removed automatically (with snapshots) if the user confirms the jlmkr remove command.

For existing users jailmaker should continue to create plain directories inside the "jails" dir. I prefer to keep these 2 cases consistent (all jails are in plain directories or all jails are in their own dataset). Merging the PR in its current state would mean, for a single user, some jails may be in datasets while others are in plain directories. While I appreciate your approach and its simplicity I don't think it would be good to mix these cases.

[templehasfallen]

Glad to hear you want to work on this feature some more :)

delete the dataset with snapshots recursively.

I think this is what needs to happen. If the user wants to keep snapshots, then just don't remove the jail.

The jlmkr remove command already requires user confirmation before removing a jail. So this is an opportunity to warn the user that all jail snapshots will be removed too. The user can then abort if this is not indented.

What do you think?

I agree with you, yes it makes sense.

By the way I don't think the user should manually create or delete datasets. I think the default, for new users, should be to create the "jails" dir as a dataset automatically. And then for each jail a new dataset should be created automatically and this dataset should be removed automatically (with snapshots) if the user confirms the jlmkr remove command.

For existing users jailmaker should continue to create plain directories inside the "jails" dir. I prefer to keep these 2 cases consistent (all jails are in plain directories or all jails are in their own dataset). Merging the PR in its current state would mean, for a single user, some jails may be in datasets while others are in plain directories. While I appreciate your approach and its simplicity I don't think it would be good to mix these cases.

The only problem is that this would technically be a breaking change for updating, requiring users to create datasets in place of the folders and move the contents of jails/jailmaker into those datasets to continue using it.

However, I absolutely agree with this approach - when I first used jailmaker I thought it already was like this.

So by default, I should rewrite the code to required the jailmaker directory and the jails directory to be zfs datasets, along with jails being created as datasets, right?

[Jip-Hop]

It doesn't have to be a breaking change if implemented in a backwards compatible way, as suggested here. You could treat the "jails" dir a bit like a boolean. If it is a plain directory (no dataset), then skip creating a dataset for a new jail (just make a plain directory like jailmaker currently does). If the "jails" dir is a dataset (which won't be the case for any existing users), then do create a dataset when creating a new jail. If the "jails" dir doesn't exist, then create it as a dataset, as the new default for new users.

The changes to jlmkr remove won't be breaking either. Similar to what you've already implemented. You check if is_zfs_dataset(jail_name). If it's not a dataset then we do shutil.rmtree(jail_path), but if it is a dataset then we should destroy the dataset and all snapshots (after warning user and awaiting confirmation of course).

Existing users may migrate to the new default manually, by stopping all running jails and then renaming the "jails" dir to e.g. "jails_old". Then they could update to the latest jlmkr.py and re-create their jails. A new dataset will be created for each jail. To restore the rootfs/config file they can then rsync from "jails_old" to "jails".

So by default, I should rewrite the code to required the jailmaker directory and the jails directory to be zfs datasets, along with jails being created as datasets, right?

Yes, but keep in mind the backwards compatibility. And you make a good point. The "jailmaker" directory itself needs to be a dataset too for this to work I suppose. Currently jailmaker doesn't check for this. So some users may have /mnt/tank/some_dataset/some_plain_subdirectory/jailmaker. Where "jailmaker" is a plain directory instead of a dataset...

[templehasfallen]

It doesn't have to be a breaking change if implemented in a backwards compatible way, as suggested here. You could treat the "jails" dir a bit like a boolean. If it is a plain directory (no dataset), then skip creating a dataset for a new jail (just make a plain directory like jailmaker currently does). If the "jails" dir is a dataset (which won't be the case for any existing users), then do create a dataset when creating a new jail. If the "jails" dir doesn't exist, then create it as a dataset, as the new default for new users.

The changes to jlmkr remove won't be breaking either. Similar to what you've already implemented. You check if is_zfs_dataset(jail_name). If it's not a dataset then we do shutil.rmtree(jail_path), but if it is a dataset then we should destroy the dataset and all snapshots (after warning user and awaiting confirmation of course).

Existing users may migrate to the new default manually, by stopping all running jails and then renaming the "jails" dir to e.g. "jails_old". Then they could update to the latest jlmkr.py and re-create their jails. A new dataset will be created for each jail. To restore the rootfs/config file they can then rsync from "jails_old" to "jails".

So by default, I should rewrite the code to required the jailmaker directory and the jails directory to be zfs datasets, along with jails being created as datasets, right?

Yes, but keep in mind the backwards compatibility. And you make a good point. The "jailmaker" directory itself needs to be a dataset too for this to work I suppose. Currently jailmaker doesn't check for this. So some users may have /mnt/tank/some_dataset/some_plain_subdirectory/jailmaker. Where "jailmaker" is a plain directory instead of a dataset...

Yeah I agree, this makes absolute sense. So:

-> Jailmaker directory is plain -> create plain jail directory
-> Jailmaker directory is zfs dataset -> create zfs dataset

Similarly with removing jails. :)

Hopefully I'll get to this this week, this also makes things a lot easier for my own use case

[templehasfallen]

Closed in Favor of #118