-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implement -t option to zpool create for temporary pool names #2417
Conversation
To provide an example of where this is helpful, here are some commands from my workstation to setup a VM as the root user:
Then to start the VM as a user:
This patch eliminates the need to do a machine context switch to a VM environment before the guest is installed. Consequently, I can now quickly create VMs with a rootfs on ZFS to test ideas without having to utilize an emulated graphical console. The only caveat is that the VMs created this way do not have whole_disk set or a partition table created, but that is a relatively minor detail when I care about debugging the kernel code during early boot. Having to juggle guest pool names is far more annoying. |
Seems to work like a charm, thank you. |
@@ -691,7 +691,17 @@ zpool_valid_proplist(libzfs_handle_t *hdl, const char *poolname, | |||
goto error; | |||
} | |||
break; | |||
case ZPOOL_PROP_TNAME: | |||
if (!flags.create && !flags.import) { |
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.
This merits some comment. While temporary names are useful for both creation and import, the API for the import case is easier to use than the API for the create case. On import, the API already supported two names and import flags. Consequently, -t
could just set a bit in the import flags to state that the old name is to be sent to disk. On create, the API only uses 1 name and opts for a nvlist does not support "creation flags", such that we are forced to use the nvlist. We have validation code in userland and a lingering question on my mind is if the import code should handle passing this flag via a nvlist when doing pool import. If it is, then some additional code should be added to spa_import() to handle this. If not, then this userland validation code should to change.
@sempervictus Thanks for catching that. This should also affect |
Merging and testing. Thank you, as always. |
@ryao when you get a minute could you refresh this against master. @sempervictus how has this been working for you? |
86223eb
to
2fe4410
Compare
@behlendorf It is refreshed. |
2fe4410
to
65518ae
Compare
Adding to a property list only if there is no existing value is used twice. Once by zpool create -R and again by zpool import -R. Now that zpool create -t and zpool import -t also need it, lets refactor it into a helper function to make the code more readable. Signed-off-by: Richard Yao <[email protected]>
zpool import's -t parameter is intended for use with -R when operating on pools that belong to other systems. Like -R, pools imported in this way should not update the cachefile unless explicitly requested. The initial implementation allowed the cachefile to be updated when -R was not used. This went uncaught during testing because -R had implicitly disabled use of the cachefile. Signed-off-by: Richard Yao <[email protected]>
65518ae
to
d0866ad
Compare
@behlendorf The problem is that spa_add() will make a copy of the nvlist_t passed to it such that the caller is responsible for freeing it. I had wrongly assumed that such an allocation would not occur. Consequently, adding a free should be sufficient to correct the leak that this introduced. |
d0866ad
to
8e74318
Compare
@behlendorf The failures appear to be pre-existing issues in the build bot. This should be safe to merge. |
8e74318
to
ee3cf59
Compare
Creating virtual machines that have their rootfs on ZFS on hosts that have their rootfs on ZFS causes SPA namespace collisions when the standard name rpool is used. The solution is either to give each guest pool a name unique to the host, which is not always desireable, or boot a VM environment containing an ISO image to install it, which is cumbersome. 26b42f3 introduced `zpool import -t ...` to simplify situations where a host must access a guest's pool when there is a SPA namespace conflict. We build upon that to introduce `zpool import -t tname ...`. That allows us to create a pool whose in-core name is tname, but whose on-disk name is the normal name specified. This simplifies the creation of machine images that use a rootfs on ZFS. That benefits not only real world deployments, but also ZFSOnLinux development by decreasing the time needed to perform rootfs on ZFS experiments. Signed-off-by: Richard Yao <[email protected]>
@behlendorf I made a slight change in the placement of code with respect to a comment in module/zfs/spa.c when using this as a test case for the process of sending changes back to Illumos. I have updated the pull request. Hopefully, pre-existing issues affecting the buildbot will not stop it from being marked green this time. |
This looks good to me. As noted the buildbot failures were unrelated. |
That is good to hear. :) |
Adding to a property list only if there is no existing value is used twice. Once by zpool create -R and again by zpool import -R. Now that zpool create -t and zpool import -t also need it, lets refactor it into a helper function to make the code more readable. Signed-off-by: Richard Yao <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#2417
zpool import's -t parameter is intended for use with -R when operating on pools that belong to other systems. Like -R, pools imported in this way should not update the cachefile unless explicitly requested. The initial implementation allowed the cachefile to be updated when -R was not used. This went uncaught during testing because -R had implicitly disabled use of the cachefile. Signed-off-by: Richard Yao <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#2417
Creating virtual machines that have their rootfs on ZFS on hosts that have their rootfs on ZFS causes SPA namespace collisions when the standard name rpool is used. The solution is either to give each guest pool a name unique to the host, which is not always desireable, or boot a VM environment containing an ISO image to install it, which is cumbersome. 26b42f3 introduced `zpool import -t ...` to simplify situations where a host must access a guest's pool when there is a SPA namespace conflict. We build upon that to introduce `zpool import -t tname ...`. That allows us to create a pool whose in-core name is tname, but whose on-disk name is the normal name specified. This simplifies the creation of machine images that use a rootfs on ZFS. That benefits not only real world deployments, but also ZFSOnLinux development by decreasing the time needed to perform rootfs on ZFS experiments. Signed-off-by: Richard Yao <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#2417
@behlendorf That made my day. :) |
Adding to a property list only if there is no existing value is used twice. Once by zpool create -R and again by zpool import -R. Now that zpool create -t and zpool import -t also need it, lets refactor it into a helper function to make the code more readable. Signed-off-by: Richard Yao <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#2417
zpool import's -t parameter is intended for use with -R when operating on pools that belong to other systems. Like -R, pools imported in this way should not update the cachefile unless explicitly requested. The initial implementation allowed the cachefile to be updated when -R was not used. This went uncaught during testing because -R had implicitly disabled use of the cachefile. Signed-off-by: Richard Yao <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#2417
Creating virtual machines that have their rootfs on ZFS on hosts that
have their rootfs on ZFS causes SPA namespace collisions when the
standard name rpool is used. The solution is either to give each guest
pool a name unique to the host, which is not always desireable, or boot
a VM environment containing an ISO image to install it, which is
cumbersome.
26b42f3 introduced
zpool import -t ...
to simplify situations where a host must access a guest's pool whenthere is a SPA namespace conflict. We build upon that to introduce
zpool import -t tname ...
. That allows us to create a pool whosein-core name is tname, but whose on-disk name is the normal name
specified.
This simplifies the creation of machine images that use a rootfs on ZFS.
That benefits not only real world deployments, but also ZFSOnLinux
development by decreasing the time needed to perform rootfs on ZFS
experiments.
Signed-off-by: Richard Yao [email protected]