Add storage pool resource #571

zeenix · 2019-03-21T21:50:23Z

This patch adds a resource type for libvirt storage pool.

Known issue:

The tests TestAccLibvirtPool_ManuallyDestroyed and 'TestAccLibvirtPool_UniqueName' currently fail. I failed to figure out why yet.

Fixes #435.

As obvious from the title, this is still WIP. I just wanted to publish it already to get possible early feedback and maybe help with figuring out why exact the tests are not passing. Next steps:

zeenix · 2019-03-26T16:46:14Z

Actually, after implementing Update, I found out that libvirt currently does not support even renaming storage pools so Update fails with an error.

Terraform will perform the following actions:

  ~ libvirt_pool.whatever2
      name: "whatsyourname" => "mynameiskhan"


Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

libvirt_pool.whatever2: Modifying... (ID: 2835bc67-1fc0-417d-8c9b-6d6be866f089)
  name: "whatsyourname" => "mynameiskhan"

Error: Error applying plan:

1 error(s) occurred:

* libvirt_pool.whatever2: 1 error(s) occurred:

* libvirt_pool.whatever2: Error updating libvirt storage pool: virError(Code=9, Domain=18, Message='operation failed: pool 'whatsyourname' is already defined with uuid 2835bc67-1fc0-417d-8c9b-6d6be866f089')

So I'm wondering if we want Update at all? If we do, it shouldn't be a lot of work to implement it in libvirt but I'm wondering if it's worth it, given that nobody every asked for this feature from libvirt before.

MalloZup · 2019-03-26T17:02:10Z

if libvirt doesn't support update we should document it in the doc as limitation imho. thx @zeenix for working on this

zeenix · 2019-03-27T20:33:02Z

So I added the docs and I fixed the testcase for unique names. From my side the only thing left is the failing TestAccLibvirtPool_ManuallyDestroyed test. After spending a lot of hours on this, I at least figured out why it's failing. The reason is simple: This implementation is not able to cope with pool deletion behind Terrform's back, as it should and that's what the testcase is supposed to test.

What is a big mystery to me still is that terroform destroy runs successfully for volumes even if you delete the volume before that but the same is not true for the pool implementation despite the fact that I'm doing exactly the same as Volume implementation is doing (since I copied and updated that code): error out if resource being deleted can not be found.

So I'd really like a second pair of eyes to see what I'm missing here.

dmacvicar

Hi @zeenix , thanks a lot for your work on this!

I am certainly eager to have a pool resource in the provider, which can help lot of CI use-cases.

The concerns I have:

This is not a pool resource, but a dir pool resource. There is no way to create something different, and this will not be immediately obvious to the users.
This limitation is hardcoded in the design. type is hardcoded in the XML created. The dir specific setting path leaks to the schema.
The first issue we will get after merging this, is somebody asking for a different pool type

How do you propose we tackle this?

zeenix · 2019-04-01T11:02:12Z

Thanks @MalloZup and @dmacvicar for reviewing.

* This is not a _pool resource_, but a _dir pool resource_. There is no way to create something different, and this will not be immediately obvious to the users.

I understood that we can later add support for other types of pools by adding more attributes (e.g type) when there is a demand for it, without breaking anything. Is this understanding not correct?

* This limitation is hardcoded in the design. _type_ is hardcoded in the XML created. The _dir_ specific setting _path_ leaks to the schema.

If you are talking about xml attribute in resource schema, I actually didn't touch that when i copied volume resource code over as the starting point. I can have a look at what that really does/is and fix it. :) If you're not talking about that, I would need more context/pointers. :)

* The first issue we will get after merging this, is somebody asking for a different pool type

As long as new attributes/types can be added later, that's not an issue. Otherwise, I can add attributes already for type so other types of pools can later be added.

Those "hard lookups" were, IIRC added very early to workaround race issues in libvirt.

Hm.. do you have any idea why manual deletion doesn't break destroy operation for volume but only for pool?

MalloZup · 2019-04-01T15:48:56Z

@zeenix @dmacvicar :

Imho what we need here is to create a generic interface with 2 layers abstraction, so we can enable plug-gable pools.

As i understand also libvirt upstream, some attributes varies depending on the type of the pool, so our schema need to make it visible the type.

