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_drs_group - Add VM to DRS Group #81

Closed
vMarkusK opened this issue Mar 27, 2020 · 19 comments · Fixed by #512
Closed

vmware_drs_group - Add VM to DRS Group #81

vMarkusK opened this issue Mar 27, 2020 · 19 comments · Fixed by #512
Assignees
Labels
affects_2.10 feature This issue/PR relates to a feature request has_pr needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly waiting_on_contributor Needs help. Feel free to engage to get things unblocked
Milestone

Comments

@vMarkusK
Copy link

Currently only a new Group von be created or overwritten. If you want to add a single VM, a workarounds needs to be applied.

- name: Get all DRS VM groups
  vmware_drs_group_facts:
      hostname: "{{ vcenter_hostname }}"
      password: "{{ vcenter_password }}"
      username: "{{ vcenter_username }}"
      datacenter_name: "{{ datacenter_name }}"
  delegate_to: localhost    
  register: drs_groups
- name: Display DRS Group Object
  debug:
    msg: "{{ drs_groups }}"
- name: Filter existing VMs from DRS Group Object
  set_fact: 
    vmnames: "{{ drs_groups | json_query( vmnames_query ) }}"
  vars:
    vmnames_query: 'drs_group_facts.{{ clustername }}[?group_name==`{{ vm_group }}`].vms'
- name: Display DRS Group VM List
  debug:
    msg: "{{ vmnames[0] }}"
  when: vmnames | list | count  > 0
- name: Display new DRS Group VM List
  debug:
    msg: "{{ vmnames[0] + [ vm_name ] }}"
  when: vmnames | list | count  > 0  
- name: Build full VM  List
  set_fact:
    all_vmnames: "{{ vmnames[0] + [ vm_name ] }}"
  when: vmnames | list | count  > 0
- name: Add {{ vm_name }} to exiting DRS VM group
  vmware_drs_group:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: "{{ clustername }}" 
    datacenter_name: "{{ datacenter_name }}"
    group_name: "{{ vm_group }}"
    vms: "{{ all_vmnames }}"
    state: present
  delegate_to: localhost  
  when: vmnames | list | count > 0
- name: Add {{ vm_name }} to new DRS VM group
  vmware_drs_group:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: "{{ clustername }}"
    datacenter_name: "{{ datacenter_name }}"
    group_name: "{{ vm_group }}"
    vms:
      - "{{ vm_name }}"
    state: present
  delegate_to: localhost  
  when: vmnames | list | count == 0

This is verry complex and not pretty stable.

@mariolenz
Copy link
Collaborator

@alexsuppanz
Copy link

Hi
ran into the same issue. Since this module seems to work as expected (which I do not agree, anyway), maybe it would be a solution to create a new module which will add vms to existing DRS groups?
As @mycloudrevolution mentioned, any workaround is pretty unstable and error prone

@alexsuppanz
Copy link

What would be the proper way to resolve this issue?
Create a new module which will add VMs to existring groups, or change the current one to add the new VM to the group?

@vMarkusK
Copy link
Author

vMarkusK commented May 4, 2020

I would prefer a single module with three different methods.

Set, add, remove

Set method should be the current behavior.

@alexsuppanz
Copy link

Maybe we should do the following:
state: present will add the provided VM names to the group
state: replace will overwrite the group with the provided VM names (as it is currently the case)
state: absent will remove the group

So the states are alligned to the Ansible convention. I also think that current plays which use any kind of workaround will be not affected by this change.

@mariolenz
Copy link
Collaborator

state: present will add the provided VM names to the group
state: replace will overwrite the group with the provided VM names (as it is currently the case)
state: absent will remove the group

This would break the current behavior of the module... never a good idea. Anyway, you missed a use case: If we provide a way to add a VM to a DRS group, we also should provide a way to remove a VM.

I think it would be better to leave state: present and state: absent as is. The module is named vmware_drs_group and it makes sense that present means the DRS group should be present as defined.

I think it would be better to add states like:

state: contains will add the provided VMs to the group
state: does_not_contain will remove the provided VMs from the group

So the states are alligned to the Ansible convention.

Is there some explicit convention / best practice for states? I wasn't able to find one. But maybe I was just too stupid to find it or there's some implicit convention I don't know about.

I also think that current plays which use any kind of workaround will be not affected by this change.

All playbooks that use the module as it's meant to be used will break! Let's pretend someone defines a DRS group with vm1, vm2 and vm3. Then they decide that they only want to have vm1 and vm2 in this group. At the moment, the DRS group won't contain vm3 any more; with your solution, it would: vm1 and vm2 are added to this DRS group (no change, they are already part of it) but vm3 won't be removed. So (and this is really problematic) your solution breaks existing playbooks.

@alexsuppanz
Copy link

@mariolenz You are absolutely right, thanks for pointing it out. I missed these cases and forgot how this module is intended to work. Maybe it would be better to create a new module, as already suggested by @mycloudrevolution called something like vmware_drs_group_member
So we can leave the module vmware_drs_group as it is (and we won't break stuff, even by accident) and keep the modules simple. Adding all these cases to the current module would make it very complex and may lead to various bugs.
In the new module we can use the well known states present to ensure the vm is member ob the group, and absent to remove the vm.
What do you guys think about that?

@vMarkusK
Copy link
Author

vMarkusK commented May 5, 2020

I fully agree with @mariolenz point to make sure not to break existing playbooks with a new module version.

I also see the need for the four types of behavior: Create Group (with a list of VMs), remove group add VM, remove VM.

If the dedicated modules for Group and VM operations are created (which is likely), I would like to see that a specific workflow is possible without much fuss:

  • Add VM to Group, if Group does not exist create the Group with this VM

@alexsuppanz
Copy link

@mycloudrevolution I totally see the use case for the workflow you mentioned, but we do have a problem. We can create a group with vmware_drs_group and add members to a group with the new module vmware_drs_group_member. So a play with the mentioned workflow will look like this:

  - name: Ensure {{ vcenter_vm_group }} is present
    vmware_drs_group:
      hostname: "{{ vcenter_hostname }}"
      password: "{{ user_password }}"
      username: "{{ user_name }}"
      cluster_name: "{{ vcenter_cluster }}"
      datacenter_name: "{{ vcenter_datacenter }}"
      group_name: "{{ vcenter_vm_group }}"
      state: present

  - name: Add vm {{ vm_name }} to group {{ vcenter_vm_group }}
    vmware_drs_group_member:
      hostname: "{{ vcenter_hostname }}"
      password: "{{ user_password }}"
      username: "{{ user_name }}"
      cluster_name: "{{ vcenter_cluster }}"
      datacenter_name: "{{ vcenter_datacenter }}"
      group_name: "{{ vcenter_vm_group }}"
      vm: {{ vm_name }}
      state: present

But the problem is, vmware_drs_group requires either the parameter vms or hosts to be set, which will (as it is currently the case) all current members of that group exept of the ones provides as parameter.

I do not know your exact use case, but if you already know that you have to create a group because it does not exist yet you can use the current module vmware_drs_group and provide the group member. If you just want to add a newly provisioned vm to an existing group, use the new module.

You can also check if the group already exists using the module vmware_drs_group_facts and check if it contains the group. If not, the group must be created first.

Another option would be to 'simply' change the behaviour of the module vmware_drs_group to not have vms or hosts as mandatory parameter and add a new parameter type, which says the group is either a 'VM Group' or a 'Host Group' (and make the parameter vms, hosts and type mutial exclusive)

I know, it is getting complex, but we should make the workflow as simple as possible and do not break existing playbooks. What do you think about that? Do I missed something and there may be a simpler solution?

@vMarkusK
Copy link
Author

vMarkusK commented May 6, 2020

For my opinion the new modules should work like that:

  • The original module works as before (for Hosts and VMs)
    NOTE: Creating empty Groups in not allowed
- name: "Create DRS VM group"
  delegate_to: localhost
  vmware_drs_group:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: DC0_C0
    datacenter_name: DC0
    group_name: TEST_VM_01
    vms:
      - DC0_C0_RP0_VM0
      - DC0_C0_RP0_VM1
    state: present

- name: "Create DRS Host group"
  delegate_to: localhost
  vmware_drs_group:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: DC0_C0
    datacenter_name: DC0
    group_name: TEST_HOST_01
    hosts:
      - DC0_C0_H0
      - DC0_C0_H1
      - DC0_C0_H2
    state: present

- name: "Delete DRS Host group"
  delegate_to: localhost
  vmware_drs_group:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: DC0_C0
    datacenter_name: DC0
    group_name: TEST_HOST_01
    state: absent
  • A new Module for add and remove of VMs should be created
    NOTE: force_create switch should create the Group if it does not exist, like the vmware_drs_group module
- name: "Add VMs to DRS VM group"
  delegate_to: localhost
  vmware_drs_group_member:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: DC0_C0
    datacenter_name: DC0
    group_name: TEST_VM_01
    force_create: true
    vms:
      - DC0_C0_RP0_VM0
      - DC0_C0_RP0_VM1
    state: present

- name: "remove VMs from DRS VM group"
  delegate_to: localhost
  vmware_drs_group_member:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: DC0_C0
    datacenter_name: DC0
    group_name: TEST_VM_01
    vms:
      - DC0_C0_RP0_VM0
      - DC0_C0_RP0_VM1
    state: absent

- name: "Add hosts to DRS Host group"
  delegate_to: localhost
  vmware_drs_group_member:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: DC0_C0
    datacenter_name: DC0
    group_name: TEST_HOST_01
    hosts:
      - DC0_C0_H0
      - DC0_C0_H1
      - DC0_C0_H2
    state: present

- name: "remove Hosts to DRS Host group"
  delegate_to: localhost
  vmware_drs_group_member:
    hostname: "{{ vcenter_hostname }}"
    password: "{{ vcenter_password }}"
    username: "{{ vcenter_username }}"
    cluster_name: DC0_C0
    datacenter_name: DC0
    group_name: TEST_HOST_01
    state: absent

@ansibullbot
Copy link

@vMarkusK: Greetings! Thanks for taking the time to open this issue. In order for the community to handle your issue effectively, we need a bit more information.

Here are the items we could not find in your description:

  • component name

Please set the description of this issue with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/ISSUE_TEMPLATE/bug_report.md

click here for bot help

@ansibullbot ansibullbot added affects_2.10 feature This issue/PR relates to a feature request needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly needs_triage Needs a first human triage before being processed. labels Aug 19, 2020
@Akasurde Akasurde added waiting_on_contributor Needs help. Feel free to engage to get things unblocked and removed needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly needs_triage Needs a first human triage before being processed. labels Oct 21, 2020
@richardlock
Copy link

Hi. Are there any plans to look at this issue? Thanks.

@danielmhi
Copy link

Another call for this to be looked at? The introduction of vmware_drs_group_member to add VMs to an already existing DRS Group would be great.

@vMarkusK
Copy link
Author

Sadly I am only able to test and verify. My Python skills are not on a level that makes the development possible.

Akasurde added a commit to Akasurde/community.vmware that referenced this issue Nov 3, 2020
Added new parameter 'operation' to manage existing members
in the existing DRS group. Users can now add, set, and remove
existing members in DRS Group

fixes: ansible-collections#81

Signed-off-by: Abhijeet Kasurde <[email protected]>
@Akasurde
Copy link
Member

Akasurde commented Nov 3, 2020

@vMarkusK @danielmhi @richardlock @alexsuppanz I added a new parameter called operation with values set, add and remove to manage existing members of DRS group. Please check and test #475, let me know your views. Thanks,

@Akasurde
Copy link
Member

Akasurde commented Nov 3, 2020

resolved_by_pr #512

@Akasurde Akasurde added has_pr and removed waiting_on_contributor Needs help. Feel free to engage to get things unblocked labels Nov 3, 2020
@Akasurde Akasurde self-assigned this Nov 3, 2020
@danielmhi
Copy link

@Akasurde will test when i can, thx

Akasurde added a commit to Akasurde/community.vmware that referenced this issue Nov 20, 2020
@Akasurde
Copy link
Member

I added a new module to manage VMs/Hosts in DRS group - vmware_drs_group_manager. Please review and let me know.

@Akasurde
Copy link
Member

resolved_by_pr #512

@Akasurde Akasurde added this to the v.1.5.0 milestone Nov 23, 2020
Akasurde added a commit to Akasurde/community.vmware that referenced this issue Nov 30, 2020
Akasurde added a commit to Akasurde/community.vmware that referenced this issue Dec 3, 2020
Akasurde added a commit to Akasurde/community.vmware that referenced this issue Dec 3, 2020
@Akasurde Akasurde modified the milestones: v.1.5.0, v1.6.0 Dec 7, 2020
Akasurde added a commit to Akasurde/community.vmware that referenced this issue Dec 9, 2020
@Akasurde Akasurde modified the milestones: v1.6.0, v1.7.0 Jan 6, 2021
Akasurde added a commit to Akasurde/community.vmware that referenced this issue Jan 6, 2021
@ansibullbot ansibullbot added needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly waiting_on_contributor Needs help. Feel free to engage to get things unblocked labels Jan 15, 2021
Akasurde added a commit to Akasurde/community.vmware that referenced this issue Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects_2.10 feature This issue/PR relates to a feature request has_pr needs_info This issue requires further information. Please answer any outstanding questions needs_template This issue/PR has an incomplete description. Please fill in the proposed template correctly waiting_on_contributor Needs help. Feel free to engage to get things unblocked
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants