OS-8178 Allow `null` for vmadm(1M) integers to mean the same as the e…

[danmcd]

…mpty-string.

[danmcd]

I want to be sure I'm not breaking any assumptions. Please consider that in your reviews.

[sjorge]

I actually toyed around with it before when looking to see why some parameters did not get cleared properly when setting to null.

I didn't run into issues then (nor did it actually fix my issues) but it's on the patch list for the next pi I build which will probably be tomorrow.

[jlevon]

shouldn't this have a test?

[danmcd]

shouldn't this have a test?

Yes... yes it should. Watch for it later today.

[sjorge]

I did pull it in to my new PI I build, I create manual test vm where I forced bhvye_extra_opts to be "" and once where it was null and it had the same result.

I also ran my salt state that creates a docker zone and native zone and they were fine. The docker one is very finicky because all the weird stuff required in internal_metadata. There as o change between the old and new PI with the patch.

But yes, a test would be nice.

[danmcd]

Pardon delay, needed to run existing /usr/vm/test tests. They were equal on a stock PI from late-April, and a PI with most of this change in it. Added a test now to make it complete, but need to rebuild the PI to see if it checks out.

@sjorge --> bhvye_extra_opts is an integer? If so, glad it worked.

@jlevon --> The new test is an old test with a slight difference.

[danmcd]

Testing with the new test in place showed no differences/regression, AND passed the new "use null" test like the "use empty string"/default test did.

[sjorge]

I found a difference! :(

{
  "brand": "bhyve",
  "alias": "test",
  "ram": 1024,
  "bootrom": "uefi",
  "vnc_port": -1,
  "flexible_disk_size": 2048,
  "disks": [
    {
      "boot": true,
      "model": "virtio",
      "media": "disk",
      "size": 1024,
      "compression": "off",
      "block_size": 4096
    }
  ],
  "nics": [
    {
      "vlan_id": 10,
      "nic_tag": "trunk",
      "ips": [
        "dhcp",
        "addrconf"
      ],
      "model": "virtio"
    }
  ],
  "bhyve_extra_opts": null
}

Will result in a vm with vcpu set to 1 (default) and empty bhyve_extra_opts (str) as expected.

This however:

{
  "brand": "bhyve",
  "alias": "test",
  "ram": 1024,
  "bootrom": "uefi",
  "vcpus": null,
  "vnc_port": -1,
  "flexible_disk_size": 2048,
  "disks": [
    {
      "boot": true,
      "model": "virtio",
      "media": "disk",
      "size": 1024,
      "compression": "off",
      "block_size": 4096
    }
  ],
  "nics": [
    {
      "vlan_id": 10,
      "nic_tag": "trunk",
      "ips": [
        "dhcp",
        "addrconf"
      ],
      "model": "virtio"
    }
  ],
  "bhyve_extra_opts": null
}

Results in vpcu being unset... instead of being the default.

pargs for bhyve also indicates that '-c 1' is missing in the first example.

argv[0]: bhyve
argv[1]: -H
argv[2]: -U
argv[3]: e30a0800-a582-41e0-d014-f8a940cb6e53
argv[4]: -B
argv[5]: 1,manufacturer=Joyent,product=SmartDC HVM,version=7.20200527T091055Z,serial=e30a0800-a582-41e0-d014-f8a940cb6e53,sku=001,family=Virtual Machine
argv[6]: -s
argv[7]: 31,lpc
argv[8]: -l
argv[9]: bootrom,/usr/share/bhyve/uefi-rom.bin
argv[10]: -l
argv[11]: com1,/dev/zconsole
argv[12]: -l
argv[13]: com2,socket,/tmp/vm.ttyb
argv[14]: -s
argv[15]: 0,hostbridge,model=i440fx
argv[16]: -c
argv[17]: 1
argv[18]: -m
argv[19]: 1024
argv[20]: -s
argv[21]: 0:4:0,virtio-blk,/dev/zvol/rdsk/zones/e30a0800-a582-41e0-d014-f8a940cb6e53/disk0
argv[22]: -s
argv[23]: 6:0,virtio-net-viona,net0
argv[24]: SYSbhyve-843

vs

35472:  bhyve -H -U 18566c61-9a8a-ec92-b641-d4b5ceb18f20 -B 1,manufacturer=Joyent,produ
argv[0]: bhyve
argv[1]: -H
argv[2]: -U
argv[3]: 18566c61-9a8a-ec92-b641-d4b5ceb18f20
argv[4]: -B
argv[5]: 1,manufacturer=Joyent,product=SmartDC HVM,version=7.20200527T091055Z,serial=18566c61-9a8a-ec92-b641-d4b5ceb18f20,sku=001,family=Virtual Machine
argv[6]: -s
argv[7]: 31,lpc
argv[8]: -l
argv[9]: bootrom,/usr/share/bhyve/uefi-rom.bin
argv[10]: -l
argv[11]: com1,/dev/zconsole
argv[12]: -l
argv[13]: com2,socket,/tmp/vm.ttyb
argv[14]: -s
argv[15]: 0,hostbridge,model=i440fx
argv[16]: -m
argv[17]: 1024
argv[18]: -s
argv[19]: 0:4:0,virtio-blk,/dev/zvol/rdsk/zones/18566c61-9a8a-ec92-b641-d4b5ceb18f20/disk0
argv[20]: -s
argv[21]: 6:0,virtio-net-viona,net0
argv[22]: SYSbhyve-842

[sjorge]

OK, setting vcpu to an empty string "" has the same result. So it doesn't change behavior! Maybe something that does deseve a followup issue to fix this eventually.

[jlevon]

Surely that difference is exactly what you'd want: if you're passing in null, you really do mean "undefined" not "the default"

[danmcd]

So is that vcpu issue a big enough blocker? Or is this a followup issue? Sounds like at most a followup issue to me, but I am biased. :)

[jlevon]

I don't think there is an issue?

[danmcd]

Cool. Is this good enough to approve, now that I have a new test in place (and AFAICT no regressions from the existing test suite)?

[jlevon]

If it's OK I'd like a little time to review it.

[sjorge]

So is that vcpu issue a big enough blocker? Or is this a followup issue? Sounds like at most a followup issue to me, but I am biased. :)

Not a block, see

OK, setting vcpu to an empty string "" has the same result. So it doesn't change behavior! Maybe something that does deseve a followup issue to fix this eventually.

At best a follow up issue. vmadm's manpage is not clear on the behavior. Should setting something to null/undef/"" be remove this item completely or reset to the default.

Personally I would lean to the later, but I'm fine with either really. But might be useful to explicitly mention it in the man page.