the 1st layer would be :

mandatory

	"name": {
				Type:     schema.TypeString,
				Required: true,
				ForceNew: true,
			},
         "type": {
				Type:     schema.TypeMap,
				Required: true,
				ForceNew: true,
                             ... ( see 2nd layer)
			},

( we might have other global attributes like xml etc)

name is the name of pool.

the type would be the type of pool which will contain other maps, which will be the implementations e.g directory, disk pools etc , which they will be other maps.

2nd layer

			"type": {
				Type:     schema.TypeMap,
				Required: true,
				ForceNew: true,
				Elem: &schema.Resource{
					Schema: map[string]*schema.Schema{
						"dir": {
							Type:     schema.TypeMap,
                                                            // implementation here  which will be the fields for the map
							},
						"disk": {
							Type:     schema.TypeMap,
                                                           // implementation here
						},
						"scsi": {...

https://www.terraform.io/docs/extend/schemas/schema-types.html#typemap

So at the end, an end-user would specify name, type of pools, and then consequentely the specificity attributes of the pool type.

In this way, we can create for this 1st PR only the dir (directory type). and sub sequentially we could extend it if other user want to .

zeenix · 2019-04-01T21:52:27Z

Imho what we need here is to create a generic interface with 2 layers abstraction, so we can enable plug-gable pools.

@MalloZup Thanks. Sounds good to me but I could easily be missing something important here. :) So if this sounds good to @dmacvicar as well, I can change my PR accordingly.

zeenix · 2019-04-02T15:36:26Z

@MalloZup I tried with

            "type": {     
                Type:     schema.TypeMap,
                Required: true,
                ForceNew: true,
                Elem: &schema.Resource{
                    map[string]*schema.Schema{
                        "dir": {
                            Type: schema.TypeMap,
                            Required: true,
                            Elem: &schema.Resource{
                                Schema: map[string]*schema.Schema{
                                    "path": { 
                                        Type: schema.TypeString,
                                        Required: true,
                                        ForceNew: true,
                                    },
                                },
                            },
                        },
                    },
                },
            },

as schema and test case:

                resource "libvirt_pool" "%s" {
                    name = "%s"
                    type = {                                                                                                                                                                                                                                                              
                        dir = {
                            path = "%s"
                        }
                    }
                }

but i get error: type (dir): '' expected type 'string', got unconvertible type '[]map[string]interface {}'. Which part is wrong? The spec or the testcase?

MalloZup · 2019-04-02T15:47:30Z

afaik you don't need the =

take this examples:

terraform-provider-libvirt/libvirt/resource_libvirt_domain_test.go

Line 684 in 6536f78

cpu {

and schema

terraform-provider-libvirt/libvirt/resource_libvirt_domain.go

Line 293 in 6536f78

"cpu": {

zeenix · 2019-04-02T15:51:59Z

afaik you don't need the =

Yeah i tried without as well. Still the same.

take this examples:

I did, since it's the only example of Map in the resource implementations I could find. However, that's more basic since it's a map rather than map of a map.

MalloZup · 2019-04-03T08:01:55Z

@zeenix Take inspiration from this https://github.com/terraform-providers/terraform-provider-nutanix/blob/03625014f48382e934bda2e4648ff097262e6161/future/volume_group/data_source_nutanix_volume_groups.go.future#L131.

We could use a TypeList/.TypeSet of TypeMaps ( which should be not mandatory or should exclude themselfs).

It might be a terraform limitation/design to don't allow nested maps in this way

zeenix · 2019-04-03T12:01:20Z

@MalloZup I actually thought of List+Map but didn't want to go for that because then we can't mandate one of the types in the schema, i-e people could provide multiple types or even worse, no types under 'types' and it'll be valid spec as per the schema.

Other solutions that come to mind:

Separate resource types for each of the pool type. This would be easiest as I just need to rename basically but there will be a bit of code duplication.
Separate backend-specific resources corresponding to pool's target node (libvirt_pool_dir_target in this case) and a target field under pool resource to connect them both. This will be a bit of a work, also more complicated for users.

Unless we're ok with the problem with using of List+Map I mentioned above, I'd say we should just bite the bullet and go for idea 1.

MalloZup · 2019-04-03T12:25:14Z

I think that 1 is valid option allthough we should more investigate the maps of maps approach, e.G find out if it is a terraform bug limitation and at least open an issue if it that the case (I did not researched lot on that)

zeenix · 2019-04-03T14:52:53Z

I think that 1 is valid option

Cool, I'll go for that one then.

allthough we should more investigate the maps of maps approach, e. G find out if it is a terraform bug limitation and at least open an issue if it that the case (I did not researched lot on that)

Actually I did quite a bit of searching yesterday and the only thing that I found that came close was this (and similar articles/posts) but it's about nested maps in variables and not about how custom resource providers can do this.

zeenix · 2019-04-03T16:02:32Z

Solution 1 implemented. PTALA

MalloZup · 2019-04-04T08:54:21Z

thx @zeenix i have also opened an issue upstream ( hashicorp/terraform#20927).

So recapitulating, ManuallyDestroyed still fail. Besides the schema pb.

To me , i agree it is a trade-off but could be ok. Let's wait for @dmacvicar opinion on the schema. Meanwhile i will try to help you once i have some time on the failing tests.

MalloZup · 2019-04-05T07:10:45Z

https://github.com/hashicorp/terraform/issues/6215
hashicorp/terraform#20927 (comment)

:( I think the only way for us is to have a libvirt_poo_dir and libvirt_pool_type as we already implemented.

zeenix · 2019-04-05T10:14:42Z

:( I think the only way for us is to have a libvirt_poo_dir and libvirt_pool_type as we already implemented.

Oh well, I think it's not too bad actually. When we add another pool resource, much of the code can be reused and moved to a separate file but I don't see the point of doing that right now.

So that brings us to the failing testcase. So nobody has any clue why the manual pool deletion works for volume and not for pool?

Also the does the CI not run the added test, since it's passing?

MalloZup · 2019-04-05T10:29:47Z

@zeenix for the test i actually didn't have the time for diving it.

about the ci on PRs the problems is that we need libvirt, which in travis/circleci is really problematic to setup. We had in the past anhack in travis but it was not easy to maintain. If you have any idea/improvents in that direction feel free to experiment 😬

zeenix · 2019-04-05T10:58:21Z

@zeenix for the test i actually didn't have the time for diving it.

Oh ok. I was hoping @dmacvicar might have some insight. I'll ask some other folks around if they could take a look as well.

about the ci on PRs the problems is that we need libvirt, which in travis/circleci is really problematic to setup.

I'll add to my TODO to look into that. :) Any specific reason it was problematic? If it was using system libvirt, I can imagine how that can be problematic to sandbox. We should use session libvirt.

libvirt/pool.go

MalloZup

small nitpick thx @zeenix to me seem pretty well done this pr.

MalloZup

to me looks good this pr. thx @zeenix .

Let's wait for duncan opinion on this.

when i will have some free time i will try to play around on this patch since is pretty a major one ( in term we add a major feature), if i found something i will ping you on code as usual.

thx 👍 stay tuned

zeenix · 2019-04-11T15:25:56Z

Thanks for reviews, tests and very encouraging words in general @MalloZup. :)

zeenix · 2019-04-29T10:35:48Z

@dmacvicar I understand you're very busy but just wanted to remind about this issue since it's been waiting for more than 2 weeks now.

dmacvicar

I have been looking for analog examples, and I arrived to https://www.terraform.io/docs/providers/aws/r/db_instance.html

This is a resource that supports different engines too (MySQL, Oracle, PostgreSQL), therefore some attributes are supported by some backends, while some are general. The documentation will need to be very explicit about it (eg. the path) attribute.

Therefore I think it should be fine, or at least there is a precedent on how to handle it by a very popular provider. I suggest to name this libvirt_pool, and then have attributes specific to the type attribute being dir, iscsi, etc. We just need to be careful with choosing the attribute names, as we are flattening out a complex struct.

I will ask that the documentation marks this resource as very experimental and subject to change while we get some feedback.

Otherwise, apart of that, and trivial stuff like make test failing on me while testing the code because of trivial stuff like vet, I think this can be merged.

zeenix · 2019-05-06T16:03:28Z

@dmacvicar Thanks for the review. I'll make the changes. :)

zeenix · 2019-05-08T15:54:55Z

@dmacvicar PTALA.

Otherwise, apart of that, and trivial stuff like make test failing on me while testing the code because of trivial stuff like vet, I think this can be merged.

Strange. Test succeed here:

$ make testacc TEST_ARGS="-run TestAccLibvirtPool"
./travis/run-tests-acceptance -run TestAccLibvirtPool
+ go test -v -covermode=count -coverprofile=profile.cov -timeout=1200s -run TestAccLibvirtPool ./libvirt
=== RUN   TestAccLibvirtPool_Basic
--- PASS: TestAccLibvirtPool_Basic (13.97s)
=== RUN   TestAccLibvirtPool_ManuallyDestroyed
--- PASS: TestAccLibvirtPool_ManuallyDestroyed (10.10s)
=== RUN   TestAccLibvirtPool_UniqueName
--- PASS: TestAccLibvirtPool_UniqueName (10.04s)
=== RUN   TestAccLibvirtPool_NoDirPath
--- PASS: TestAccLibvirtPool_NoDirPath (0.01s)
PASS
coverage: 6.3% of statements
ok  	github.com/dmacvicar/terraform-provider-libvirt/libvirt	34.140s	coverage: 6.3% of statements

website/docs/r/pool.html.markdown

MalloZup · 2019-05-13T08:41:17Z

I will re-review this PR this week and I think we can merge it if nobody is against it. Thx @zeenix . I will check if I have some issues locally with go vet

MalloZup · 2019-05-14T07:31:32Z

so I didn't have any problem with go-vet. However golint is failing and that is why also travis is faling.

make lint-check 
go run golang.org/x/lint/golint -set_exit_status ./libvirt .
libvirt/pool.go:49:21: error strings should not be capitalized or end with punctuation or a newline
libvirt/pool.go:68:21: error strings should not be capitalized or end with punctuation or a newline
Found 2 lint suggestions; failing.

This patch adds an experimental resource type for libvirt storage pool. Currently only directory-based pool are supported. Fixes dmacvicar#435.

zeenix · 2019-05-16T16:18:12Z

@MalloZup Getting closer to end of the week btw. :)

all adressed

MalloZup

I think looks good. I will merge it on Friday unless @dmacvicar has some concern but all was adressed and it is a nice beginning. THx @zeenix for your contrib and patience ❤️

zeenix · 2019-05-17T15:23:47Z

Thanks @MalloZup and @dmacvicar for your reviews and patience with me. :) I'm looking into #579 now. Fingers crossed if i succeed. :)

MalloZup · 2019-05-17T15:46:27Z

@zeenix thank you for this. 👍 welcome

MalloZup requested review from MalloZup and inercia and removed request for MalloZup and inercia March 25, 2019 09:03

zeenix force-pushed the pool-resource branch from 08a0cde to 3cf9588 Compare March 27, 2019 20:23

MalloZup requested review from MalloZup, inercia and dmacvicar March 28, 2019 21:25

dmacvicar reviewed Mar 31, 2019

View reviewed changes

zeenix force-pushed the pool-resource branch 2 times, most recently from 09cb778 to e3fdd8d Compare April 3, 2019 16:01

MalloZup reviewed Apr 11, 2019

View reviewed changes

libvirt/pool.go Outdated Show resolved Hide resolved

MalloZup reviewed Apr 11, 2019

View reviewed changes

libvirt/pool.go Outdated Show resolved Hide resolved

MalloZup reviewed Apr 11, 2019

View reviewed changes

zeenix force-pushed the pool-resource branch from 43bc876 to 23fe4aa Compare April 11, 2019 14:43

MalloZup approved these changes Apr 11, 2019

View reviewed changes

zeenix force-pushed the pool-resource branch from 23fe4aa to 1b39b88 Compare April 11, 2019 15:29

inercia approved these changes Apr 12, 2019

View reviewed changes

MalloZup requested a review from dmacvicar April 12, 2019 10:02

dmacvicar previously requested changes May 5, 2019

View reviewed changes

zeenix force-pushed the pool-resource branch from 1b39b88 to 6913a88 Compare May 8, 2019 15:52

MalloZup reviewed May 13, 2019

View reviewed changes

website/docs/r/pool.html.markdown Show resolved Hide resolved

MalloZup requested a review from dmacvicar May 13, 2019 08:40

zeenix force-pushed the pool-resource branch 2 times, most recently from eba317f to fdd03f8 Compare May 14, 2019 16:17

Zeeshan Ali and others added 2 commits May 15, 2019 14:18

Add storage pool resource

9f00f3d

This patch adds an experimental resource type for libvirt storage pool. Currently only directory-based pool are supported. Fixes dmacvicar#435.

examples: Add new pool resource to Ubuntu example

476095b

zeenix force-pushed the pool-resource branch from fdd03f8 to 476095b Compare May 15, 2019 14:50

MalloZup approved these changes May 16, 2019

View reviewed changes

MalloZup merged commit c573bc1 into dmacvicar:master May 17, 2019

zeenix deleted the pool-resource branch May 17, 2019 15:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add storage pool resource #571

Add storage pool resource #571

zeenix commented Mar 21, 2019 •

edited

zeenix commented Mar 26, 2019 •

edited

MalloZup commented Mar 26, 2019

zeenix commented Mar 27, 2019

dmacvicar left a comment

zeenix commented Apr 1, 2019

MalloZup commented Apr 1, 2019 •

edited

zeenix commented Apr 1, 2019

zeenix commented Apr 2, 2019

MalloZup commented Apr 2, 2019 •

edited

zeenix commented Apr 2, 2019

MalloZup commented Apr 3, 2019

zeenix commented Apr 3, 2019

MalloZup commented Apr 3, 2019 •

edited

zeenix commented Apr 3, 2019

zeenix commented Apr 3, 2019

MalloZup commented Apr 4, 2019

MalloZup commented Apr 5, 2019

zeenix commented Apr 5, 2019 •

edited

MalloZup commented Apr 5, 2019

zeenix commented Apr 5, 2019

MalloZup left a comment

MalloZup left a comment

zeenix commented Apr 11, 2019

zeenix commented Apr 29, 2019

dmacvicar left a comment

zeenix commented May 6, 2019

zeenix commented May 8, 2019

MalloZup commented May 13, 2019

MalloZup commented May 14, 2019

zeenix commented May 16, 2019

MalloZup left a comment

zeenix commented May 17, 2019

MalloZup commented May 17, 2019

Add storage pool resource #571

Add storage pool resource #571

Conversation

zeenix commented Mar 21, 2019 • edited

Fixes #435.

zeenix commented Mar 26, 2019 • edited

MalloZup commented Mar 26, 2019

zeenix commented Mar 27, 2019

dmacvicar left a comment

Choose a reason for hiding this comment

zeenix commented Apr 1, 2019

MalloZup commented Apr 1, 2019 • edited

the 1st layer would be :

2nd layer

zeenix commented Apr 1, 2019

zeenix commented Apr 2, 2019

MalloZup commented Apr 2, 2019 • edited

zeenix commented Apr 2, 2019

MalloZup commented Apr 3, 2019

zeenix commented Apr 3, 2019

MalloZup commented Apr 3, 2019 • edited

zeenix commented Apr 3, 2019

zeenix commented Apr 3, 2019

MalloZup commented Apr 4, 2019

MalloZup commented Apr 5, 2019

zeenix commented Apr 5, 2019 • edited

MalloZup commented Apr 5, 2019

zeenix commented Apr 5, 2019

MalloZup left a comment

Choose a reason for hiding this comment

MalloZup left a comment

Choose a reason for hiding this comment

zeenix commented Apr 11, 2019

zeenix commented Apr 29, 2019

dmacvicar left a comment

Choose a reason for hiding this comment

zeenix commented May 6, 2019

zeenix commented May 8, 2019

MalloZup commented May 13, 2019

MalloZup commented May 14, 2019

zeenix commented May 16, 2019

MalloZup left a comment

Choose a reason for hiding this comment

zeenix commented May 17, 2019

MalloZup commented May 17, 2019

zeenix commented Mar 21, 2019 •

edited

zeenix commented Mar 26, 2019 •

edited

MalloZup commented Apr 1, 2019 •

edited

MalloZup commented Apr 2, 2019 •

edited

MalloZup commented Apr 3, 2019 •

edited

zeenix commented Apr 5, 2019 •

edited