Pure Go terraform-provider-libvirt experiment

[dmacvicar]

This is an experiment to attempt to make the provider independent of the operating system. The motivation is to be able to distribute it via the terraform registry (#747). As an experiment, it is not guaranteed that the changes will make into the main branch.

The strategy is to move from libvirt-go to go-libvirt. The later implements the libvirt RPC protocol natively in Go.

The main challenges are:

• The go-libvirt API is different. Closer to the C API.

  The libvirt-go API attempts to be more object oriented. This means carrying the libvirt connection object into many helper functions, resulting in diffs like this:

-func domainIsRunning(domain libvirt.Domain) (bool, error) {
-	state, _, err := domain.GetState()
+func domainIsRunning(virConn *libvirt.Libvirt, domain libvirt.Domain) (bool, error) {
+	state, _, err := virConn.DomainGetState(domain)
 	if err != nil {
 		return false, fmt.Errorf("Couldn't get state of domain: %s", err)
 	}
 
-	return state == libvirt.DOMAIN_RUNNING, nil
+	return libvirt.DomainState(state) == libvirt.DomainRunning, nil
 }

• There are missing features, like PolicyKit authentication (Support polkit authentication digitalocean/go-libvirt#110)
• We don't know much about the maintainability of the native client library, though Digital Ocean is supposedly using it internally

(we will document more as we learn in this branch)

To make it easier:

• No refactorings, and just have as a goal compilation and later passing most of the testsuite
• The old import was renamed from libvirt to libvirtc and opens both connections simultaneously, so we can port the code that calls one client with code that calls the native client

Most of the changes are tedious and mechanic. This is a great opportunity to read a lot of the code and get an idea of what improvements and cleanups can be done later.

If you are interested in helping this experiment, get in touch with me so that we work in different areas/files.

Ideas for post-port refactorings and clenups

Brainstorming some ideas here

[karlskewes]

Keen to help, will jump on IRC. :)

[MalloZup]

@kskewes thx so much for helping us on this.

One thing we need to be careful:

don't introduce subpackage as you did with the package volume.

It is the design of the providers to have a plain structure, with only main package.

see for example https://github.com/hashicorp/terraform-provider-google/tree/master/google

[MalloZup]

@dmacvicar I saw on the gist brainstorming an indication of introducing golang subpackages.

In my opinion we should conserve the plain structure as many other terraform providers.
In case we need some reorganization of code an the subpackages are in this direction , considered as a need, we can introduce them later on, but in order to isolate changes and structural changes, I think we should for this big refactoring keep the change as minimal as possible.

E.g what I think is that we we could risk a bad naming or even structure in this refactoring, and later we can with difficulty modify

[dmacvicar]

@dmacvicar I saw on the gist brainstorming an indication of introducing golang subpackages.

In my opinion we should conserve the plain structure as many other terraform providers.
In case we need some reorganization of code an the subpackages are in this direction , considered as a need, we can introduce them later on, but in order to isolate changes and structural changes, I think we should for this big refactoring keep the change as minimal as possible.

E.g what I think is that we we could risk a bad naming or even structure in this refactoring, and later we can with difficulty modify

Yes. As part of the port, we should not refactor.

After the port, we can discuss the potential refactorings. I can't remember the context of subpackages, but AWS provider has them (internal package and many subpackages there).

[dmacvicar]

@kskewes I think I now understand the issue with NetworkUpdate.

• The bug was corrected automatically by libvirt client, swapping what came via the API to what was sent to the daemon, which had them reversed.
• Now everything is fixed to the right order, and libvirt does introspection using a new "Feature" check to instrospect if the driver has the right order.
• What you saw as a failing test was not due to the upstream, but just that as we don't use libvirt client anymore, the order did not get reversed between client and daemon, and you had a daemon with the wrong order.
• bec3fcd put the parameters in the wrong order, if you were using a libvirt with the fixed order (like myself)
• I have committed 22df5a2 which I think handles this correctly by checking the libvirt version

[dmacvicar]

The only failing testacc here is:

=== RUN   TestAccLibvirtDomain_CheckDHCPEntries
    testing.go:669: Step 0 error: errors during apply:

        Error: Operation not supported: can't update 'bridge' section of network 'itrfzutrnl'

          on /tmp/tf-test345760586/main.tf line 9:
          (source code not available)

--- FAIL: TestAccLibvirtDomain_CheckDHCPEntries (10.51s)

Which I understand is because this operation is really not supported by libvirt, so we will fix it on master.

[dmacvicar]

The branch is now on master!

Thanks a lot @kskewes @MalloZup !!

[rgl]

This is so cool. Thanks for making things simpler!

I took it for a spin at https://github.com/rgl/terraform-libvirt-ubuntu-example/tree/wip and I found this problem:

libvirt_domain.example: Creating...
╷
│ Error: Operation not supported: can't update 'bridge' section of network 'terraform_example'
│ 
│   with libvirt_domain.example,
│   on main.tf line 104, in resource "libvirt_domain" "example":
│  104: resource "libvirt_domain" "example" {
│ 
╵

Should I open a new issue?

[dmacvicar]

This is so cool. Thanks for making things simpler!

I took it for a spin at https://github.com/rgl/terraform-libvirt-ubuntu-example/tree/wip and I found this problem:

libvirt_domain.example: Creating...
╷
│ Error: Operation not supported: can't update 'bridge' section of network 'terraform_example'
│ 
│   with libvirt_domain.example,
│   on main.tf line 104, in resource "libvirt_domain" "example":
│  104: resource "libvirt_domain" "example" {
│ 
╵

Should I open a new issue?

I believe this one is related to #784 and #788 and more in detail #788 (review)

So I need to fix this before the release. Thanks for giving it up a spin. I let you know when I publish a pre-release with the fix.

[dmacvicar]

@rgl it was more related to #813 (comment)

Fixed. Can you check if 0.6.9-pre4 works for you?

[rgl]

@dmacvicar, I would, but there's no -pre4 at https://registry.terraform.io/providers/dmacvicar/libvirt/latest only -pre3.

[dmacvicar]

@dmacvicar, I would, but there's no -pre4 at https://registry.terraform.io/providers/dmacvicar/libvirt/latest only -pre3.

@rgl sorry, I forgot I still have the automated released generating draft releases. Published now!

[rgl]

At the first try it failed (because the network was created when the previous run fail) to start with:

libvirt_domain.example: Creating...
╷
│ Error: Requested operation is not valid: network is not running
│ 
│   with libvirt_domain.example,
│   on main.tf line 105, in resource "libvirt_domain" "example":
│  105: resource "libvirt_domain" "example" {
│ 
╵

After manually starting the network, all worked! Thank you! :-)