-
Notifications
You must be signed in to change notification settings - Fork 75
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 affinity groups field to VM object and bump go to v1.15 #258
Conversation
f673dbc
to
eb0cf77
Compare
@Gal-Zaidman do you need this to be in OCP 4.8? |
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.
Looks good at first glance, however I would prefer if we had a test that ran against an empty oVirt cluster. That way it would be easier to verify.
Yep this is needed for my affinity PR it will simplify the complex logic in terraform, plus now there is an annoying situation that the affinity groups for the control plain machines are "best effort" :/ I tested it locally against my ovirt instance |
To test this PR I created a VM with terraform and affinity groups: resource "ovirt_vm" "my_vm_1" {
name = "my_vm_1"
cluster_id = "${var.cluster_id}"
memory = 1024 # in MiB - don't mix with instance_type_id
initialization {
authorized_ssh_key = "${file(pathexpand("~/.ssh/id_rsa.pub"))}"
}
template_id = "${var.template_id}"
affinity_groups = ["test1", "test2"]
} With the following tfvars file: cluster_id = "07716001-e669-4ded-b2e7-099543a0446e"
ovirt_username = "admin@internal"
ovirt_url = "https://xxxx/ovirt-engine/api"
ovirt_pass = "xxxxx"
storagedomain_id = "fa4bb6d3-f0bd-4880-b6ed-20393bb02932"
template_id = "8fbf982b-b1cd-4743-83bc-18f3d7bd9dad" Ran it against a local ovirt instance and verified that the VM was added to the existing affinity groups. Also tried:
No affinity group --> VM created with no problems |
ovirt/resource_ovirt_vm.go
Outdated
} | ||
for _, agName := range agNames { | ||
if _, ok := agNamesMap[agName.(string)]; !ok { | ||
return nil, fmt.Errorf("affinity group %v was not found on cluster %v", agName, cID) |
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.
Please build the error based on the entire loop processing rather than exiting on the first error
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.
Fixed this is an example of the error
Error: affinity groups [test5 test6 test9] was not found on cluster 07716001-e669-4ded-b2e7-099543a0446e
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.
lgtm
eb0cf77
to
b7ec9cb
Compare
f2d9edb
to
cfcd2b0
Compare
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.
A few minor nit-picks apart from the Go version.
cfcd2b0
to
1aaebe3
Compare
Looks good from the code, haven't tested it as I'm on PTO. |
This change adds the ability to specify affinity groups for a VM upon creation.
It is required because once a VM is created and started then if it is added to an affinity group it is just "best effort".
If it is added to the affinity group before it is started then the affinity group rules will apply.
I'll open a separate PR for the update and read flow because I suspect it will take more time to implement