Domain disk target `dev` attribute for SCSI disks should have `sd` prefix

omertuc commented 1 year ago

System Information

Linux distribution

NAME="Fedora Linux"
VERSION="35 (Workstation Edition)"
ID=fedora
VERSION_ID=35
VERSION_CODENAME=""
PLATFORM_ID="platform:f35"
PRETTY_NAME="Fedora Linux 35 (Workstation Edition)"
ANSI_COLOR="0;38;2;60;110;180"
LOGO=fedora-logo-icon
CPE_NAME="cpe:/o:fedoraproject:fedora:35"
HOME_URL="https://fedoraproject.org/"
DOCUMENTATION_URL="https://docs.fedoraproject.org/en-US/fedora/f35/system-administrators-guide/"
SUPPORT_URL="https://ask.fedoraproject.org/"
BUG_REPORT_URL="https://bugzilla.redhat.com/"
REDHAT_BUGZILLA_PRODUCT="Fedora"
REDHAT_BUGZILLA_PRODUCT_VERSION=35
REDHAT_SUPPORT_PRODUCT="Fedora"
REDHAT_SUPPORT_PRODUCT_VERSION=35
PRIVACY_POLICY_URL="https://fedoraproject.org/wiki/Legal:PrivacyPolicy"
VARIANT="Workstation Edition"
VARIANT_ID=workstation

Terraform version

$ terraform -v

Terraform v1.0.10
on linux_amd64
+ provider registry.terraform.io/dmacvicar/libvirt v0.6.14

Your version of Terraform is out of date! The latest version
is 1.2.6. You can update by downloading from https://www.terraform.io/downloads.html

Provider and libvirt versions

./.terraform/providers/registry.terraform.io/dmacvicar/libvirt/0.6.14/linux_amd64/terraform-provider-libvirt_v0.6.14 0.6.14

Compiled against library: libvirt 7.6.0
Using library: libvirt 7.6.0
Using API: QEMU 7.6.0
Running hypervisor: QEMU 6.1.0

Checklist

[ ] Is your issue/contribution related with enabling some setting/option exposed by libvirt that the plugin does not yet support, or requires changing/extending the provider terraform schema?
- [ ] Make sure you explain why this option is important to you, why it should be important to everyone. Describe your use-case with detail and provide examples where possible.
- [ ] If it is a very special case, consider using the XSLT support in the provider to tweak the definition instead of opening an issue
- [ ] Maintainers do not have expertise in every libvirt setting, so please, describe the feature and how it is used. Link to the appropriate documentation
[x] Is it a bug or something that does not work as expected? Please make sure you fill the version information below:

Description of Issue/Question

The <target> dev attribute given to SCSI host disks should have an sd prefix and not vd prefix (e.g. sda as opposed to vda), to better match the name that Linux would give them.

Setup

terraform {
  required_providers {
    libvirt = {
      source = "dmacvicar/libvirt"
      version = "0.6.14"
    }
  }
}

provider "libvirt" {
  uri = "qemu:///system"
}

resource "libvirt_volume" "test-disk" {
  name = "test-disk"
  format = "qcow2"
}

resource "libvirt_domain" "host" {
  name = "test-host"

  disk {
    scsi = true
    volume_id = "/var/lib/libvirt/images/test-disk"
  }
}

Steps to Reproduce Issue

$ terraform apply -auto-approve 
$ virsh dumpxml test-host | grep 'target dev'
<target dev='vda' bus='scsi'/>

Additional information:

None

dmacvicar commented 1 year ago

As the original code was not specifying any convention, the vdX pattern was determined by libvirt. Shouldn't this be fixed in libvirt instead? ie. to use sda pattern if no device name is specified.

omertuc commented 1 year ago

Looks like this provider is setting it explicitly to vd...

omertuc commented 1 year ago

For .iso the Dev is already being overidden: https://github.com/openshift-assisted/terraform-provider-libvirt/blob/f6f8a26f9a7b044e5648156bd632a88dfcea707a/libvirt/domain.go#L545

so my PR fix is not unprecedented

dmacvicar commented 1 year ago

Thanks. It makes sense now.

I need to find out if the right fix is to stop overriding it at all, or like you suggested, do it also for SCSI. Some evidence how libvirt and qemu layers deal with defaults would be helpful.

omertuc commented 1 year ago

When I try to omit the Dev: parameter (by modifying the provider source code, not through tf files or anything like that) libvirt is telling me:

╷
│ Error: error defining libvirt domain: missing target information for device pool = 'default', volume = 'test-disk'
│
│   with libvirt_domain.host,
│   on main.tf line 19, in resource "libvirt_domain" "host":
│   19: resource "libvirt_domain" "host" {
│

Which would lead me to believe it's probably required

dmacvicar / terraform-provider-libvirt