Skip to content
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

F-72: Allow update of VM NICs and update disks for consistency #80

Merged
merged 9 commits into from
Dec 8, 2020

Conversation

treywelsh
Copy link
Collaborator

@treywelsh treywelsh commented Nov 4, 2020

This PR allow to update the NICs of a VM in a RUNNING/POWEROFF state.

But overall, the behaviour is the same that for disk update in #59
This PR remove some computed attributes around that were not useful.

I updated helpers that make diffs on list of configs (NIC, disk...)

One change that doesn't impact much the code is that a VM doesn't have transitional states when attach/detach a NIC from a VM in POWEROFF state.
To make less requests we could remove the waitForVMState call in this case.

opennebula/helpers.go Outdated Show resolved Hide resolved
opennebula/helpers_vm.go Outdated Show resolved Hide resolved
opennebula/resource_opennebula_virtual_machine.go Outdated Show resolved Hide resolved
@treywelsh treywelsh closed this Nov 17, 2020
@treywelsh treywelsh deleted the F-72 branch November 17, 2020 15:32
@treywelsh treywelsh reopened this Nov 17, 2020
opennebula/resource_opennebula_virtual_machine.go Outdated Show resolved Hide resolved
opennebula/resource_opennebula_virtual_machine.go Outdated Show resolved Hide resolved
opennebula/shared_schemas.go Outdated Show resolved Hide resolved
@treywelsh
Copy link
Collaborator Author

treywelsh commented Nov 23, 2020

Here is what's happening on this PR: new acceptance tests don't pass due to the fact that NIC schema fields are marked as computed and optional.

This is ok, but this lead to an unexpected behaviour: in case of a NIC update (tf file is modified to detach and attach a single NIC) the diff is broken.
The Get methods return new NIC datas mixed with old values previously computed (on optional fields that weren't set):

2020/11/17 18:21:05 [WARN] Test: Step plan: DIFF:
UPDATE: opennebula_virtual_machine.test
...
  memory:                  "128" => "128"
  name:                    "test-virtual_machine" => "test-virtual_machine"
  nic.#:                   "1" => "1"
  nic.0.ip:                "172.16.100.131" => "172.16.100.111"
  nic.0.mac:               "02:00:ac:10:64:83" => "02:00:ac:10:64:83"
  nic.0.model:             "" => ""
  nic.0.network:           "test-net1" => "test-net1"
  nic.0.network_id:        "0" => "1"
  nic.0.nic_id:            "0" => "0"
  nic.0.physical_device:   "" => ""
  nic.0.security_groups.#: "1" => "1"
  nic.0.security_groups.0: "0" => "0"

Here, the network_id and ip diff is ok, but for the mac (left empty in tf file), the field is not shown as empty and the old mac is kept.

There is a lot of issues around, this one has some clear explanations: hashicorp/terraform-plugin-sdk#282

Some ways I explore to try to fix this:

  1. try to handle the diff with customdiff package and RessourceDiff methods:
    I tried SetNewComputed method on diff but doesn't seems to work on nested keys like nic.0.mac...
    Related: SetNew does not work on nested fields hashicorp/terraform-plugin-sdk#459
  2. split each attribute in two attributes: one marked as optional, the other as computed
  3. introduce opennebula_virtual_machine_nic resource:
    This would be an attachment resource between a VM and a VNet.
    If the ressource require the VM ID, it's not possible to create a VM with attached NIC.
    To solve this, the VM ressource should provide basic NIC management at VM creation (already implemented).
    It's may be possible manage NIC only via the opennebula_virtual_machine_nic resource (even at VM creation) but this lead to some new others problems:
    • an arbitraty ID should be generated for the NIC at opennebula_virtual_machine_nic creation
    • NIC template content should be appended to the VM template at VM creation: this imply data sharing between resources (but I don't know any similar case of serialized data shared among other providers)

This also concern a previous PR already merged about disks #64
For disks I minimized the problem taking into account only the target attribute but it's also marked as optional and computed

@treywelsh
Copy link
Collaborator Author

treywelsh commented Nov 25, 2020

I see two ways to solve the problem based on 2. and 3. from comment above.

The 2. could be a temporary solution until hashicorp/terraform-plugin-sdk#282 is solved (the problems seems to be that they want to keep backward compatibility with old terraform versions).
Then computed attributes added as a fix could be removed after the migration of the provider to the terraform SDK.

The 3. which introduce a new resource that manage NIC attachement would lead to a new way to describe resource dependencies that solve the problem stated above, but would also partly split NIC (same would apply for disk) management from VM resource.
This would add a bit of complexity (but still much less than in other providers...), but may be a good thing and wouldn't necessarily break the behavior currently implemented in the provider.

Here is the POC branch with new resource and tests: https://github.com/treywelsh/terraform-provider-opennebula/tree/F-72-new-ressource-nic

@treywelsh
Copy link
Collaborator Author

Implement 2.
i.e. split computed+optional attributes in two parts for nic.

There is a bit more to improve overall behavior and for consistency:

  • Update generic diff helper (for list of disks or nics) to allow update based on a set of attributes:
    Initially only network_id for nic and image_id were used to trigger attach/detach actions to update the resource.
    Now, for instance, modify disk target will trigger an attach/detach to update the resource.
  • disk update code has been updated to work in the same way than NIC

@treywelsh treywelsh changed the title F-72: Allow update of VM NICs F-72: Allow update of VM NICs and update disks for consistency Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants