TRITON-2552 CreateMachineDisk fails to add a disk to an existing instance

docs/index.md

@@ -15,7 +15,7 @@ markdown2extras: tables, code-friendly
     Copyright 2021 Joyent, Inc.
     Copyright 2021 The University of Queensland
     Copyright 2024 MNX Cloud, Inc.
-    Copyright 2025 Edgecast Cloud LLC.
+    Copyright 2026 Edgecast Cloud LLC.
 -->
 
 
@@ -904,7 +904,7 @@ The section describes API changes in CloudAPI versions.
   invalid `block_size` on a disk will now report errors rather than silently
   fail.
 
-# 9.18.0
+## 9.18.0
 
 - Support for ED25519 keys.
 
@@ -5435,6 +5435,7 @@ id         | UUID     | Unique id for this disk
 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.
 
 ### Returns
 
@@ -6684,6 +6685,7 @@ dangerous_allow_shrink | Boolean  | Optional, whether a disk can reduce size
 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
 boot      | Boolean  | If this is the VM's boot disk
 state     | String   | Current state of disk (i.e. 'resizing')
 
@@ -6754,6 +6756,7 @@ Fetch a specific disk on a bhyve VM.
 id        | String   | This disk's UUID
 pci_slot  | String   | This disk's PCI slot
 size      | Number   | Size in MiB
+block_size | Number  | The disk's block size in bytes
 boot      | Boolean  | If this is the VM's boot disk
 state     | String   | Current state of disk
 
@@ -6820,6 +6823,7 @@ List all disk on a bhyve VM.
 id        | String   | This disk's UUID
 pci_slot  | String   | This disk's PCI slot
 size      | Number   | Size in MiB
+block_size | Number  | The disk's block size in bytes
 boot      | Boolean  | If this is the VM's boot disk
 state     | String   | Current state of disk
 

lib/endpoints/disks.js

@@ -7,6 +7,7 @@
 /*
  * Copyright 2020 Joyent, Inc.
  * Copyright 2024 MNX Cloud, Inc.
+ * Copyright 2026 Edgecast Cloud LLC.
  */
 
 /*
@@ -20,7 +21,6 @@ var util = require('util'),
 var assert = require('assert-plus');
 var restify = require('restify');
 var string2uuid = require('uuid-by-string');
-var diskValidation = require('../validation/disk');
 
 // --- Globals
 
@@ -58,7 +58,7 @@ function createDisk(req, res, next) {
     req.vm.disks = req.vm.disks || [];
 
     if (req.vm.brand !== 'bhyve') {
-        next(InvalidArgumentError('Disk Creation is supported only for ' +
+        next(new InvalidArgumentError('Disk Creation is supported only for ' +
             'BHYVE VMs'));
         return;
     }
@@ -89,24 +89,6 @@ function createDisk(req, res, next) {
         return;
     }
 
-    // Check requested block size, if any.
-    req.vm.disks.forEach(function vrs(disk, i) {
-        if (disk.hasOwnProperty('block_size')) {
-            if (disk.hasOwnProperty('image_uuid')) {
-                throw new InvalidArgumentError(
-                    'Cannot set block_size and image_uuid.'
-                );
-            }
-            if (!diskValidation.validRecordSize(disk.block_size)) {
-                throw new InvalidArgumentError(
-                    'Invalid block_size: ' + disk.block_size +
-                    ' on disk: ' + i + '. Must be 512-131072 and ' +
-                    'a power of 2.'
-                );
-            }
-        }
-    });
-
     if (pciSlot) {
         var diskId = getDiskUuid(vmUuid, pciSlot);
     }
@@ -665,5 +647,6 @@ function mount(server, before, pre, post) {
 
 
 module.exports = {
+    _createDisk: createDisk, // exported for testing only
     mount: mount
 };

lib/validation/disk.js

@@ -6,18 +6,37 @@
 
 /*
  * Copyright 2024 MNX Cloud, Inc.
+ * Copyright 2026 Edgecast Cloud LLC.
  */
 
 
