-
Notifications
You must be signed in to change notification settings - Fork 246
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
providers: add initial zVM hypervisor support #865
Conversation
Can one of the admins verify this patch? |
ba9885f
to
6970929
Compare
ok to test |
internal/providers/zvm/zvm.go
Outdated
return (err == nil) | ||
} | ||
} | ||
_, err = logger.LogCmd(exec.Command("udevadm", "settle", "||", "udevsettle"), "Settle udev device") |
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.
These commands are invoked directly, so shell functions are not available (||
). Does this actually work?
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.
if "udevadm settle" failed ,run "udevsettle" . It worked.
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.
It probably worked because udevadm ignored the ||
and udevsettle
arguments. This should be handled by the distro package; it exists to allow setting different commands at link-time so it can be compiled against different distros with different commands.
Actually, the udevsettle can almost certainly be dropped. Ignition depends on systemd, which means udevadm should always exist.
internal/providers/zvm/zvm.go
Outdated
_, err := f.Logger.LogCmd(exec.Command("/sbin/vmur", "re", "-f", spoolid, file), "Receive the spool file") | ||
if err == nil { | ||
f.Logger.Info("using config file at %q", file) | ||
_, err := f.Logger.LogCmd(exec.Command("tar", "-xzvf", file), "Extract the spool file") |
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.
Are spool files always sent as compressed tarballs?
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.
tarballs are not necessary for spool files. But for ignition config JSON file, the vmur tools will format the config file which cause parse failure. So tarball can avoid the ignition config JSON file be formatted.
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.
I don't understand. Are you saying that the spool file is not presenting a tarball but vmur is converting it?
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.
^^^ Can you clarify about my previous question?
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.
Sorry. I forgot this question. I mean If not using tarball, vmur will format the json config file with padding char. There will error when do later ParseConfig operation. So to avoid the config file being formatted , I chose to use tarball.
internal/providers/zvm/zvm.go
Outdated
record := strings.Split(records, " ") | ||
f.Logger.Info("The single record info: ") | ||
fmt.Println(record, len(record)) | ||
if len(record) == 1 { |
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 length-check should verify that all five fields exist.
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 length-check verify that it is the last null record to avoid the “record[*]” error.
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.
I don't understand? What is the record[*]
error? We should still check that len(record) == 5
(or is it >=5
? I don't know the format) before accessing record[4]
to eliminate the chance of panics from out of bounds access.
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.
Can you clarify about my previous question?
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.
The length of the record is >=11, but when I use strings.Split(s, "\n"). There will be a null record as the last record. It will cause error when read record[1].
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.
Overall approach seems good. Left comments about a bunch of misc bits.
bd0fc90
to
8ac85dd
Compare
@ajeddeloh @crawford @lucab Thanks for your review comments. I have finished the code changes and test the function on zVM platform referring to your comments. Could you please review my pull request again? |
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.
Thanks, gave it another pass
internal/providers/zvm/zvm.go
Outdated
_, err := f.Logger.LogCmd(exec.Command("/sbin/vmur", "re", "-f", spoolid, file), "Receive the spool file") | ||
if err == nil { | ||
f.Logger.Info("using config file at %q", file) | ||
_, err := f.Logger.LogCmd(exec.Command("tar", "-xzvf", file), "Extract the spool file") |
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.
I don't understand. Are you saying that the spool file is not presenting a tarball but vmur is converting it?
internal/providers/zvm/zvm.go
Outdated
record := strings.Split(records, " ") | ||
f.Logger.Info("The single record info: ") | ||
fmt.Println(record, len(record)) | ||
if len(record) == 1 { |
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.
I don't understand? What is the record[*]
error? We should still check that len(record) == 5
(or is it >=5
? I don't know the format) before accessing record[4]
to eliminate the chance of panics from out of bounds access.
3d79404
to
46a7b05
Compare
internal/providers/zvm/zvm.go
Outdated
return (err == nil) | ||
} | ||
} | ||
_, err = logger.LogCmd(exec.Command("udevadm", "settle", "||", "udevsettle"), "Settle udev device") |
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.
It probably worked because udevadm ignored the ||
and udevsettle
arguments. This should be handled by the distro package; it exists to allow setting different commands at link-time so it can be compiled against different distros with different commands.
Actually, the udevsettle can almost certainly be dropped. Ignition depends on systemd, which means udevadm should always exist.
internal/providers/zvm/zvm.go
Outdated
_, err := f.Logger.LogCmd(exec.Command("/sbin/vmur", "re", "-f", spoolid, file), "Receive the spool file") | ||
if err == nil { | ||
f.Logger.Info("using config file at %q", file) | ||
_, err := f.Logger.LogCmd(exec.Command("tar", "-xzvf", file), "Extract the spool file") |
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.
^^^ Can you clarify about my previous question?
internal/providers/zvm/zvm.go
Outdated
record := strings.Split(records, " ") | ||
f.Logger.Info("The single record info: ") | ||
fmt.Println(record, len(record)) | ||
if len(record) == 1 { |
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.
Can you clarify about my previous question?
a2afdaa
to
8e21c66
Compare
@ajeddeloh Please review my code again for my latest updates. Thanks. |
internal/providers/zvm/zvm.go
Outdated
readerInfo, err := exec.Command(distro.VmurCmd(), "li").CombinedOutput() | ||
if err != nil { | ||
f.Logger.Err("Can not get reader device: %v", err) | ||
return types.Config{}, report.Report{}, err |
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.
The returned error should be wrapped with a fmt.Errorf()
like the log message so the error retains context after it's been returned.
internal/providers/zvm/zvm.go
Outdated
_, err := f.Logger.LogCmd(exec.Command(distro.VmurCmd(), "re", "-f", spoolid, file), "Receive the spool file") | ||
if err == nil { | ||
f.Logger.Info("using config file at %q", file) | ||
_, err := f.Logger.LogCmd(exec.Command(distro.TarCmd(), "-xzvf", file), "Extract the spool file") |
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.
You previously replied:
Sorry. I forgot this question. I mean If not using tarball, vmur will format the json config file with padding char. There will error when do later ParseConfig operation. So to avoid the config file being formatted , I chose to use tarball.
What is padding char in this case? If the config is just padded I think it would be better to just strip the padding than to need to run tar?
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.
@ajeddeloh Hi, The process is :
Write data to the z/VM punch file queue and transfer it to virtual reader. The data is sliced up into 80-byte chunks (called records) and written to the punch device. If the data length is not an integer multiple of 80 or 132, the last record is padded. So The length of padded string is not confirmed.
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.
@ajeddeloh I have resolved the padding string with bytes package. So now tar is not necessary.
1fe990c
to
b8d1875
Compare
To support coreos/ignition#865 Reported-by: XiaoMeiZheng <[email protected]>
doc/supported-platforms.md
Outdated
@@ -10,6 +10,7 @@ Ignition is currently only supported for the following platforms: | |||
* [Packet] - Ignition will read its configuration from the instance userdata. SSH keys are handled by coreos-metadata. | |||
* [QEMU] - Ignition will read its configuration from the 'opt/com.coreos/config' key on the QEMU Firmware Configuration Device (available in QEMU 2.4.0 and higher). | |||
* [DigitalOcean] - Ignition will read its configuration from the droplet userdata. SSH keys and network configuration are handled by coreos-metadata. | |||
* [zVM] - Ignition will read its configuration from the reader device directly.The vmur program is necessary,which requires the vmcp and vmur kernel module as prerequisite, and the corresponding z/VM virtual unit record devices (in most cases 000c as reader, 000d as punch) must be set online. |
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.
Nit: two missing spaces after the first .
and ,
.
bc174d5
to
27e5e66
Compare
8d72d0c
to
b944ed6
Compare
@ajeddeloh Hi, please help recheck my PR for the Black Box Tests. I have fixed some comments and please give another review and go ahead this PR. Thanks. |
To support coreos/ignition#865 Reported-by: XiaoMeiZheng <[email protected]>
I see the following tests failed:
Seem like flakes though? |
retest this please |
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 is looking good, just two minor style nits. I don't have a way to test this though. @jlebon @arithx @darkmuggle can one of you give this a quick skim too?
@ajeddeloh @jlebon @arithx Hi , I have updated the code with your latest comments and test the function on my zVM platform. Please go head this PR. Thanks. The initramfs logs is: [ 2.385906] [01;31m [Kignition [m [K[650]: Ignition 2.99.998 |
_, err := f.Logger.LogCmd(exec.Command(distro.ModprobeCmd(), "vmur"), "Loading zVM control program module") | ||
if err != nil { | ||
f.Logger.Err("Couldn't install vmur module: %v", err) | ||
errors := fmt.Errorf("Couldn't install vmur module: %v", err) |
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.
Minor: can probably dedupe these by assigning to a var first?
Edit: and same in other spots below
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.
Minor: can probably dedupe these by assigning to a var first?
Edit: and same in other spots below
Did you mean a function like printError() to do f.loggr.Err? I don't think printError should replace f.logger.Err to involve the context.
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.
I just mean something like:
errmsg := fmt.Sprintf("Couldn't install vmur module: %v", err)
f.Logger.Err(errmsg)
errors := fmt.Errorf(errmsg)
5179c9b
to
9a06e48
Compare
Thanks for all the comments. @ajeddeloh If there is no more comments, Could you please help merge this pull request? |
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 looks sane to me! Will let @ajeddeloh do the final look and merge.
To support coreos/ignition#865 Reported-by: XiaoMeiZheng <[email protected]>
Support extracting an Ignition config from the reader device on z/VM platform. It doesn't require network setup in the initramfs, nor waiting for a config-drive that may not show up.
Fixes #810