search

dmacvicar / terraform-provider-libvirt

Terraform provider to provision infrastructure with Linux's KVM using libvirt
Apache License 2.0
1.54k stars 457 forks source link

Fix domain running status fails when shutdown expected (#622) #1029

Open luc-phan opened 9 months ago

luc-phan commented 9 months ago

This PR fixes the issue of the guest not being shutdown when updating running to false (#622).

$ vim main.tf
terraform {
  required_providers {
    libvirt = {
      source = "dmacvicar/libvirt"
      version = ">= 0.7.1"
    }
  }
}

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

resource "libvirt_domain" "myvm" {
  name   = "myvm"
  vcpu   = 1
  running = true
}
$ terraform init
$ terraform apply
$ virsh list --all
 Id    Name   State
-----------------------
 140   myvm   running
$ vim main.tf
terraform {
  required_providers {
    libvirt = {
      source = "dmacvicar/libvirt"
      version = ">= 0.7.1"
    }
  }
}

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

resource "libvirt_domain" "myvm" {
  name   = "myvm"
  vcpu   = 1
  running = false  # I changed this!
}
$ terraform apply
$ virsh list --all
 Id    Name   State
-----------------------
 140   myvm   running

The expected state is shut off.

tiaden commented 9 months ago

@luc-phan, Thank you for the PR. I was about to create a PR for this issue and I noticed that you already did. I hope we can collaborate on this together. I looked at your code. It's well written. However if the domain is created with the running at false, then switched to true during update, it will start the domain and immediately issue a shutdown. What I did in my case was to leverage the existing code and implement the same functionality in just 4 lines of code and avoid the issue explained above. 

diff --git a/libvirt/resource_libvirt_domain.go b/libvirt/resource_libvirt_domain.go
index 5565d8542..c0d86aeeb 100644
--- a/libvirt/resource_libvirt_domain.go
+++ b/libvirt/resource_libvirt_domain.go
@@ -675,7 +675,7 @@ func resourceLibvirtDomainUpdate(ctx context.Context, d *schema.ResourceData, me
        return diag.FromErr(err)
    }

-   if !domainRunningNow {
+   if !domainRunningNow && d.Get("running").(bool) {
        err = virConn.DomainCreate(domain)
        if err != nil {
            return diag.Errorf("error creating libvirt domain: %s", err)
@@ -756,6 +756,9 @@ func resourceLibvirtDomainUpdate(ctx context.Context, d *schema.ResourceData, me
        }
    }

+   if err := destroyDomainByUserRequest(virConn, d, domain); err != nil {
+       return diag.FromErr(err)
+   }
    return nil
 }

Let me know what you think. This is a link to the commit I made https://github.com/tiaden/terraform-provider-libvirt/commit/06341a437e95288597e034db732aaaf6ab8428d6

luc-phan commented 9 months ago

However if the domain is created with the running at false, then switched to true during update, it will start the domain and immediately issue a shutdown.

No. It will not issue a shutdown, but there is some redundant code... The VM is started...

I think it's what you mean. Thank you, I didn't noticed that. I'm going to fix it.

Let me know what you think.

Indeed, your code is better, it leverages the existing code, and the VM is not started twice.

I'm going to fix my PR anyway, even though it's rejected, because I'm still learning, and it's a good exercise for me :)

luc-phan commented 8 months ago

I added 2 arguments :