TRITON-2552 CreateMachineDisk fails to add a disk to an existing instance by nwilkens · Pull Request #157 · TritonDataCenter/sdc-cloudapi

nwilkens · 2026-08-24T12:56:34Z

Fixes #156.

createDisk() validated the VM's existing disks rather than the requested one.
vmadm always reports block_size on a zvol (read back from volblocksize), so
every image-backed boot disk has both block_size and image_uuid and trips
the "Cannot set block_size and image_uuid" check. The throw is synchronous, so
it reaches the uncaughtException handler and callers get a 500. This has been
broken for all bhyve VMs since 9.19.0 (TRITON-2459).

CreateMachineDisk accepts no block_size at all: not in the docs, not sent to
VMAPI, not handled by VMAPI's create_disk. So the check is removed rather than
rewired. The equivalent check in getCreateOptions() is correct and stays, it
examines disks supplied to CreateMachine and its throws are caught.

Also in here:

The deepEqual disk assertions in machines.94, invalidated when TRITON-2459
added block_size to both translators.
validRecordSize() only compared its argument, so anything coercing to NaN
("garbage", {}, 4096.5) was reported valid. Type-check first, matching vmadm.
block_size added to the docs field tables it applies to.

…156) createDisk validated req.vm.disks (the VM's persisted disks) instead of the requested disk. Image-backed disks always report block_size from the zvol's volblocksize, so the check threw and cloudapi returned an InternalError. Regression from TRITON-2459. Also adds the missing `new` on the non-bhyve error, which passed undefined to next() and dropped the error. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv

TRITON-2459 added block_size to both disk translators, which broke every t.deepEqual() on a disk object in machines.94. block_size comes from the zvol's volblocksize and varies with pool layout, so check it for validity and compare the rest exactly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv

validRecordSize() only compared its argument, so anything that coerces to NaN ("garbage", {}, 4096.5) compared false against every bound and was reported valid. vmadm type-checks disks.*.block_size separately; cloudapi copied only the range helper. Match vmadm and check the type here too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv

TRITON-2459 added block_size to the disk translators and to CreateMachine input, but only mentioned it in the changelog. Add it to the field tables it actually applies to, and note that it cannot be set on the boot disk. Also fixes the heading level on the 9.18.0 notes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv

nshalman

Looks good to me. I'd prefer an extra +1 before it lands, though.

danmcd · 2026-08-24T13:27:16Z

Thank you for including tests on this PR. Someone will need to create a TRITON- ticket (which will refer to TRITON-2459) and please make sure testing and what-not get reported there once created

danmcd · 2026-08-24T14:00:36Z

Since it was discovered publically, I've set https://smartos.org/bugview/TRITON-2552 as public so folks can see.

danmcd

Needs package.json update, probably to ~~9.19.1~~ (edit) 9.21.1 since this is a small bug fix for the bump to 9.19. More to come.

danmcd

The PR report is very sketchy on WHY this fix purportedly works. I had to go digging for other callers of validRecordSize() in the current master to begin to see what an analysis in the ticket should say:

kebe(~/ws/sdc-cloudapi)[0]% git grep validRecordSize
lib/endpoints/disks.js:            if (!diskValidation.validRecordSize(disk.block_size)) {
lib/machines.js:                        if (!diskValidation.validRecordSize(disk.block_size)) {
lib/validation/disk.js:function validRecordSize(candidate) {
lib/validation/disk.js:    validRecordSize: validRecordSize
kebe(~/ws/sdc-cloudapi)[0]%

So the one in endpoints/disks.js is getting yanked, seemingly with good reason, BUT, that only leaves a TEST caller for validRecordSize().

Let's look at what's in this PR:

CreateMachineDisk accepts no block_size at all: not in the docs, not sent to VMAPI, not handled by VMAPI's create_disk.

THAT is the insight. The action suggested next: So the check is removed rather than rewired. obscures the real problem:

vmadm(8) has block_size as a parameter for VM disks:

[root@nuc ~]# vmadm get 9335517e-86fe-44d7-8f92-c7b057a0b36f | json disks
[
  {
    "path": "/dev/zvol/rdsk/zones/9335517e-86fe-44d7-8f92-c7b057a0b36f/disk0",
    "boot": true,
    "model": "virtio",
    "media": "disk",
    "image_size": 10240,
    "image_uuid": "cccbdd29-adc0-4231-ac2c-26b4a762df5f",
    "pci_slot": "0:4:0",
    "uuid": "35896073-95c8-407a-a953-0ec676efe259",
    "zfs_filesystem": "zones/9335517e-86fe-44d7-8f92-c7b057a0b36f/disk0",
    "zpool": "zones",
    "size": 10240,
    "compression": "off",
    "refreservation": 10563,
    "block_size": 8192
  }
]
[root@nuc ~]# zfs get volblocksize zones/9335517e-86fe-44d7-8f92-c7b057a0b36f/disk0
NAME                                              PROPERTY      VALUE     SOURCE
zones/9335517e-86fe-44d7-8f92-c7b057a0b36f/disk0  volblocksize  8K        default
[root@nuc ~]#

But, as the likely-LLM report in this PR said, block_size is not handled by VMAPI!

This fix is necessary, but not sufficient, to solve the filer's problem. We need:

VMAPI to support block_size in its new disk objects if it doesn't already. (It reports them, but it is not clear if it will ACCEPT them!)
This fix (modulo any other flaws) to check ONLY the block_size on the new disks, needs to be correct and present.

This PR covers the second part, not the first.

danmcd · 2026-08-24T14:07:04Z

 boot       | Boolean  | If `true`, this is the boot disk
 image      | UUID     | The image from which the disk was created
 size       | Integer  | The size of the disk in mebibytes or "remaining". If "remaining", size will be set to the difference between the package quota and sum of the other disks.
+block_size | Integer  | Optional. The disk's block size in bytes. Must be 512-131072 and a power of 2. Cannot be set on an image-backed disk, which includes the boot disk. When not specified, the block size is chosen for you and reported back on subsequent requests.


If it's optional, it must have a default if not present, right? What is that default?

VMAPI does not document this either. If we can confirm VMAPI creation with variable block_size parameters we should update VMAPI.

I recommend withdrawing this change until we fix it concurrently with VMAPI.

danmcd · 2026-08-24T14:08:02Z


    if (req.vm.brand !== 'bhyve') {
-        next(InvalidArgumentError('Disk Creation is supported only for ' +
+        next(new InvalidArgumentError('Disk Creation is supported only for ' +


Good catch. I wonder if there are other (maybe not for this commit) issues like this? A JS-savvy person (e.g. @travispaul ) may know where to look?

danmcd · 2026-08-24T14:13:06Z

+/*
+ * block_size is reported back from the zvol's volblocksize, which varies with
+ * the pool's layout, so it can only be checked for validity and not against a
+ * fixed value. Everything else must match exactly.


This might answer my question above ("the hypervisor's virtual disk block size" or use the literal "zvol's volblocksize"), but it needs to be in the docs.

Strike the above given we need to plumb these doc (and later disk-update) fixes through VMAPI to on-machine vmadm(8).

danmcd · 2026-08-24T15:29:31Z

VMAPI to support block_size in its new disk objects if it doesn't already. (It reports them, but it is not clear if it will ACCEPT them!)

Good news from TRITON-2459:

Turns out, you can already specify custom block sizes and cloudapi doesn’t filter this out. It’s passed directly to vmapi, which is then passed directly to vmadm.

danmcd

Much closer than I had initially thought. Please update around the documentation comments I cited.

The creation of a new disk on an existing VM needs much updated in both CLOUDAPI and VMAPI, with a small possibility of vmadm(8) ALSO needing work if block_size is to be a creation-time parameter.

For new-VM creation, those whole disk objects should be getting passed straight on to vmadm(8) through VMAPI or CLOUDAPI sending whole-json to VMAPI.

This fix mostly fixed the bug reported ("Cannot add to existing instance"), modulo documentation nits. Understanding the CAUSE of this bug was what led to what still needs to be done.

danmcd · 2026-08-24T19:34:58Z

        return;
    }

-    // Check requested block size, if any.


I finally think I know what's going on here.

LLM analysis was more right than I initially thought, SPECIFICALLY on THIS section of code which only creates new disks for an existing VM. The existing documentation has disk-parameters for new instance/VM creation, where block_size can be created.

This check was put in because the author-and-reviewers thought the VM object contained the new disk to create, when it fact it does not. The reporting of block-size portion of TRITON-2459 was effective, but the specification of block_size in new disks for existing machiens needs DEEP help in VMAPI, its documentation once built, CLOUDAPI, and ITS documentation once built

danmcd · 2026-08-24T19:42:29Z

 boot       | Boolean  | If `true`, this is the boot disk
 image      | UUID     | The image from which the disk was created
 size       | Integer  | The size of the disk in mebibytes or "remaining". If "remaining", size will be set to the difference between the package quota and sum of the other disks.
+block_size | Integer  | Optional. The disk's block size in bytes. Must be 512-131072 and a power of 2. Cannot be set on an image-backed disk, which includes the boot disk. When not specified, the block size is chosen for you and reported back on subsequent requests.


VMAPI does not document this either. If we can confirm VMAPI creation with variable block_size parameters we should update VMAPI.

I recommend withdrawing this change until we fix it concurrently with VMAPI.

danmcd · 2026-08-24T19:42:52Z

 id        | String   | This disk's UUID
 pci_slot  | String   | This disk's PCI slot
 size      | Number   | Size in MiB (before resize)
+block_size | Number  | The disk's block size in bytes


This and its friends below are good, please keep them.

danmcd · 2026-08-24T19:44:49Z

+/*
+ * block_size is reported back from the zvol's volblocksize, which varies with
+ * the pool's layout, so it can only be checked for validity and not against a
+ * fixed value. Everything else must match exactly.


Strike the above given we need to plumb these doc (and later disk-update) fixes through VMAPI to on-machine vmadm(8).

danmcd · 2026-08-24T19:46:56Z

 var assert = require('assert-plus');
 var restify = require('restify');
 var string2uuid = require('uuid-by-string');
-var diskValidation = require('../validation/disk');


You may wish to keep this commented-out with a TODO around it; your call.

nwilkens and others added 4 commits August 24, 2026 08:30

nshalman approved these changes Aug 24, 2026

View reviewed changes

danmcd changed the title ~~Fix CreateMachineDisk 500 on bhyve VMs with image-backed boot disks~~ TRITON-2552 CreateMachineDisk fails to add a disk to an existing instance Aug 24, 2026

danmcd requested changes Aug 24, 2026

View reviewed changes

danmcd reviewed Aug 24, 2026

View reviewed changes

danmcd requested changes Aug 24, 2026

View reviewed changes

Uh oh!

Conversation

nwilkens commented Aug 24, 2026

Uh oh!

nshalman left a comment

Choose a reason for hiding this comment

Uh oh!

danmcd commented Aug 24, 2026

Uh oh!

danmcd commented Aug 24, 2026

Uh oh!

danmcd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmcd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmcd commented Aug 24, 2026

Uh oh!

danmcd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danmcd left a comment •

edited

Loading