-
Notifications
You must be signed in to change notification settings - Fork 458
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add storage pool resource #571
Conversation
Actually, after implementing
So I'm wondering if we want |
if |
So I added the docs and I fixed the testcase for unique names. From my side the only thing left is the failing What is a big mystery to me still is that So I'd really like a second pair of eyes to see what I'm missing here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Thanks @MalloZup and @dmacvicar for reviewing.
I understood that we can later add support for other types of pools by adding more attributes (e.g
If you are talking about
As long as new attributes/types can be added later, that's not an issue. Otherwise, I can add attributes already for
Hm.. do you have any idea why manual deletion doesn't break destroy operation for volume but only for pool? |
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 the 1st layer would be :
"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
the 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 In this way, we can create for this 1st PR only the |
@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. |
@MalloZup I tried with
as schema and test case:
but i get error: |
afaik you don't need the take this examples:
and schema
|
Yeah i tried without as well. Still the same.
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. |
@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 |
@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:
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. |
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) |
Cool, I'll go for that one then.
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. |
09cb778
to
e3fdd8d
Compare
Solution 1 implemented. PTALA |
thx @zeenix i have also opened an issue upstream ( hashicorp/terraform#20927). So recapitulating, To me , i agree it is a trade-off but could be ok. Let's wait for @dmacvicar opinion on the |
https://github.com/hashicorp/terraform/issues/6215 :( I think the only way for us is to have a |
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? |
@zeenix for the test i actually didn't have the time for diving it. about the |
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nitpick thx @zeenix to me seem pretty well done this pr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Thanks for reviews, tests and very encouraging words in general @MalloZup. :) |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@dmacvicar Thanks for the review. I'll make the changes. :) |
@dmacvicar PTALA.
Strange. Test succeed here:
|
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 |
so I didn't have any problem with go-vet. However golint is failing and that is why also travis is faling.
|
eba317f
to
fdd03f8
Compare
This patch adds an experimental resource type for libvirt storage pool. Currently only directory-based pool are supported. Fixes dmacvicar#435.
@MalloZup Getting closer to end of the week btw. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ❤️
Thanks @MalloZup and @dmacvicar for your reviews and patience with me. :) I'm looking into #579 now. Fingers crossed if i succeed. :) |
@zeenix thank you for this. 👍 welcome |
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:
update
implementation.