vatesfr / terraform-provider-xenorchestra

Xen Orchestra provider for Terraform
MIT License
149 stars 33 forks source link

Importing xenorchestra_network resources does not set vif_id properly #257

Closed hkraal closed 9 months ago

hkraal commented 10 months ago

I'm trying to import an existing network into my state without creating changes when running terraform. This fails because pif_id is not set in the tfstate causing the recreation of the resource.

Steps to reproduce; 1) Grab the example from docs/resources/network.md 2) Add the network to the pool manually using Xen Orchestra and copy it's uuid from the pools networking list. 3) Import the network using terraform import xenorchestra_network.network <uuid> (if this fails make sure you have copied the networks uuid on the pool level).

The import should be successful but when running terraform plan you will see;

Terraform will perform the following actions:

  # xenorchestra_network.net must be replaced
-/+ resource "xenorchestra_network" "net" {
      ~ id                = "2b75f341-a6cc-d7e3-7347-efcfe19d38e2" -> (known after apply)
      + pif_id            = "e527a7ff-88fe-62ce-a634-9684ff2f7fa2" # forces replacement
        # (8 unchanged attributes hidden)
    }

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

It seems that this is caused due the pif_id not imported into the tfstate, running with debugging enabled I'm seeing;

2023-08-23T08:36:25.926+0200 [INFO]  provider.terraform-provider-xenorchestra: 2023/08/23 08:36:25 [DEBUG] Found the following objects for type 'client.PIF' from xo.getAllObjects: [{Device:bond2 Host:1d0ff7ea-c01b-4130-bccd-d6a1ac40701d Network:a06069a7-628d-f6f1-bfdd-4e456f01390b Id:c4f6db30-e311-0775-28ee-a81de690d88d Uuid:c4f6db30-e311-0775-28ee-a81de690d88d PoolId:ec6d36d8-8012-6358-8ac2-80fa0cdd202d Attached:true Vlan:3345}]: timestamp=2023-08-23T08:36:25.926+0200
2023-08-23T08:36:25.926+0200 [WARN]  Provider "registry.terraform.io/terra-farm/xenorchestra" produced an unexpected new value for xenorchestra_network.net during refresh.
      - .automatic: was null, but now cty.False
      - .name_description: was null, but now cty.StringVal("")
      - .name_label: was null, but now cty.StringVal("exo-k8s-3345")
      - .pool_id: was null, but now cty.StringVal("ec6d36d8-8012-6358-8ac2-80fa0cdd202d")
      - .vlan: was null, but now cty.NumberIntVal(3345)
      - .default_is_locked: was null, but now cty.False
      - .mtu: was null, but now cty.NumberIntVal(1500)
      - .nbd: was null, but now cty.False
2023-08-23T08:36:25.927+0200 [DEBUG] provider.stdio: received EOF, stopping recv loop: err="rpc error: code = Unavailable desc = error reading from server: EOF"

What catches my eye is that the (uu)id of the found PIF (c4f6db30-e311-0775-28ee-a81de690d88d) is the correct uuid but it's on the host level instead of the pool.

ddelnano commented 10 months ago

Hi @hkraal, thanks for reporting this and trying out the new xenorchestra_network resource.

I decided to omit pif_id from the tfstate because I believe a network's list of PIFs can change as hosts are added and removed to the pool. The XO api requires pif_id to create a network, but once it's created PIFs can come and go as the pool changes.

For example, here is a network with 3 PIFs:

$ xo-cli xo.getAllObjects filter='json:{"id": "a12df741-f34f-7d05-f120-462f0ab39a48"}'
{
  'a12df741-f34f-7d05-f120-462f0ab39a48': {
    type: 'network',
    uuid: 'a12df741-f34f-7d05-f120-462f0ab39a48'
    '$pool': '355ee47d-ff4c-4924-3db2-fd86ae629676',
    MTU: 1500,
    PIFs: [
      '7c89e2f2-ec3c-2af9-31e2-d329fc133c50',
      '7fbe3e96-8bd0-a138-0588-8891f4372fa1',
      '8bea7345-773b-6289-0f5f-8b5bd61b2ca1'
    ],

  [ ... ]
  }
}

What catches my eye is that the (uu)id of the found PIF (c4f6db30-e311-0775-28ee-a81de690d88d) is the correct uuid but it's on the host level instead of the pool.

Can you please clarify what you mean by this? Based on what I explained above, I don't think there is a separate host or pool "PIF". I believe the network will reference the PIFs defined on the host level and will change as the pool changes.

