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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify volume attachment #81

Closed
wants to merge 7 commits into from

Conversation

moio
Copy link
Collaborator

@moio moio commented Oct 27, 2016

This changes the syntax to attach a volume to a domain from:

resource "libvirt_domain" "domain" {
  ...
  disk = {
    volume_id = "${libvirt_volume.volume1.id}"
  }
  disk = {
    volume_id = "${libvirt_volume.volume2.id}"
  }
}

to:

resource "libvirt_domain" "domain" {
  ...
  volume_ids = ["${libvirt_volume.volume1.id}, ${libvirt_volume.volume2.id}"]
}

Besides simplification, this has the advantage of allowing to use interpolation to create variable-volume resources. Example:

resource "libvirt_volume" "base_volume" {
  name = "base"
  base_volume_id = "${libvirt_volume.opensuse_leap.id}"
}

variable "data_volume_ids" {
  description = "array of ids of volumes additional to the base one"
  default = []
}

resource "libvirt_domain" "domain" {
  ...
  volume_ids = ["${concat(list(libvirt_volume.base_volume.id), var.data_volume_ids)}"]
}

Original motivation for this was to DRY-up the package-mirror and minion-swarm modules in sumaform.

Please see the discussion in issue #79, I needed to get this fixed so after some days I attempted myself. I am a total newbie in Go and in this project so please bear with me 馃槈!

Also note this is on top of PR #80.

Copy link

@MaximilianMeister MaximilianMeister left a comment

Choose a reason for hiding this comment

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

Nice improvement 馃憤

Elem: &schema.Resource{
Schema: diskCommonSchema(),
Elem: &schema.Schema{
Type: schema.TypeString,

Choose a reason for hiding this comment

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

you forgot to use gofmt on the file :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, I thought I did! Thanks for pointing out, fixed.

@moio moio force-pushed the simplify-volume-attachment branch from e78f223 to 30f43be Compare October 28, 2016 11:03
@dmacvicar
Copy link
Owner

The original idea of using volume_id inside disk is because not all disks are volumes, and not all volumes are disks.

So if you want to add a cdrom explicitly, you will need a disk { } with different parameters. Same for iSCSI disks, rbd and others. (Right now CDROMs are created/supported only implicitly by the cloud-init feature).

This PR assumes that all disks are volumes, that is the case right now, but closes the door to support other types to solve a completely unrelated problem of variable expansion.

@dmacvicar
Copy link
Owner

dmacvicar commented Oct 31, 2016

After discussing on irc for a while I am:

  • Convinced that this is not the way to go (because it mixes volumes and disks, when not all disks are volumes)
  • Not sure if Add volume_attachment resources or equivalent聽#79 proposal about volume attachment is the way to go. I don't have the knowledge of the importance of the problem being solved (DRY and parametrization) with the amount of questions this raises.

In any case, while volume_attachment is also wrong in the sense that you don't attach volumes to domains, but disks to domains, and disks can be volumes:

  • Allowing disks resources outside of domains, with same feature set as the inline disk (eg, to reference a volume).
  • Adding a disk <-> domain attachment resource

May satisfy the conceptual libvirt model and solve @moio 's problem ?

So you would create a volume resource, a disk resource referencing it, and attachment resource.
Alternatively it could be:

disk resource
volume resource (disk references it)
and then allowing in domain disk_ids as an alternative to a list of disk blocks.
The list of disk_ids is equivalent to "attachments" to the domain.

@moio ?

@moio
Copy link
Collaborator Author

moio commented Oct 31, 2016

@dmacvicar understand and agree. Closing this PR now, adding your comment to #79 as to the preferred ways to solve this issue.

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

Successfully merging this pull request may close these issues.

None yet

3 participants