search

TritonDataCenter / smartos-live

For more information, please see http://smartos.org/ For any questions that aren't answered there, please join the SmartOS discussion list: https://smartos.topicbox.com/groups/smartos-discuss
1.57k stars 246 forks source link

Fix setup of encrypted zpool when setting up a new compute node #1021

Closed teutat3s closed 2 years ago

teutat3s commented 2 years ago

Fixes #1020

jasonbking commented 2 years ago

While it no longer counts, +1 from me.

bahamat commented 2 years ago

@jasonbking it definitely counts.

@teutat3s have you been able to properly test this? If so, please add testing output to #1020. If you need a test platform built, let me know.

jasonbking commented 2 years ago

@bahamat Not to GitHub (what I meant) :)

danmcd commented 2 years ago

I maybe broke this with the RFD 176 support. What I'm not seeing here nor in #1020 is why this broke. Since I broke it, I'll provide an analysis text both here, and in #1020 :

. . . The kbmadm(1M-soon-to-be-8) man page states clearly that its ZFS pool creation arguments to appear after a "--", as they are passed straight into zpool(1M-soon-to-be-8). For some reason (and upon reading the source history of zfs.js I'm wondering if it ever did, as the "--" requirement was always there, maybe the introduction of "-f" in zfs.js?) the zfs.js caller does not insert the required "--" and things go sideways. . . .

danmcd commented 2 years ago

Oh, it's failing because copyright needs to be updated. You can put your own in, or we@joyent can update ours.

jasonbking commented 2 years ago

Yeah -- that might have been a bug in the original bit I added that got surfaced with the latter changes... since getopt() stops at either '--' or a non argument, if could be the latter was causing the kbmadm specific flag processing to stop previously.

danmcd commented 2 years ago

Also @bahamat -- will we need to update other repos' zfs.js per the suggestion at the top of this file?

teutat3s commented 2 years ago

Maybe this gist from @blackwood821 could add some insight to why this didn't surface before: it seems the elif statement in the joysetup script jumped in and before introduction of the -f flag, -e was the only flag in that conditional scope. https://gist.github.com/blackwood821/cb09dde3b716715149d565804a90ef67

blackwood821 commented 2 years ago

I think the issue was introduced in this commit: https://github.com/joyent/smartos-live/commit/da5a7509459e7599610dd1c2644f3b98a6fa0332#diff-61b2771ed09ddd69a765a254a7b1727b7661f7985441194591b8e44cb284f007R143

which changed from:

args = [ 'create', '-f', pool ];

to:

cmd = exports.paths.kbmadm;
args = [ 'create-zpool' ];

So -- was never in there once kbadm started being used when encryption is enabled.

danmcd commented 2 years ago

Also TIL, Jenkins won't run from a non-core-contributor PR. I've kicked that off now.