I also see another issue with importing this resource. The vlan argument has a dependency on pif_id since that is a required parameter on VLAN network creation (source). However once a network exists, we cannot determine the original PIF id specified or if it still exists (per the explanation above). This creates a situation where a network with a vlan cannot be imported (since pif_id needs to be specified in order to supply vlan).

I need to think about how to handle this more, but the ideal way to model this would be to have the pif_id only apply for creation but otherwise not be a meaningful argument. This forum thread talks about using computed properties and/or terraform's diff suppress functionality. That was along the lines I was thinking of and I need to spend some time investigating those options.

hkraal commented 10 months ago

Hi @hkraal, thanks for reporting this and trying out the new xenorchestra_network resource.

I decided to omit pif_id from the tfstate because I believe a network's list of PIFs can change as hosts are added and removed to the pool. The XO api requires pif_id to create a network, but once it's created PIFs can come and go as the pool changes.

I think this is more nuanced being the Host vs Pool level uuid differences.

For example, here is a network with 3 PIFs:

$ xo-cli xo.getAllObjects filter='json:{"id": "a12df741-f34f-7d05-f120-462f0ab39a48"}'
{
  'a12df741-f34f-7d05-f120-462f0ab39a48': {
    type: 'network',
    uuid: 'a12df741-f34f-7d05-f120-462f0ab39a48'
    '$pool': '355ee47d-ff4c-4924-3db2-fd86ae629676',
    MTU: 1500,
    PIFs: [
      '7c89e2f2-ec3c-2af9-31e2-d329fc133c50',
      '7fbe3e96-8bd0-a138-0588-8891f4372fa1',
      '8bea7345-773b-6289-0f5f-8b5bd61b2ca1'
    ],

  [ ... ]
  }
}

What catches my eye is that the (uu)id of the found PIF (c4f6db30-e311-0775-28ee-a81de690d88d) is the correct uuid but it's on the host level instead of the pool.

Can you please clarify what you mean by this? Based on what I explained above, I don't think there is a separate host or pool "PIF". I believe the network will reference the PIFs defined on the host level and will change as the pool changes.

Maybe my wording is off due lack of deeper knowledge but bare with me. As I understand the "PIFs" are the actual networking interfaces which are created on the hosts. Thus, there should be as many PIFs as host in the pool on which it was defined.

When importing a xenorchestra_network resource none of those PIFs will be a valid UUID. There is a 4th (in your case) uuid which you can find in the network tab of a pool. Using the uuid in your example this should be at https:///#/pools/355ee47d-ff4c-4924-3db2-fd86ae629676/network for you. The uuid of the "new network name - vlan 22" interface here does work to import the resource.

I'm not able to test it but afaik this uuid stays the same during the existence of the Pool and is not impacted by adding or removing hosts at all.

I also see another issue with importing this resource. The vlan argument has a dependency on pif_id since that is a required parameter on VLAN network creation (source). However once a network exists, we cannot determine the original PIF id specified or if it still exists (per the explanation above). This creates a situation where a network with a vlan cannot be imported (since pif_id needs to be specified in order to supply vlan).

XO is be able to detect the relation, copy the UUID of the pool level network interface (this might be a12df741-f34f-7d05-f120-462f0ab39a48 for you), go to one of the Hosts in your pool and search of the UUID in the network tab of your Host. The PIF which is shown doesn't have the same UUID as you were looking for but is certainly related to the one on the Pool level.

I need to think about how to handle this more, but the ideal way to model this would be to have the pif_id only apply for creation but otherwise not be a meaningful argument. This forum thread talks about using computed properties and/or terraform's diff suppress functionality. That was along the lines I was thinking of and I need to spend some time investigating those options.

I know too little of TF to give any meaningful feedback on this but I hope the provided info might help. If anything is unclear please let me know and I'll try to do better ;)

ddelnano commented 9 months ago

When importing a xenorchestra_network resource none of those PIFs will be a valid UUID. There is a 4th (in your case) uuid which you can find in the network tab of a pool. Using the uuid in your example this should be at https:///#/pools/355ee47d-ff4c-4924-3db2-fd86ae629676/network for you. The uuid of the "new network name - vlan 22" interface here does work to import the resource.

@hkraal that "4th" uuid is the network id and not a PIF. I'm fairly confident that all PIFs are defined at the host level. The Xen Orchestra UI joins the network and PIF data to display it in a more cohesively.

I believe #260 will address this issue and will fit better within terraform's mental model. It didn't require the hacks that I mentioned from the forum above since I found a way to determine what "PIF input" is needed to recreate the resource. VLAN networks have the same device name as the original network it was created from.

Would appreciate if you have any feedback on this change to the resource (in terms of usability or the "source pif" concept).

ddelnano commented 9 months ago

This is fixed in #260 and will be released in v0.25.0 of the provider.