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

VMware: add new module vmware_guest_network #52075

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
6 participants
@Tomorrow9
Copy link
Contributor

Tomorrow9 commented Feb 12, 2019

SUMMARY

Fixes #51066

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

vmware_guest_network

ADDITIONAL INFORMATION

This module is used to add, reconfigure, remove network adapters for virtual machine.

- name: Change network adapter settings of virtual machine
  vmware_guest_network:
    hostname: "{{ vcenter_hostname }}"
    username: "{{ vcenter_username }}"
    password: "{{ vcenter_password }}"
    datacenter: "{{ datacenter_name }}"
    validate_certs: no
    name: test-vm
    gather_network_facts: false
    networks:
      - name: "VM Network"
        state: new
        mac: "00:50:56:11:22:33"
      - state: present
        device_type: e1000e
        mac: "00:50:56:44:55:66"
      - state: present
        label: "Network adapter 3"
        connected: false
      - state: absent
        label: "Network adapter 4"
  delegate_to: localhost
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 12, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 12, 2019

@Tomorrow9, just so you are aware we have a dedicated Working Group for vmware.
You can find other people interested in this in #ansible-vmware on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 12, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/modules/cloud/vmware/vmware_guest_network.py:453:0: missing-final-newline Final newline missing

The test ansible-test sanity --test pep8 [explain] failed with 5 errors:

lib/ansible/modules/cloud/vmware/vmware_guest_network.py:78:100: W291 trailing whitespace
lib/ansible/modules/cloud/vmware/vmware_guest_network.py:324:41: E131 continuation line unaligned for hanging indent
lib/ansible/modules/cloud/vmware/vmware_guest_network.py:328:41: E127 continuation line over-indented for visual indent
lib/ansible/modules/cloud/vmware/vmware_guest_network.py:329:41: E127 continuation line over-indented for visual indent
lib/ansible/modules/cloud/vmware/vmware_guest_network.py:453:11: W292 no newline at end of file

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 12, 2019

@MikeKlebolt @Akasurde @bedecarroll @CastawayEGR @chrrrles @dav1x @garbled1 @gyorgypeter @imjoseangel @jjahns @kamsz @karstenjakobsen @kryptsi @lrivallain @nafpliot-ibm @nerzhul @oboukili @rhoop @ritzk @rmin @stravassac @tchernomax @warthog9 @woshihaoren

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@Tomorrow9

This comment has been minimized.

Copy link
Contributor Author

Tomorrow9 commented Feb 15, 2019

I also hit this issue #50656 in this module. On vSphere 6.5, it works well, on vSphere 6.7, there is error "VM network does not exist" and "Hot plug failed". I'll take a look at that. Thanks.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 15, 2019

@Tomorrow9

This comment has been minimized.

Copy link
Contributor Author

Tomorrow9 commented Feb 15, 2019

I also hit this issue #50656 in this module. On vSphere 6.5, it works well, on vSphere 6.7, there is error "VM network does not exist" and "Hot plug failed". I'll take a look at that. Thanks.

Sorry, my mistake, configured the wrong "datacenter" parameter. :(

@xenlo
Copy link
Contributor

xenlo left a comment

Edited… see next Review

@xenlo
Copy link
Contributor

xenlo left a comment

I just tested this new module. Here is my feedback on what I tested, what works, what doesn't.

Adding NICs on an existing VM

  networks:
    - name: "vlan{{ vlan.id }}"
      mac: "{{ 'aa:bb:cc:dd:%02d:%02d' | format((vlan.id |int), (host.id |int)) }}"
      state: new

It works!

That really eases what was theoretically possible to do with vmware_guest module but to hard in practice. So that's a huge improvement for me.

Removing NICs on an existing VM

  networks:
    - name: "vlan{{ vlan.id }}"
      mac: "{{ 'aa:bb:cc:dd:%02d:%02d' | format((vlan.id |int), (host.id |int)) }}"
      state: new

Well here I have 2 issues:

     - C(label) or C(device_type) is required to reconfigure or remove an existing network adapter.
     - 'If there are multiple network adapters with the same C(device_type), you should set C(label),
        or will use the first matched network adapter.'
  • In our use case, we don't work with the label but with the mac. I assume that it would be possible to delete or edit a NIC identified by its mac address. Am I wrong?
  • It sounds strange to me to be able to delete a NIC just based on its device_type. I can imagine use case where you have to change something on all NICs of a defined type, … but not on just the first one found.
@Tomorrow9

This comment has been minimized.

Copy link
Contributor Author

Tomorrow9 commented Feb 18, 2019

@xenlo thanks for your reviewing and feedback.
So the first question is: use mac address as one parameter to delete or reconfigure the matched network adapter. Right, that's reasonable. I'm not using this scenario but you do, so I'll add this.
The second one is: for multiple network adapters, what I wanted to do is if we do not get network adapter label, just know their device type and want to configure one of them, we can use it. But specify the "device_type" to reconfigure/remove all of them with that type, this also make sense. I'll make some changes.
Also, if there are any more scenarios not covered in this module, please let me know. Thank you very much.

@xenlo

This comment has been minimized.

Copy link
Contributor

xenlo commented Feb 18, 2019

@Tomorrow9,
I don't see more scenario in the use cases we have here at work. But there is a chance it could be useful in cases #51123 and #51124. So other scenarios to test this module! 😄

@jsteel44 and @zeot,
Can you have a look on the work of @Tomorrow9? I assume it could help you to update mac of a NIC. If so, can you test it for you scenario?

Thanks

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 18, 2019

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/vmware/vmware_guest_network.py:85:76: W291 trailing whitespace
lib/ansible/modules/cloud/vmware/vmware_guest_network.py:88:124: W291 trailing whitespace

click here for bot help

@ansibot ansibot added ci_verified and removed ci_verified labels Feb 18, 2019

@mrmeszaros

This comment has been minimized.

Copy link

mrmeszaros commented Feb 21, 2019

@Tomorrow9 I tried to use it to add a network interface to a linux (ubuntu) machine.

The network interface was added, however, it was not configured.

The vmware_guest module also configures the interface with DHCP or a static IP.

Would it be feasible to incorporate that too?

@Tomorrow9

This comment has been minimized.

Copy link
Contributor Author

Tomorrow9 commented Feb 22, 2019

@mrmeszaros Hi, do you mean IP address and other related configs? These parameters in vmware_guest under "Networks" are used for OS customization, and this module is used to configure vNIC from VM side, it does not manage things inside OS. Thanks.

  • 'Optional parameters per entry (used for OS customization):'
    • ' - C(type) (string): Type of IP assignment (either C(dhcp) or C(static)). C(dhcp) is default.'
    • ' - C(ip) (string): Static IP address (implies C(type: static)).'
    • ' - C(netmask) (string): Static netmask required for C(ip).'
    • ' - C(gateway) (string): Static gateway.'
    • ' - C(dns_servers) (string): DNS servers for this network interface (Windows).'
    • ' - C(domain) (string): Domain name for this network interface (Windows).'
    • ' - C(wake_on_lan) (bool): Indicates if wake-on-LAN is enabled on this virtual network adapter. version_added: 2.5'
    • ' - C(allow_guest_control) (bool): Enables guest control over whether the connectable device is connected. version_added: 2.5'
@mrmeszaros

This comment has been minimized.

Copy link

mrmeszaros commented Feb 22, 2019

@Tomorrow9 Yes I was hoping this module would work in a similar way to the vmware_guest networks customization.

I assumed a similar interface (as for OS customization) would be the way of least astonishment.

However I can see, that OS cusomization is a different resposibility.

Does vmware_guest use some PyVmomi api calls to do the customization, or extra code is needed?

@Akasurde Akasurde removed the needs_triage label Feb 22, 2019

@Tomorrow9

This comment has been minimized.

Copy link
Contributor Author

Tomorrow9 commented Feb 25, 2019

Yeah, there is "vm_obj.CustomizeVM_Task()" to do OS customization work, I think here we do not have requirement to combine customization and reconfigure together, since @Akasurde has separate module for OS customization #38583. Thanks.

@xenlo
Copy link
Contributor

xenlo left a comment

I rested with your last commit. Now the Removing NICs based on the MAC address is working.
But I had strange reaction on the creation as you changed stuff (for the state=new with manual_mac). So I dive deeper in the code. Which bring me more question about:

  • State: present VS new, is new really the good way to go?
  • Which attributes is there to identify the NIC, and which can be used to reconfigure?
    • Can we accept several NIC with the same MAC? for me not
    • Or with the same label? I don't think so neither.
    • If we take the precept that MAC and label identify a NIC and are unique, how do we indicate the new value in the case we want to reconfigure the MAC or the Label?

Here under the stuff to review and discuss…

@ansibot ansibot removed the stale_review label Feb 26, 2019

xenlo added a commit to xenlo/ansible that referenced this pull request Feb 26, 2019

@ansibot ansibot removed the stale_ci label Feb 27, 2019

add new module vmware_guest_network
modify to VirtualEthernetCard to get network device

@Tomorrow9 Tomorrow9 force-pushed the Tomorrow9:fix_pr51066 branch from 3159818 to 4e8056d Mar 20, 2019

@Tomorrow9

This comment has been minimized.

Copy link
Contributor Author

Tomorrow9 commented Mar 20, 2019

@xenlo @sumkincpp, Hi, do you have any more comments for this? Thanks.

@Tomorrow9

This comment has been minimized.

Copy link
Contributor Author

Tomorrow9 commented Mar 20, 2019

@Akasurde, hi, would you please review this PR? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.