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

Add placement policy to vcd_vapp_vm and vcd_vm resources and data sources #922

Merged
merged 34 commits into from
Nov 16, 2022

Conversation

adambarreiro
Copy link
Collaborator

@adambarreiro adambarreiro commented Oct 25, 2022

Description

This PR:

The proposed changes onboard the new attribute placement_policy_id to vcd_vapp_vm and vcd_vm resource and data source. This attribute complements sizing_policy_id: It assigns the referenced VM Placement Policy to the specified VM.

This VM Placement Policy can be assigned, updated or removed, although it can be removed only if the VM has already a VM Sizing Policy, as the VM must have either a VM Sizing Policy or a VM Placement Policy (if the VDC has any assigned, of course). If one tries to remove both, operation should fail.

Both VM Sizing Policy and VM Placement Policy need to be updated in the same API call, as both are "VDC Compute Policies". Otherwise, one or another would get unassigned from the VM.

Use cases

  • A VM picks the Compute Policy automatically from the VDC default on creation:
resource "vcd_vm" "foo" {
  ...
}

The state should populate either sizing_policy_id or placement_policy_id with the Compute Policy ID.

  • A VM may have only a VM Sizing Policy. In this case, it can't be unassigned (in UI you usually can choose between either System Default or another one)
resource "vcd_vm" "foo" {
  ...
  sizing_policy_id = vcd_vm_sizing_policy.sizing.id
}
  • A VM may have only a VM Placement Policy. In this case, if we had already created the VM with a Sizing Policy, we need to explicitly set an empty value in the Sizing Policy to unassign it, as it's an Optional+Computed value which would get ignored otherwise.
resource "vcd_vm" "foo" {
  ...
  sizing_policy_id = "" # It is computed so the attribute needs this explicit empty value
  placement_policy_id = vcd_vm_placement_policy.placement.id
}
  • A VM may have both VM Sizing Policy and VM Placement Policy.
resource "vcd_vm" "foo" {
  ...
  sizing_policy_id = vcd_vm_sizing_policy.sizing.id
  placement_policy_id = vcd_vm_placement_policy.placement.id
}
  • A VM must have either a VM Sizing Policy or a VM Placement Policy. Having none should fail:
resource "vcd_vm" "foo" {
  ...
  sizing_policy_id = ""
  # placement_policy_id = vcd_vm_placement_policy.placement.id
}

should give this error:

│ Error: error updating compute policy: either sizing policy ID or placement policy ID is needed

Issues

In the original code, sizing_policy_id was Computed and Optional. To preserve compatibility, this didn't change, but adds some complexity/quirks to the way that the combination of sizing_policy_id + placement_policy_id work. The main problem is that sizing_policy_id does not trigger an Update if one puts the following snippets and nothing else changed in config:

  sizing_policy_id = null
  sizing_policy_id = ""
  # sizing_policy_id = "urn:vcloud:..."

In the Refresh stage, Terraform will say that the state matches the configuration, unless we put:

    sizing_policy_id = "  "  # some spaces there

In the code we explicitly trim the spaces so this should be good. I didn't find any way to force an Update when one tries to explicitly set an empty value to this attribute. Terraform simply ignores it during Refresh, as if you didn't specify anything at all.

Also this is not really a big deal as this situation (trying to remove the sizing policy when there is not a placement policy and nothing else changed in config) should always trigger an error (at least one policy is required, if the VDC has assigned policies). So unless someone wants to make the Apply fail, she should never face this scenario.

In versions <= v3.7.0 this was not an issue because we did not support VM Placement Policies, so one could not (and should not) remove the VM Sizing Policy once set. Now, with VM Placement Policies, one can remove the Sizing Policy.

Tests

  • Acceptance tests pass
  • Binary tests pass
  • Upgrade tests pass
  • Complementary manual tests:
    • Create a VM in a VDC with several policies and a default sizing policy. It should be created with just the default sizing policy in state. Placement policy is nil.
    • Create a VM in a VDC with several policies and a default placement policy. It should be created with just the default placement policy in state. Sizing policy is empty and Placement policy is not.
    • Create a VM with a defined VM Placement Policy and VM Sizing Policy. State should have both.
    • Update previous VM, removing the VM Placement Policy entirely. Placement Policy should be removed and not appear in state.
    • Update previous VM, putting sizing_policy_id = " ". It should error as it needs at least one policy.
    • Change to different VM Placement Policies / VM Sizing Policies. State should show the changes.

abarreiro added 11 commits October 25, 2022 12:28
Signed-off-by: abarreiro <[email protected]>
fmt
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
#
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro self-assigned this Nov 7, 2022
abarreiro added 3 commits November 7, 2022 12:55
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
abarreiro added 2 commits November 8, 2022 09:43
Fix
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
abarreiro added 2 commits November 8, 2022 09:47
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
abarreiro added 2 commits November 15, 2022 11:35
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
@vmwclabot
Copy link
Member

@adambarreiro, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

1 similar comment
@vmwclabot
Copy link
Member

@adambarreiro, you must sign every commit in this pull request acknowledging our Developer Certificate of Origin before your changes are merged. This can be done by adding Signed-off-by: John Doe <[email protected]> to the last line of each Git commit message. The e-mail address used to sign must match the e-mail address of the Git author. Click here to view the Developer Certificate of Origin agreement.

abarreiro added 3 commits November 15, 2022 11:57
Signed-off-by: abarreiro <[email protected]>
Copy link
Collaborator

@Didainius Didainius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good as long as the tests pass.

abarreiro added 2 commits November 16, 2022 10:44
Signed-off-by: abarreiro <[email protected]>
Signed-off-by: abarreiro <[email protected]>
@adambarreiro adambarreiro merged commit be8dd2d into vmware:main Nov 16, 2022
@adambarreiro adambarreiro deleted the add-placement-policy-to-vm branch November 16, 2022 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants