ovirt_vm enhancements - nics, affinity groups

[shantur]

Changes proposed in this pull request:

• Add data source ovirt_affinity_group to get details of affinity_group
• Support affinity_groups list when creating ovirt_vm Moved to seprate PR Add affinity group data source from #257 #260
• Add remove_cloned_nics property to ovirt_vm to clean up nics cloned when cloning a template
• Add interface property to define type of nic to attach to vm in ovirt_vm

Output from acceptance testing:

$ make testacc TEST=./ovirt TESTARGS='-run=TestAccOvirtDataCenter_'
TBC
...

[Gal-Zaidman]

Thanks a lot for the PR!
Regarding the affinity part, I already have a 2 PRs in progress:
#258
#259

So I think it is best if you will remove the parts in the resource from the PR :)

[shantur]

@Gal-Zaidman : Awesome. Sure I will remove the affinity group from ovirt_vm.
Do you want me to keep the affinity_group data source?

[Gal-Zaidman]

@Gal-Zaidman : Awesome. Sure I will remove the affinity group from ovirt_vm.
Do you want me to keep the affinity_group data source?

Yes sure.

[shantur]

@Gal-Zaidman : All yours

[emesika]

@shantur please rebase on master which has now PR #258 merged and verify that there are no duplicate handling

[shantur]

@emesika : Done

[shantur]

@emesika @Gal-Zaidman

Any chance to get this merged?

[ghost]

@shantur on the surface this looks good, haven't tested it yet. However, I'm having a bit of a problem merging two features in one PR in case we need to roll it back. Could you split this PR so the AG data source and the NIC feature goes in separately?

I'm sorry for the extra rounds needed on this, I'll work with you on getting both merged and released.

[shantur]

@janoszen : created #260 for affinity group
Modified this to nic features

[Gal-Zaidman]

@emesika
Can you take a look at the way we handle the nics in general?
It looks like in create, we take the nics that the template has and if the user-specified the "nics" field we add the nics after the VM is created but the template VM is not override.
Shouldn't we do that before the VM is created?
I mean:

• if a user didn't specify nic default to what the template have
• if the user did specify nics then the nics that are specified should be the nics that the VM will get and not added, so you can use something like (and I haven't tested that) vmBuilder.nics(THE_NICS_SLICE_THE_USER_PROVIDED).

Removing/Adding fields one by one can cause issues if something fails in the middle plus I think if a user specified nics for a VM he expects the nics to be the nics he specified and not added to what the template holds

What do you think?

@shantur after eli will take a look I think you will also need to add the update part to the resource

[shantur]

@Gal-Zaidman :
I believe the resource should be recreated if the clone nics property is changed.
The nics in the template could have changed after creation of the vm and hence we can't be sure if the new nics in template is what user was expecting.

[shantur]

@Gal-Zaidman @emesika

Any chance to get this in?

[ghost]

Hey @shantur sorry for the delayed answer, we are still working on organizing things over here. I'll give you feedback during the coming 1 week.

[ghost]

@shantur my apologies this took so long. I reviewed your code and I have no direct objections to the code quality, so I'm fine with having this change merged, but I'm also not an expert on oVirt internals. @Gal-Zaidman @emesika any objections?

However, I have a problem with the NIC resource (and other resources) being directly implemented on the VM resource, which we should address, possibly in a later release. My main concern is that the VM resource is extremely bloated as it is and it is very hard to test and keep sane. It also doesn't allow for flexibility of using the oVirt API to its full potential.

I believe we should split out at least the following resources:

• Affinity group membership (as discussed in remove vm_list from affinity group resource #265 )
• Power state (This will allow managing the desired power state of the VM separately.)
• NICs (this will allow waiting for a network to be created)
• Disk attachment (this would allow programmatically moving a disk from one VM to another via TF)

Now, this is an idealistic approach and we may not end up doing this, especially since we don't have unlimited resources for developing the TF provider, but code quality is a pretty big concern. I found a number of bugs while working on #267 and we currently have no working test suite to verify that patches actually don't break anything beyond testing that it doesn't break the OpenShift installer.

[ovirt-infra]

Hello contributor, thanks for submitting a PR for this project!

I am the bot who triggers "standard-CI" builds for this project.
As a security measure, I will not run automated tests on PRs that are not from white-listed contributors.

In order to allow automated tests to run, please ask one of the project maintainers to review the code and then do one of the following:

1. Type ci test please on this PR to trigger automated tests for it.
2. Type ci add to whitelist on this PR to trigger automated tests for it and also add you to the contributor white-list so that your future PRs will be tested automatically. ( keep in mind this list might be overwritten if the job XML is refreshed, for permanent whitelisting, please follow New resource: ovirt_datacenter #3 option )
3. If you are planning to contribute to more than one project, maybe it's better to ask them to add you to the project organization, so you'll be able to run tests for all the organization's projects.

[ghost]

@shantur feel free to ignore the ovirt-infra bot for the time being, I need to have a few things ironed out here before this actually starts working.

[ghost]

@shantur (sorry, last ping). It might be easier if we talked on a call. If you'd like to do that please drop me an e-mail at [email protected].

[ghost]

Dear contributors,

We are making some large-scale changes to the Terraform provider which affect this pull request. Please read the announcement on the oVirt blog.

We are very sorry for the inconvenience and would like to hear your feedback on the community call on the 14th of October.

[ghost]

Dear contributor, we are closing this pull request as a preparation for the move to the new Terraform provider announced above. If you feel that this PR is still relevant please re-file it once we have made the move to the new provider.