TRITON-1213 VM resize fails when tmpfs is 0 and no tmpfs set in package

[joyent-automation]

TRITON-1213 VM resize fails when tmpfs is 0 and no tmpfs set in package

This PR was migrated-from-gerrit, https://cr.joyent.us/#/c/5565/.
The raw archive of this CR is here.
See MANTA-4594 for info on Joyent Eng's migration from Gerrit.

CR discussion

@bahamas10 commented at 2019-02-12T19:49:24

Patch Set 1: Code-Review+1

I see this has been tested with various test suites, but have you run it against the platform test suite? via:

/usr/vm/test/runtests

@marsell commented at 2019-02-13T10:57:34

Patch Set 2: Patch Set 1 was rebased

@marsell commented at 2019-02-13T13:00:31

Patch Set 2:

No, I didn't run the /usr/vm/test tests -- thanks for pointing that out! I have now, and all tests pass.

@kusor commented at 2019-02-13T16:27:07

Patch Set 2: Code-Review+1 Integration-Approval+1

@kusor commented at 2019-02-15T15:38:49

Patch Set 3: Code-Review+1

Looks fine, probably sightly more clear now.

@marsell commented at 2019-02-15T15:40:08

Patch Set 4: Patch Set 3 was rebased

@mgerdts commented at 2019-02-15T16:34:11

Patch Set 4:

(2 comments)

@marsell commented at 2019-02-15T22:42:44

Patch Set 4:

(2 comments)

@mgerdts commented at 2019-02-18T05:43:14

Patch Set 4:

(1 comment)

@bahamas10 commented at 2019-02-19T17:49:02

Patch Set 4:

(1 comment)

@bahamas10 commented at 2019-02-19T17:51:56

Patch Set 4:

(1 comment)

Patch Set 4 code comments

src/vm/node_modules/VM.js#4609 @mgerdts

It seems the initial intent was to get an integer, but it would get a string. Now we are potentially getting a float.

Should the intent be preserved, but ensure we get an integer? Otherwise I worry that we could end up setting tmpfs to a float value that eventually translates into mount -F tmpfs -o size=512.5m - /tmp. This will fail because size must be an integer.

t = "4" / "3"
1.3333333333333333
Number(t)
1.3333333333333333
Number(t).toFixed()
'1'
Number(t.toFixed())
1

src/vm/node_modules/VM.js#4609 @marsell

This was the only instance of toFixed() in the file.

I can certainly switch this to something more rigorous, but then we have a bigger problem on our hands -- the rest of the file. I'd argue this deserves a separate ticket.

src/vm/node_modules/VM.js#4609 @mgerdts

With this fix:

[root@buglets ~]# vmadm get c8d2b761-b0fd-ce85-d7a6-837061e94cce | json max_physical_memory tmpfs
1024
1024
[root@buglets ~]# vmadm update c8d2b761-b0fd-ce85-d7a6-837061e94cce max_physical_memory=1500
Successfully updated VM c8d2b761-b0fd-ce85-d7a6-837061e94cce
[root@buglets ~]# vmadm get c8d2b761-b0fd-ce85-d7a6-837061e94cce | json max_physical_memory tmpfs
1500
1500
[root@buglets ~]# vmadm update c8d2b761-b0fd-ce85-d7a6-837061e94cce tmpfs=500
Successfully updated VM c8d2b761-b0fd-ce85-d7a6-837061e94cce
[root@buglets ~]# vmadm get c8d2b761-b0fd-ce85-d7a6-837061e94cce | json max_physical_memory tmpfs
1500
500
[root@buglets ~]# vmadm update c8d2b761-b0fd-ce85-d7a6-837061e94cce max_physical_memory=1000
Invalid value(s) for: tmpfs

If the fix is not present:

[root@buglets ~]# umount /tmp/VM.js
[root@buglets ~]# vmadm update c8d2b761-b0fd-ce85-d7a6-837061e94cce max_physical_memory=1000
Successfully updated VM c8d2b761-b0fd-ce85-d7a6-837061e94cce
[root@buglets ~]# vmadm get c8d2b761-b0fd-ce85-d7a6-837061e94cce | json max_physical_memory tmpfs
1000
333

src/vm/node_modules/VM.js#4609 @bahamas10

I don't think the whole file needs to be taken into account here, it seems like this line is simply a bug (in current master)... currently it results in a strinng, when it should result in an integer.

src/vm/node_modules/VM.js#16611 @mgerdts

vmadm(1M) says:

   tmpfs:

       This property specifies how much of the VM´s memory will be available
       for the /tmp filesystem. This is only available for OS VMs, and doesn´t
       make any sense for HVM VMs.

       If set to 0 this indicates that you would like to not have /tmp mounted
       as tmpfs at all. When changing to/from a "0" value, the VM must be
       rebooted in order for the change to take effect.

This code seems to ignore setting tmpfs to 0 rather than removing the tmpfs property (remove attr name=tmpfs).

src/vm/node_modules/VM.js#16611 @marsell

var tmpfs is taken from the existing VM, not the new changes.

The code is currently changing an existing VM's tmpfs from 0 to the payload's ram value, which I don't think we want to be doing here. And if we do, vmadm isn't actually applying that change (the VM's tmpfs remains 0).

src/vm/node_modules/VM.js#16611 @bahamas10

This code here doesn't control what properties set, but instead what properties get waited on to be changed by vminfod.

@bahamas10 commented at 2019-02-25T15:01:46

Patch Set 5: Code-Review+1

Is there a test that can be written for this specifically? I assume, since this a bug but tests currently pass just fine on the latest master branch that there is not. Could a test be added to test-update.js for instance to resize a VM to trigger this bug, and ensure that this change fixes it?

I ask because I have a change planned soon that may touch logic around tmpfs and I don't want to accidentally reintroduce this bug.

@mgerdts commented at 2019-02-25T15:06:17

Patch Set 5:

Looks good, but could use a test as Dave suggests.