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

qemu driver: Add option to configure drive_interface #11864

Merged
merged 4 commits into from
Jun 10, 2022
Merged

qemu driver: Add option to configure drive_interface #11864

merged 4 commits into from
Jun 10, 2022

Conversation

bogue1979
Copy link
Contributor

For QEMU the options -drive file=image.img and -drive file=image.img,if=ide are identical.
This change will use the second variant as default and allow to define an image file using the virtio driver inside of the VM.
Using this driver should improve the disk IO a lot.

@bogue1979
Copy link
Contributor Author

Ping

@tgross tgross added this to Needs Triage in Nomad - Community Issues Triage via automation Mar 11, 2022
@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @bogue1979! This looks great. I think the only thing that comes to mind here is being sure about the default behavior for QEMU. Do you know if ide is platform-specific behavior and that QEMU isn't doing something "smart" based on the available interfaces on the platform? I want to make sure we're not introducing a breaking change, and that we're not boxing ourselves in for future updates we have on the roadmap for expanding QEMU support beyond x86_64 platform.

The man page has this to say:

if=interface
This option defines on which type on interface the drive is connected. Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.

With nothing about the default.

drivers/qemu/driver.go Outdated Show resolved Hide resolved
@tgross tgross moved this from Needs Triage to In Progress in Nomad - Community Issues Triage Mar 31, 2022
@tgross tgross self-assigned this Mar 31, 2022
@bogue1979
Copy link
Contributor Author

Hi @bogue1979! This looks great. I think the only thing that comes to mind here is being sure about the default behavior for QEMU. Do you know if ide is platform-specific behavior and that QEMU isn't doing something "smart" based on the available interfaces on the platform? I want to make sure we're not introducing a breaking change, and that we're not boxing ourselves in for future updates we have on the roadmap for expanding QEMU support beyond x86_64 platform.

The man page has this to say:

if=interface
This option defines on which type on interface the drive is connected. Available types are: ide, scsi, sd, mtd, floppy, pflash, virtio.

With nothing about the default.

Honestly I have no possibility to test another architecture. When I run a VM only with file=image.img on linux_x64, qemu implicitly attaches a virtual ide disk. On the other side giving the option to explicitly configure the interface, any non default behaviour can be overriden within the nomad job file. I guess if another architecture does not understand the given ide interface updating the documentation should be sufficient.

@bogue1979
Copy link
Contributor Author

@tgross
any news on this?

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Sorry about the delayed follow-up @bogue1979. This looks pretty good.

You haven't made the PR editable by maintainers, so I'll also need you to add a changelog entry at .changelog/11864.txt with the following contents:

```release-note:improvement
qemu: Added option to configure `drive_interface`
```

Once that's done we can get this merged and shipped in the upcoming Nomad 1.3.2

drivers/qemu/driver.go Outdated Show resolved Hide resolved
@tgross tgross added this to the 1.3.2 milestone Jun 9, 2022
Co-authored-by: Tim Gross <tgross@hashicorp.com>
tgross
tgross previously approved these changes Jun 10, 2022
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your patience on this one @bogue1979 !

@tgross tgross dismissed their stale review June 10, 2022 13:45

merge conflicts

@tgross
Copy link
Member

tgross commented Jun 10, 2022

Oops, sorry jumped the gun there @bogue1979. GitHub is saying there are some merge conflicts here. Might need to rebase on main.

@bogue1979
Copy link
Contributor Author

Sorry I'm not really a developer and Git sometimes confuses me when it does not work as expected.

@tgross tgross merged commit 9bb9aab into hashicorp:main Jun 10, 2022
Nomad - Community Issues Triage automation moved this from In Progress to Done Jun 10, 2022
@bogue1979 bogue1979 deleted the qemu_drive_interface_option branch June 13, 2022 12:37
@lgfa29 lgfa29 added the backport/1.3.x backport to 1.3.x release line label Aug 24, 2022
@lgfa29 lgfa29 modified the milestones: 1.3.2, 1.3.4 Aug 24, 2022
lgfa29 pushed a commit that referenced this pull request Aug 24, 2022
Co-authored-by: Daniel Rossbach <daniel@family-neuner.de>
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.3.x backport to 1.3.x release line theme/driver/qemu
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants