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 NSX-T VDC Group support for vcd_network_routed_v2, vcd_network_isolated_v2 and vcd_nsxt_network_imported #801

Merged
merged 20 commits into from
Mar 24, 2022

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Mar 7, 2022

This PR adds VDC Group support for:

  • vcd_network_routed_v2 (deprecates vdc field and instead inherits parent VDC / VDC Group from NSX-T Edge Gateway)
  • vcd_network_isolated_v2 (deprecates vdc field in favor of owner_id which can be VDC and VDC Group)
  • vcd_nsxt_network_imported (deprecates vdc field in favor of owner_id which can be VDC and VDC Group)

There is a major difference how these resources support it. Routed networks follow the location of parent Edge Gateway ID - move together to VDC Group and back.

It is worth reading a new Guides doc page at first (https://github.com/vmware/terraform-provider-vcd/blob/b2beb8152cee15627e3220003498244fd5379223/website/docs/guides/vdc_groups.html.markdown) which outlines details done so far.

Locking mechanism has changed:

  • Edge Gateway locks are now put directly on URN of Edge Gateway instead of previous org:vdc:name format.
  • lockIfOwnerIsVdcGroup(d) and unLockIfOwnerIsVdcGroup are introduced to only lock on ownerRef if it is VDC Group (this is required for imported and isolated networks)
  • Locking has become quite tricky for vcd_network_routed_v2 because if it is a member of simple VDC - lock must be acquired on Edge Gateway, but if it is a member of VDC Group - a lock must be acquired on VDC Group.

Note. The goal of this PR is to preserve backwards compatibility and not break anything for existing users. vdc fields are deprecated in favor of owner_id (for isolated network) and edge_gateway_id (for routed network), but they still work.

Additionally:

  • Remove 10.1 from supported list of versions

Ran acceptance, org user acceptance, binary and upgrade tests with tags network nsxt vdcGroup edgegateway

@Didainius Didainius changed the title self-review Add NSX-T VDC Group support for vcd_network_routed_v2 Mar 7, 2022
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
@Didainius Didainius changed the title Add NSX-T VDC Group support for vcd_network_routed_v2 Add NSX-T VDC Group support for vcd_network_routed_v2 and vcd_network_isolated_v2 Mar 9, 2022
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
@Didainius Didainius force-pushed the pr-vdc-grp-org-network branch 2 times, most recently from 28eac9a to c80ad82 Compare March 10, 2022 13:01
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
@Didainius Didainius force-pushed the pr-vdc-grp-org-network branch 9 times, most recently from 27c37c1 to b2beb81 Compare March 16, 2022 20:29
@Didainius Didainius changed the title Add NSX-T VDC Group support for vcd_network_routed_v2 and vcd_network_isolated_v2 Add NSX-T VDC Group support for vcd_network_routed_v2, vcd_network_isolated_v2 and vcd_nsxt_network_imported Mar 16, 2022
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

Looks very prommising

vcd/vdc_group_common.go Outdated Show resolved Hide resolved
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

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

Thank you for all the tuning!

Copy link
Collaborator

@adambarreiro adambarreiro left a comment

Choose a reason for hiding this comment

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

Looks great to me! I'm approving the PR.

Note: Probably I'll do another go in the review as I may have missed something in the first one, and play around with manual tests

Copy link
Contributor

@vbauzys vbauzys left a comment

Choose a reason for hiding this comment

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

LGTM. Let's try to have smaller PR's in the future :)

@Didainius
Copy link
Collaborator Author

LGTM. Let's try to have smaller PR's in the future :)

No doubt about that. I dont like it myself.

Copy link
Contributor

@mikeletux mikeletux left a comment

Choose a reason for hiding this comment

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

LA(Awesome)TM!
My manual testing was especially focused on backwards compatibility, moving from vdc to owner_id and everything worked as expected.
Also moved back and forth from single VDC to VDC groups and also everything worked as a charm!
It's true that in the future smaller PRs would be great 😄

Thanks for this awesome implementation!

Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
Signed-off-by: Dainius Serplis <dserplis@vmware.com>
@Didainius Didainius merged commit c6507f6 into vmware:main Mar 24, 2022
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.

None yet

6 participants