From f63f3381aa190e58879caf9bc4b3c45b326164ca Mon Sep 17 00:00:00 2001 From: Nick Wilkens Date: Mon, 24 Aug 2026 08:29:23 -0400 Subject: [PATCH 1/4] Fix CreateMachineDisk 500 on bhyve VMs with image-backed boot disks (#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) Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv --- lib/endpoints/disks.js | 23 +---- test/createDisk.test.js | 220 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 223 insertions(+), 20 deletions(-) create mode 100644 test/createDisk.test.js diff --git a/lib/endpoints/disks.js b/lib/endpoints/disks.js index 9e1951c3..51731a32 100644 --- a/lib/endpoints/disks.js +++ b/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 }; diff --git a/test/createDisk.test.js b/test/createDisk.test.js new file mode 100644 index 00000000..07964685 --- /dev/null +++ b/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(); + }); +}); From 0bc873d52491a39406d50bb457e1c58011ceb132 Mon Sep 17 00:00:00 2001 From: Nick Wilkens Date: Mon, 24 Aug 2026 08:31:39 -0400 Subject: [PATCH 2/4] Stop asserting exact disk shapes that now carry block_size (#156) 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) Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv --- test/machines.94.test.js | 45 ++++++++++++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/test/machines.94.test.js b/test/machines.94.test.js index bc2c3319..14b3d804 100644 --- a/test/machines.94.test.js +++ b/test/machines.94.test.js @@ -6,6 +6,7 @@ /* * Copyright 2020 Joyent, Inc. + * Copyright 2026 Edgecast Cloud LLC. */ var test = require('tape'); @@ -13,6 +14,7 @@ var util = require('util'); var vasync = require('vasync'); var common = require('./common'); +var diskValidation = require('../lib/validation/disk'); var machinesCommon = require('./machines/common'); var checkMachine = machinesCommon.checkMachine; var deleteMachine = require('./machines/delete'); @@ -79,6 +81,31 @@ function checkDisks(t, expectedDisks, actualDisks) { }); } +/* + * 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. + */ +function checkDiskEqual(t, actualDisk, expectedDisk, msg) { + var disk = Object.assign({}, actualDisk); + + if (disk.hasOwnProperty('block_size')) { + t.ok(diskValidation.validRecordSize(disk.block_size), + 'valid block_size: ' + disk.block_size); + delete disk.block_size; + } + + t.deepEqual(disk, expectedDisk, msg); +} + +function checkDisksEqual(t, actualDisks, expectedDisks, msg) { + t.equal(actualDisks.length, expectedDisks.length, 'disk count'); + + actualDisks.forEach(function check(disk, idx) { + checkDiskEqual(t, disk, expectedDisks[idx], msg); + }); +} + function checkDisksQuota(t, disks, quota) { var disksSum = disks.reduce(function sumDisk(sum, disk) { return sum + disk.size; @@ -440,7 +467,7 @@ test('No disks/inflexible disk package', function (suite) { CLIENT.get(diskPath, function gotDisk(err, req, res, disk) { t.ifError(err); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, boot: false, pci_slot: '0:4:1', @@ -717,7 +744,7 @@ test('Disks/flexible disk package', function (suite) { t.strictEqual(body.flexible, true); checkDisksQuota(t, body.disks, BHYVE_128_FLEXIBLE.quota - body.free_space); - t.deepEqual(body.disks, expectedDisks); + checkDisksEqual(t, body.disks, expectedDisks); t.end(); }); }); @@ -764,7 +791,7 @@ test('Disks/flexible disk package', function (suite) { }, function resizeDisk(err, req, res, disk) { t.ifError(err, 'err'); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:1', size: 512, @@ -793,7 +820,7 @@ test('Disks/flexible disk package', function (suite) { return; } - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:1', size: 128, @@ -844,7 +871,7 @@ test('Disks/flexible disk package', function (suite) { } t.ifError(err, 'err'); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:1', size: 128, @@ -901,7 +928,7 @@ test('Disks/flexible disk package', function (suite) { t.ifError(err, 'err'); - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:4', size: 256, @@ -984,7 +1011,7 @@ test('Disks/flexible disk package', function (suite) { t.ifError(err, 'err'); if (count === 1) { - t.deepEqual(disk, { + checkDiskEqual(t, disk, { id: DISK_UUID, pci_slot: '0:4:2', size: BHYVE_128_FLEXIBLE.quota - @@ -1096,7 +1123,7 @@ test('Disks sum to quota/flex disk package', function (suite) { t.strictEqual(body.flexible, true); checkDisksQuota(t, body.disks, BHYVE_128_FLEXIBLE.quota); - t.deepEqual(body.disks, expectedDisks); + checkDisksEqual(t, body.disks, expectedDisks); t.end(); }); }); @@ -1203,7 +1230,7 @@ test('Disks with remaining/flex disk package', function (suite) { t.strictEqual(body.flexible, true); checkDisksQuota(t, body.disks, BHYVE_128_FLEXIBLE.quota - body.free_space); - t.deepEqual(body.disks, expectedDisks); + checkDisksEqual(t, body.disks, expectedDisks); t.end(); }); }); From 7e6beeb2d81e22affe895e41640900b202b456d9 Mon Sep 17 00:00:00 2001 From: Nick Wilkens Date: Mon, 24 Aug 2026 08:32:32 -0400 Subject: [PATCH 3/4] Type-check block_size before validating it 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) Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv --- lib/validation/disk.js | 27 ++++++++++++--- test/validationDisk.test.js | 68 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+), 4 deletions(-) create mode 100644 test/validationDisk.test.js diff --git a/lib/validation/disk.js b/lib/validation/disk.js index cfc99c8d..f366f331 100644 --- a/lib/validation/disk.js +++ b/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); } diff --git a/test/validationDisk.test.js b/test/validationDisk.test.js new file mode 100644 index 00000000..231684d0 --- /dev/null +++ b/test/validationDisk.test.js @@ -0,0 +1,68 @@ +/* + * 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. + */ + +var test = require('tape'); + +var validRecordSize = require('../lib/validation/disk').validRecordSize; + +// --- Tests + +test('validRecordSize accepts the supported range', function (t) { + [512, 1024, 4096, 8192, 16384, 65536, 131072].forEach(function chk(size) { + t.ok(validRecordSize(size), size + ' is valid'); + }); + + t.end(); +}); + + +test('validRecordSize accepts integer strings', function (t) { + t.ok(validRecordSize('8192'), '"8192" is valid'); + t.notOk(validRecordSize('8193'), '"8193" is not a power of 2'); + t.end(); +}); + + +test('validRecordSize rejects out-of-range and non-power-of-2', function (t) { + [0, 511, 1000, 8193, 131073, 262144, -8192].forEach(function chk(size) { + t.notOk(validRecordSize(size), size + ' is invalid'); + }); + + t.end(); +}); + + +/* + * Without a type check the range comparisons coerce these to NaN, which is + * false against every bound, and (NaN & NaN) is 0, so each one used to be + * reported as a valid record size. + */ +test('validRecordSize rejects non-integers', function (t) { + var cases = [ + ['4096.5', 4096.5], + ['"garbage"', 'garbage'], + ['{}', {}], + ['[]', []], + ['[8192]', [8192]], + ['null', null], + ['undefined', undefined], + ['NaN', NaN], + ['Infinity', Infinity], + ['true', true], + ['false', false], + ['empty string', ''] + ]; + + cases.forEach(function chk(c) { + t.notOk(validRecordSize(c[1]), c[0] + ' is invalid'); + }); + + t.end(); +}); From 95964fa28dc77ee35420ebc3331a2a307aab15d4 Mon Sep 17 00:00:00 2001 From: Nick Wilkens Date: Mon, 24 Aug 2026 08:33:44 -0400 Subject: [PATCH 4/4] Document block_size on disk requests and responses 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) Claude-Session: https://claude.ai/code/session_01EcKUgjXDfGyg1ssRFQHmLv --- docs/index.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/docs/index.md b/docs/index.md index 72f00844..d9329a0d 100644 --- a/docs/index.md +++ b/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