-// This is the same function from vmadm.
+/*
+ * This mirrors vmadm, which validates disks.*.block_size as an integer (a
+ * string that parses cleanly as one is accepted too) before applying the range
+ * and power-of-2 checks below. Without the type check those comparisons coerce
+ * non-numbers to NaN, which is false against every bound, so "garbage", {} and
+ * 4096.5 would all be reported as valid.
+ */
 function validRecordSize(candidate) {
-    if (candidate < 512) {
+    var size;
+
+    if (typeof (candidate) !== 'number' && typeof (candidate) !== 'string') {
+        return (false);
+    }
+
+    size = Number(candidate);
+
+    if (!Number.isInteger(size)) {
+        return (false);
+    }
+
+    if (size < 512) {
         // too low
         return (false);
-    } else if (candidate > 131072) {
+    } else if (size > 131072) {
         // too high
         return (false);
-    } else if ((candidate & (candidate - 1)) !== 0) {
+    } else if ((size & (size - 1)) !== 0) {
         // not a power of 2
         return (false);
     }

test/createDisk.test.js

@@ -0,0 +1,220 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+/*
+ * Copyright 2026 Edgecast Cloud LLC.
+ */
+
+/*
+ * Unit tests for the CreateMachineDisk handler. These need no live datacenter:
+ * the VM is supplied directly and vmapi.createDisk() is stubbed.
+ */
+
+var test = require('tape');
+
+var createDisk = require('../lib/endpoints/disks')._createDisk;
+
+// --- Globals
+
+var IMAGE_UUID = 'ede15ae3-a2ed-4636-a4d5-c130cbd9c297';
+var OWNER_UUID = '7b315468-c6be-46dc-b99b-9c1f59224693';
+var VM_UUID = '3fdfbfe8-1c98-4d0e-b95b-1a4d5f8b7d3a';
+var LOGIN = 'bob';
+
+// --- Helpers
+
+/*
+ * An image-backed bhyve boot disk as vmadm actually reports it. block_size is
+ * read back from the zvol's volblocksize, so it is always present, including
+ * alongside image_uuid. See TritonDataCenter/sdc-cloudapi#156.
+ */
+function bootDisk() {
+    return {
+        boot: true,
+        block_size: 8192,
+        image_uuid: IMAGE_UUID,
+        pci_slot: '0:4:0',
+        size: 10240
+    };
+}
+
+function mkReq(opts) {
+    var created = [];
+
+    return {
+        created: created,
+        req: {
+            _auditCtx: { caller: LOGIN },
+            account: { login: LOGIN, uuid: OWNER_UUID },
+            getId: function getId() { return 'test-request-id'; },
+            log: { debug: function debug() {} },
+            params: opts.params,
+            sdc: {
+                vmapi: {
+                    createDisk: function stubCreateDisk(params, _opts, cb) {
+                        created.push(params);
+                        setImmediate(cb, null, { job_uuid: 'test-job' });
+                    }
+                }
+            },
+            vm: opts.vm
+        }
+    };
+}
+
+function mkRes() {
+    var res = { headers: {} };
+
+    res.header = function header(name, value) {
+        res.headers[name] = value;
+    };
+
+    res.send = function send(body) {
+        res.body = body;
+    };
+
+    return res;
+}
+
+function stoppedBhyveVm(disks) {
+    return {
+        brand: 'bhyve',
+        disks: disks,
+        flexible_disk_size: 102400,
+        state: 'stopped',
+        uuid: VM_UUID
+    };
+}
+
+// --- Tests
+
+/*
+ * The regression from TRITON-2459: createDisk validated the VM's persisted
+ * disks rather than the requested one, so every image-backed bhyve boot disk
+ * tripped the "Cannot set block_size and image_uuid" check. The throw was
+ * synchronous, so it escaped to cloudapi's uncaughtException handler and the
+ * caller saw a 500 InternalError.
+ */
+test('CreateMachineDisk accepts a VM with an image-backed boot disk',
+    function (t) {
+    var ctx = mkReq({
+        params: { pci_slot: '0:4:4', size: 20480 },
+        vm: stoppedBhyveVm([
+            bootDisk(),
+            { block_size: 4096, pci_slot: '0:4:1', size: 10240 }
+        ])
+    });
+    var res = mkRes();
+
+    createDisk(ctx.req, res, function next(err) {
+        t.ifError(err, 'err');
+
+        t.equal(ctx.created.length, 1, 'vmapi.createDisk() called once');
+
+        var args = ctx.created[0];
+        t.equal(args.uuid, VM_UUID, 'vm uuid');
+        t.equal(args.owner_uuid, OWNER_UUID, 'owner uuid');
+        t.equal(args.pci_slot, '0:4:4', 'pci_slot');
+        t.equal(args.size, 20480, 'size');
+        t.equal(args.origin, 'cloudapi', 'origin');
+        t.ok(!args.hasOwnProperty('block_size'),
+            'block_size not sent to vmapi');
+
+        t.ok(res.body, 'response body');
+        t.equal(res.body.pci_slot, '0:4:4', 'response pci_slot');
+        t.equal(res.body.size, 20480, 'response size');
+        t.equal(res.body.state, 'creating', 'response state');
+        t.equal(res.body.boot, false, 'response boot');
+        t.equal(res.headers.Location,
+            '/' + LOGIN + '/machines/' + VM_UUID + '/disks/' + res.body.id,
+            'Location header');
+
+        t.end();
+    });
+});
+
+
+/*
+ * block_size on an existing disk describes the zvol as it already is. Whether
+ * it is a value cloudapi would accept on a create request is irrelevant here,
+ * and must not block adding an unrelated disk.
+ */
+test('CreateMachineDisk does not validate existing disks', function (t) {
+    var ctx = mkReq({
+        params: { pci_slot: '0:4:4', size: 20480 },
+        vm: stoppedBhyveVm([{ block_size: 1234, pci_slot: '0:4:0',
+            size: 10240 }])
+    });
+
+    createDisk(ctx.req, mkRes(), function next(err) {
+        t.ifError(err, 'err');
+        t.equal(ctx.created.length, 1, 'vmapi.createDisk() called once');
+        t.end();
+    });
+});
+
+
+test('CreateMachineDisk with size "remaining"', function (t) {
+    var ctx = mkReq({
+        params: { pci_slot: '0:4:4', size: 'remaining' },
+        vm: stoppedBhyveVm([bootDisk(), { pci_slot: '0:4:1', size: 512 }])
+    });
+
+    createDisk(ctx.req, mkRes(), function next(err) {
+        t.ifError(err, 'err');
+        t.equal(ctx.created[0].size, 102400 - 10240 - 512, 'remaining size');
+        t.end();
+    });
+});
+
+
+test('CreateMachineDisk without pci_slot returns 202', function (t) {
+    var ctx = mkReq({
+        params: { size: 20480 },
+        vm: stoppedBhyveVm([bootDisk()])
+    });
+    var res = mkRes();
+
+    createDisk(ctx.req, res, function next(err) {
+        t.ifError(err, 'err');
+        t.equal(ctx.created.length, 1, 'vmapi.createDisk() called once');
+        t.equal(res.body, 202, 'status 202');
+        t.end();
+    });
+});
+
+
+test('CreateMachineDisk rejects non-bhyve VMs', function (t) {
+    var ctx = mkReq({
+        params: { pci_slot: '0:4:4', size: 20480 },
+        vm: { brand: 'joyent', disks: [], state: 'stopped', uuid: VM_UUID }
+    });
+
+    createDisk(ctx.req, mkRes(), function next(err) {
+        t.ok(err, 'err');
+        t.equal(err.body.code, 'InvalidArgument', 'err code');
+        t.equal(ctx.created.length, 0, 'vmapi.createDisk() not called');
+        t.end();
+    });
+});
+
+
+test('CreateMachineDisk rejects a running VM', function (t) {
+    var vm = stoppedBhyveVm([bootDisk()]);
+    vm.state = 'running';
+
+    var ctx = mkReq({
+        params: { pci_slot: '0:4:4', size: 20480 },
+        vm: vm
+    });
+
+    createDisk(ctx.req, mkRes(), function next(err) {
+        t.ok(err, 'err');
+        t.equal(err.body.code, 'InvalidArgument', 'err code');
+        t.equal(ctx.created.length, 0, 'vmapi.createDisk() not called');
+        t.end();
+    });
+});