Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

builder: don't enable iothread for all scsi #249

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

lbajolet-hashicorp
Copy link
Contributor

When a SCSI disk is specified for a configuration, the iothread argument should only be set if requested, and not all the time.

Because of a priority problem though, all the SCSI disks ended-up with iothread set, even if it was not requested.

This commit fixes that, and ensures it is fixed by adding some tests for it.

Fixes: #248

@lbajolet-hashicorp lbajolet-hashicorp requested a review from a team as a code owner January 9, 2024 18:50
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I confirmed the fix by testing against main.

@@ -286,7 +286,7 @@ func generateProxmoxDisks(disks []diskConfig) proxmox.QemuDevices {
setDeviceParamIfDefined(devs[idx], "cache", disks[idx].CacheMode)
setDeviceParamIfDefined(devs[idx], "format", disks[idx].DiskFormat)

if devs[idx]["type"] == "scsi" || devs[idx]["type"] == "virtio" &&
if (devs[idx]["type"] == "scsi" || devs[idx]["type"] == "virtio") &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 on the parenthesis

builder/proxmox/common/step_start_vm_test.go Outdated Show resolved Hide resolved
When a SCSI disk is specified for a configuration, the iothread argument
should only be set if requested, and not all the time.

Because of a priority problem though, all the SCSI disks ended-up with
iothread set, even if it was not requested.

This commit fixes that, and ensures it is fixed by adding some tests for
it.
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the no_iothread_scsi_not_requested branch from 1f8645a to 6303c37 Compare January 11, 2024 15:05
@lbajolet-hashicorp lbajolet-hashicorp merged commit 9886708 into main Jan 11, 2024
12 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the no_iothread_scsi_not_requested branch January 11, 2024 16:12
@xoxys
Copy link

xoxys commented Jan 19, 2024

Would you mind tagging a new release so we can use this fix?

@lbajolet-hashicorp
Copy link
Contributor Author

Hi @xoxys,

Good call, I'll release Proxmox this week.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iothread=1 being set on all scsi disks regardless of io_thread option
3 participants