New resource: Distributed Virtual Switch #135
Conversation
vancluever
added
enhancement
|
Hey @jorgenunez! Thanks for the PR! It's funny that you submitted this as I was about to start work on this resource myself, now that #138 and #139 are in, as an effort to fix our lack of support of essential networking management capabilities in the provider. What I want to do, however, is make sure that we make a very reasonable effort to start modeling resources after the vSphere API management and data objects that they are working with - you can see that in the host virtual switch and port group resources that I submitted, which includes most options that are necessary for a vSwitch and Port Group (save some deprecated options), and the ability to perform inheritance like you can in the vSphere Client. What I think I'd like to see is a more of a review done on the DVSCreateSpec and DVSConfigSpec data objects to see if there's anything we should be adding to the resource. Seeing as it looks fairly complex, I would not necessarily expect everything in there, but the UX should be as reasonably close to what someone would be able to expect with using the vSphere Client. Also, in addition to this, I was also planning on writing a port group resource for this as well - was this something that you were possibly looking at writing as well? Having the port group resource to match the DVS resource will be pretty important to making sure this feature is complete. If you can let me know if this is work you'd be willing to undertake, that would be great - as it would help me with planning out a roadmap for the next few months, and knowing what we need to write versus what is coming in to the community. If not, no worries we can work on it here. Thanks! |
|
Hey @vancluever, thanks for the feedback and awesome work on #138 and #139! My train of thought for this was to put the bare basics for the DVS, then push for distributed port groups with VLAN support and then touch off on Cluster and Datastores so I could have coverage of everything I would need to launch and maintain a whole environment in vSphere with Terraform. I have working code for the distributed port groups (but not with all the options) and I started looking into the Datastore setup (a lot of things in there). Nonetheless I'm willing to put the effort to, as you said, match a little bit better what's offered on vSphere and allow most config options. We can chat about it here or I would drop by the Terraform Gitter later on around 11pm CET to coordinate a little bit better. Cheers! |
|
Hey @jorgenunez, I think coordinating here is good for now - I had thoughts below: Indeed, I would rather ensure that resources get added in the way that they are ultimately going to work longer term, especially ones more critical to infrastructure such as networking, storage, and compute. If it takes a bit of extra time, that's okay, as we need to consider the fact that significant changes to schema or workflow after the fact are breaking changes that have larger implications to state and can be possibly risky to existing environments. It sounds like you have a good grip on the DVS and DV port group resources, so if you are okay with taking those on, I would love to see your contributions! Some things I will be looking for in the review, and lessons that I have learned from adding the host networking components:
If you can work on the DV components that would be great - I will work on the datastore stuff in the meantime. Will keep an eye out for updates! |
jorgenunez
changed the title
|
Still WIP, but just as a comment this won't pass until #138 is merged since it's using the validation package introduced there as well as the structure helpers. |
jorgenunez
force-pushed
the
new-resource-dvs
branch
3 times, most recently
from
c6b99ea
to
8f7ddc3
Compare
|
I will work on this over the weekend, haven't go the time this week so far to work on it. I think one of the main things remaining has to do with updates (besides general cleanup and more tests). In this case the updates are not as straight forward as taking the config provided, populating the config spec and reconfiguring. For uplink we need to check the current set, compare with what we are specifying on the terraform config and then either add, mark for removal (but still put on the config spec) or change their config. |
jorgenunez
force-pushed
the
new-resource-dvs
branch
from
46e3d85
to
b524de5
Compare
|
Hey @jorgenunez! We were hoping to be able to get this into the provider soon - do you have an ETA as to when you might be able to have this done by so that we can start review again? I understand that this kind of work can take time, especially if you have limited time to work on it, however I was hoping to have this as a supported feature in the provider within the next few weeks. If you are finding that might not be enough time, let me know - we can continue the work on the features and retain your commits. Thanks! |
|
Hey @vancluever, so sorry it's been taking so long. I'm trying to finish it in the next couple of days with at least most of the fields I have on the structure right now. |
|
Hey @vancluever, I'm missing the implementation for the flatten for the uplinks but I think the rest is close to ready, I imagine there will be several thing to tackle so if you can start reviewing the code I will finish with the flatten and update the docs. I have commented out a few fields that I initially intended to implement here but I think are not primary and they will take me too long time to finish. If agreed I will remove them. As for the tests, the one named Finally, I have put some effort on this and struggled a little bit getting some things from the SDK but don't hesitate to use what you can and close this PR if the code is not good enough :) |
|
Hey @jorgenunez, thank you very much for the effort that you have put into this for the last couple of days! I will give this a review probably tomorrow or later today (as I'm current working on #140). As far as your work goes - we highly value contributions here at HC, and we try very hard to ensure that even in the face of required refactors and what not that work gets recognized. So I want to assure you that I would not just close this - if any refactoring work is necessary to complete this, we will just continue the work on top of your existing commits and you will still get credit in the history. Thank you very much for your work on this! And you will hear from me soon 👍 |
| // Remove | ||
| // XXX I'm not sure if it's necessary to mention the specific NICs when removing a host completely | ||
| backing := new(types.DistributedVirtualSwitchHostMemberPnicBacking) | ||
| cbp := h.Config.Backing.GetDistributedVirtualSwitchHostMemberBacking() |
There was a problem hiding this comment.
I got a box with 4 NICs to test this... doesn't work :( . I'm trying to figure it out what's wrong.
There was a problem hiding this comment.
I haven't been able so far to retrieve the PnicSpec this way, will keep on trying.
There was a problem hiding this comment.
That said, taking out the backing here (remove lines 151 to 158 and line 161) seems to work, although I still need a way to get this info for flattenDistributedVirtualSwitchHostMemberConfigSpec.
There was a problem hiding this comment.
I test it further and realised that creating and modifying uplinks (either adding or removing) works just fine but completely removing a host from a dvs doesn't work (my previous comment was wrong). It feels like I'm a inch away from getting that info through this structure but it's not going to be possible. I will check if I can get it somehow through the HostSystem reference.
jorgenunez
changed the title
|
Hey @vancluever, I'm taking out the WIP tag from this. I have managed to make it work properly and taken out what I'm not going to implement as part of this PR. The code isn't the tidiest but I have done several tests beyond the tests included here and adding/updating/removing nics and whole hosts seems to work well. I have yet to update the docs, but I will do that tomorrow. |
jorgenunez
force-pushed
the
new-resource-dvs
branch
from
9186b65
to
e800ab1
Compare
This commit is a major refactor the DVS resoruce provided in PR #135. More options have been added, the DVS has been fixed so that it uses the VMware DVS object instead of the generic DVS object, which is more or less just used for 3rd party DVS switches. This meant that the DVS was missing a number of features that more than likely would have been expected in the DVS. The breadth of the features added can be seen in the documentation. It should be noted that this does not include the ability to set private VLANs, filtering policies, or port mirroring sessions, which will be added at a later date and pending business case. This work also contains a bunch of up front work that will be necessary for the vsphere_distributed_port_group resource, which will be following not too long after this resource.
This commit is a major refactor the DVS resource provided in PR #135. More options have been added, the DVS has been fixed so that it uses the VMware DVS object instead of the generic DVS object, which is more or less just used for 3rd party DVS switches. This meant that the DVS was missing a number of features that more than likely would have been expected in the DVS. The breadth of the features added can be seen in the documentation. It should be noted that this does not include the ability to set private VLANs, filtering policies, or port mirroring sessions, which will be added at a later date and pending business case. This work also contains a bunch of up front work that will be necessary for the vsphere_distributed_port_group resource, which will be following not too long after this resource.
This commit is a major refactor of the DVS resource provided in PR #135. More options have been added, the DVS has been fixed so that it uses the VMware DVS object instead of the generic DVS object, which is more or less just used for 3rd party DVS switches. This meant that the DVS was missing a number of features that more than likely would have been expected in the DVS. The breadth of the features added can be seen in the documentation. It should be noted that this does not include the ability to set private VLANs, filtering policies, or port mirroring sessions, which will be added at a later date and pending business case. This work also contains a bunch of up front work that will be necessary for the vsphere_distributed_port_group resource, which will be following not too long after this resource.
|
Hey @jorgenunez, just FYI that I'm closing this in favour of #188. There were certain issues that needed to be resolved - the major issue being is that this PR was based on the generic DVS object, which is admittedly a confusing topic and probably something I would have run into myself if it was not for #161. You can check out the PR for some of the other details - more notably a moving away from options that would be more suitable for 3rd party DVS devices and also the addition of the ability to control uplink naming, uplink count, and default port policy which allows for failover order control and options that we will need when As mentioned before as well, I built this on top of your history, so your contributions will be recognized as well. Thanks again for your work on this! |
ghost
locked and limited conversation to collaborators
Adds a new resource
vsphere_distributed_virtual_switch. It provides a consistent network config for virtual machines regardless of the host they reside in through the use of port groups.It requires two new environment variables for testing,
VSPHERE_HOSTandVSPHERE_HOST_PNICspecifying a ESXi host name and physical network card to be used for the uplink port group of the switch.Tested on vSphere 6.5.