VMware: add new module vmware_host_iscsi #224

sky-joker · 2020-06-05T19:28:40Z

Depends-On: ansible/ansible-zuul-jobs#563

SUMMARY

This PR adds a module that Manages iSCSI config of ESXi.
fix: #159

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

vmware_host_iscsi

ADDITIONAL INFORMATION

tested with vCenter6.7/ESXi6.7

pabelanger · 2020-06-07T19:22:22Z

recheck

goneri · 2020-06-08T14:11:56Z

recheck

goneri · 2020-06-08T17:45:46Z

The CI issue seems to be legit:

2020-06-08 15:17:46.367290 | centos-8 | TASK [vmware_host_iscsi : assert] **********************************************
2020-06-08 15:17:46.367543 | centos-8 | task path: /home/zuul/.ansible/collections/ansible_collections/community/vmware/tests/integration/targets/vmware_host_iscsi/tasks/connect_to_vcenter_tasks.yml:30
2020-06-08 15:17:46.493804 | centos-8 | fatal: [testhost]: FAILED! => {
2020-06-08 15:17:46.493863 | centos-8 |     "assertion": "enable_iscsi_result.changed is sameas true",
2020-06-08 15:17:46.493876 | centos-8 |     "changed": false,
2020-06-08 15:17:46.493886 | centos-8 |     "evaluated_to": false,
2020-06-08 15:17:46.493895 | centos-8 |     "msg": "Assertion failed"
2020-06-08 15:17:46.493904 | centos-8 | }

sky-joker · 2020-06-10T07:53:32Z

recheck

sky-joker · 2020-06-10T08:52:17Z

recheck

sky-joker · 2020-06-10T09:32:21Z

recheck

goneri · 2020-06-10T18:57:06Z

Hi @sky-joker, the govcsim tests are broken. I'm working on that.

goneri · 2020-06-11T02:36:21Z

recheck

goneri · 2020-06-11T16:01:17Z

recheck

goneri · 2020-06-11T23:19:51Z

recheck

sky-joker · 2020-06-12T02:13:18Z

recheck

fix: ansible-collections#159 fix flake8 F401 error add prepare of integration test for iSCSI task fix block syntax error add changelog file

goneri · 2020-06-15T18:23:15Z

changelogs/fragments/224-vmware_host_iscsi.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - vmware_host_iscsi - new code module for a new feature for manage the iSCSI configuration of ESXi host


I suggest instead: a new module for the ESXi hosts that is dedicated to the management of the iSCSI configuration

Thank you for your suggestions.
I have fixed :)

goneri · 2020-06-15T18:24:41Z

tests/integration/targets/vmware_host_iscsi/tasks/main.yml

@@ -0,0 +1,50 @@
+# Test code for the vmware_host_logbundle module.


vmware_host_logbundle -> vmware_host_iscsi, but I don't think need the header anyway since it's error prone.

I fixed the header, should I remove it?

Nah it's fine.

goneri · 2020-06-15T18:29:17Z

tests/integration/targets/vmware_host_iscsi/tasks/reset_iscsi_config_tasks.yml

+        password: "{{ vcenter_password }}"
+        validate_certs: no
+        esxi_hostname: "{{ esxi1 }}"
+        state: disabled


Could you add some assert to validate the results?

goneri · 2020-06-15T18:30:37Z

tests/integration/targets/vmware_host_iscsi/tasks/reset_iscsi_config_tasks.yml

+            address: 100.64.0.2
+        state: absent
+
+    - vmware_host_iscsi:


If I understand correctly, this reset the host to its initial state. Can you call the task from always section. This way, we will be sure we restore the test environment after a failure.

@sky-joker can you address this comment?

Thank you for your comment.
I've removed a rest_iscsi_config_tasks.yml, and I have moved the tests that weren't duplicated in reset_iscsi_config_tasks.yml to iscsi_module_test_tasks.ym

https://github.com/ansible-collections/vmware/blob/027b3ff3457644cf72c43ab2f07588007f9dabff/tests/integration/targets/vmware_host_iscsi/tasks/iscsi_module_test_tasks.yml#L840-L891

https://github.com/ansible-collections/vmware/blob/027b3ff3457644cf72c43ab2f07588007f9dabff/tests/integration/targets/vmware_host_iscsi/tasks/iscsi_module_test_tasks.yml#L1063-L1105

So, the playbook for the reset process is no longer needed.
I'm sorry if the answer is not what you expected.

goneri · 2020-06-15T18:31:56Z

tests/integration/targets/vmware_host_iscsi/tasks/reset_iscsi_config_tasks.yml

+            - vmk0
+          vmhba_name: "{{ vmhba_name }}"
+          send_target:
+            address: 100.64.0.1


I'm a but confused because you call the file reset_iscsi_config_tasks.yml , but you actually set some new configuration.

Tasks performed by reset_iscsi_config_tasks have been moved toconnect_to_vcenter_tasks.yml and connect_to_esxi_tasks.yml.
Because of that, I have removed reset_iscsi_config_tasksl.yml file.

sky-joker · 2020-06-16T14:02:07Z

Thank you @goneri for reviewing.
I'll try to fix the part that was pointed out.

xenlo

We are testing a new storage and I had to try to automate the deployment process. I was really happy to find this PR! (thanks @sky-joker). And I took the opportunity of the review to provide my feed back.
So consider my review more like a functional test than a code review.

xenlo · 2020-06-16T13:11:49Z

tests/integration/targets/vmware_host_iscsi/tasks/main.yml

+    - "'device' in item"
+    - "'isSoftwareBased' in item and item.isSoftwareBased is sameas true"
+
+- include_tasks: connect_to_vcenter_tasks.yml


connect_to_vcenter_tasks.yml and connect_to_esxi_tasks.yml are really similar. Maybe it could be re-factored to one file with generic variable names for the connections (hostname, username and password).

- include_tasks: connect_tasks.yml vars: hostname: "{{ vcenter_hostname }}" username: "{{ vcenter_username }}" password: "{{ vcenter_password }}" - include_tasks: reset_iscsi_config_tasks.yml - include_tasks: connect_tasks.yml vars: hostname: "{{ esxi1 }}" username: "{{ esxi_user }}" password: "{{ esxi_password }}"

Surely this is more concise.
I'm going to combine the test files into one.

xenlo · 2020-06-16T13:15:26Z

tests/integration/targets/vmware_host_iscsi/tasks/main.yml

+        password: "{{ vcenter_password }}"
+        validate_certs: no
+        esxi_hostname: "{{ esxi1 }}"
+        state: disabled


As far as I understood it, the ESXi need a reboot after disabling the iSCSI Software Adapter.

Note that I have no idea, if it's possible and/or a good idea to reboot the node and wait for host to be back in the middle of the integration tests.

As far as I understood it, the ESXi need a reboot after disabling the iSCSI Software Adapter.

You're right.
But in the integration test environment, there is no target for iSCSI.
For that reason, I've decided that as long as the iSCSI settings are in ESXi, it's OK.

xenlo · 2020-06-16T13:37:35Z

tests/integration/targets/vmware_host_iscsi/tasks/main.yml

+      - config.storageDevice.hostBusAdapter
+  register: gather_facts_host_bus_adapters_result
+
+- name: set iSCSI hba name to vmhba_name variable


This is not trivial to get a simple fact like a device name.

xenlo · 2020-06-16T14:28:18Z

plugins/modules/vmware_host_iscsi.py

+                        except Exception as e:
+                            self.module.fail_json(msg="Failed to remove a port bind: %s" % to_native(e))
+
+        if result['changed'] is True:


It could be very handy to get the config returned even there was no change (except if that's a bad practice in modules).

Taking my test scenario, I use first vmware_host_iscsi to enable the software adapter. From the register of that tasks, I get the vmhba_name to call a second time the vmware_host_iscsi module to set the Dynamic target for instance (see playbook snip here under). But this will fail in case the iSCSI Software adapter is already enabled as the iscsi_properties are not returned (see the outputs here under).

An other possibility could be to provide a module like vmware_hosts_iscsi_facts. (Note that I saw in the integration tests that you get the vmhba_name but I don't find that handy at all)

Snip of my test playbook

- name: Enable iSCSI Software adapter on esxi nodes vmware_host_iscsi: hostname: "{{ ansible_host }}" username: "{{ ansible_user }}" password: "{{ ansible_ssh_pass }}" esxi_hostname: "{{ ansible_host }}" validate_certs: no state: enabled register: node_iscsi_adapt delegate_to: localhost - name: Print iscsi adapter debug: var: node_iscsi_adapt - name: Configure iSCSI dynamic targets of esxi nodes vmware_host_iscsi: hostname: "{{ ansible_host }}" username: "{{ ansible_user }}" password: "{{ ansible_ssh_pass }}" esxi_hostname: "{{ ansible_host }}" validate_certs: no state: present iscsi_config: vmhba_name: "{{ node_iscsi_adapt.iscsi_properties.vmhba_name }}" send_target: - address: "{{ nimble_iscsi_ip1 }}" - address: "{{ nimble_iscsi_ip2 }}" register: cnode_iscsi_config delegate_to: localhost

Output on change

TASK [Enable iSCSI Software adapter on nodes] *********************************************************************************************************************************************************************************************** Tuesday 16 June 2020 11:46:59 +0200 (0:00:02.176) 0:00:02.324 ********** changed: [node03 -> localhost] TASK [Print iscsi adapter] ******************************************************************************************************************************************************************************************************************* Tuesday 16 June 2020 11:47:03 +0200 (0:00:03.968) 0:00:06.292 ********** ok: [node03] => node_iscsi_adapt: changed: true failed: false iscsi_properties: iscsi_alias: '' iscsi_authentication_properties: _vimtype: vim.host.InternetScsiHba.AuthenticationProperties chapAuthEnabled: false chapAuthenticationType: chapProhibited chapInherited: null chapName: '' chapSecret: XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX mutualChapAuthenticationType: chapProhibited mutualChapInherited: null mutualChapName: '' mutualChapSecret: XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX iscsi_enabled: true iscsi_name: iqn.1998-01.com.vmware:node03-02f56186 iscsi_send_targets: [] iscsi_static_targets: [] port_bind: [] vmhba_name: vmhba64

Output on status 'ok'

TASK [Enable iSCSI Software adapter on nodes] *********************************************************************************************************************************************************************************************** Tuesday 16 June 2020 11:46:08 +0200 (0:00:00.194) 0:00:00.194 ********** ok: [node01 -> localhost] TASK [Print iscsi adapter] ******************************************************************************************************************************************************************************************************************* Tuesday 16 June 2020 11:46:11 +0200 (0:00:03.803) 0:00:03.998 ********** ok: [node01] => node_iscsi_adapt: changed: false failed: false

okay :)
I'll be creating a vmware_host_iscsi_info module (It may take some time, but ...)

ansible-collections#224 (comment) ansible-collections#224 (comment) ansible-collections#224 (comment) delete a reset_iscsi_config_tasks.yml

sky-joker · 2020-06-16T14:57:45Z

Thank you @xenlo for the feedback!
I'll check your comments and respond to them.

ansible-collections#224 (comment)

sky-joker · 2020-06-17T01:15:34Z

recheck

sky-joker · 2020-06-17T03:06:11Z

recheck

sky-joker · 2020-06-17T07:09:20Z

recheck

sky-joker · 2020-06-22T14:18:58Z

ready_for_review

goneri · 2020-06-23T19:59:17Z

tests/integration/targets/vmware_host_iscsi/tasks/main.yml

@@ -0,0 +1,50 @@
+# Test code for the vmware_host_logbundle module.


Nah it's fine.

goneri · 2020-06-23T20:00:02Z

tests/integration/targets/vmware_host_iscsi/tasks/reset_iscsi_config_tasks.yml

+            address: 100.64.0.2
+        state: absent
+
+    - vmware_host_iscsi:


@sky-joker can you address this comment?

goneri · 2020-06-29T16:10:11Z

Thank you @sky-joker for your contribution :-).

sky-joker · 2020-06-30T11:26:31Z

Thank you @goneri :)

VMware: add new module vmware_host_iscsi Reviewed-by: https://github.com/apps/ansible-zuul

sky-joker force-pushed the new_module_vmware_host_iscsi branch from 8f5a1bc to a65060c Compare June 6, 2020 08:06

sky-joker closed this Jun 6, 2020

sky-joker reopened this Jun 6, 2020

sky-joker mentioned this pull request Jun 6, 2020

vmware esxi software iscsi #159

Closed

pabelanger closed this Jun 6, 2020

pabelanger reopened this Jun 6, 2020

sky-joker closed this Jun 7, 2020

sky-joker reopened this Jun 7, 2020

goneri closed this Jun 8, 2020

goneri reopened this Jun 8, 2020

sky-joker force-pushed the new_module_vmware_host_iscsi branch from f348dc1 to 7d1787d Compare June 9, 2020 15:38

sky-joker closed this Jun 12, 2020

sky-joker reopened this Jun 12, 2020

sky-joker closed this Jun 12, 2020

sky-joker reopened this Jun 12, 2020

add new module vmware_host_iscsi

44888be

fix: ansible-collections#159 fix flake8 F401 error add prepare of integration test for iSCSI task fix block syntax error add changelog file

sky-joker force-pushed the new_module_vmware_host_iscsi branch from e2f28a4 to 44888be Compare June 14, 2020 06:15

sky-joker closed this Jun 14, 2020

sky-joker reopened this Jun 14, 2020

sky-joker closed this Jun 14, 2020

sky-joker reopened this Jun 14, 2020

goneri requested changes Jun 15, 2020

View reviewed changes

goneri requested a review from Akasurde June 15, 2020 18:32

xenlo suggested changes Jun 16, 2020

View reviewed changes

fix of the pointed out part

46a8000

ansible-collections#224 (comment) ansible-collections#224 (comment) ansible-collections#224 (comment) delete a reset_iscsi_config_tasks.yml

combine integration test files into one

027b3ff

ansible-collections#224 (comment)

goneri requested changes Jun 23, 2020

View reviewed changes

goneri approved these changes Jun 29, 2020

View reviewed changes

goneri added the gate label Jun 29, 2020

ansible-zuul bot merged commit 93a3c2e into ansible-collections:master Jun 29, 2020

sky-joker deleted the new_module_vmware_host_iscsi branch June 30, 2020 11:26

phsmith pushed a commit to phsmith/vmware that referenced this pull request Jul 6, 2020

VMware: add new module vmware_host_iscsi (ansible-collections#224)

a6e90ad

VMware: add new module vmware_host_iscsi Reviewed-by: https://github.com/apps/ansible-zuul

sky-joker mentioned this pull request Sep 26, 2020

add a new module vmware_host_iscsi_info #402

Merged

		@@ -0,0 +1,2 @@
		minor_changes:
		- vmware_host_iscsi - new code module for a new feature for manage the iSCSI configuration of ESXi host

		@@ -0,0 +1,50 @@
		# Test code for the vmware_host_logbundle module.

VMware: add new module vmware_host_iscsi #224

VMware: add new module vmware_host_iscsi #224

Conversation

sky-joker commented Jun 5, 2020 • edited by goneri Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

pabelanger commented Jun 7, 2020

goneri commented Jun 8, 2020

goneri commented Jun 8, 2020

sky-joker commented Jun 10, 2020

sky-joker commented Jun 10, 2020

sky-joker commented Jun 10, 2020

goneri commented Jun 10, 2020

goneri commented Jun 11, 2020

goneri commented Jun 11, 2020

goneri commented Jun 11, 2020

sky-joker commented Jun 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sky-joker Jun 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sky-joker Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

sky-joker commented Jun 16, 2020

xenlo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xenlo Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snip of my test playbook

Output on change

Output on status 'ok'

sky-joker Jun 16, 2020 • edited Loading

Choose a reason for hiding this comment

sky-joker commented Jun 16, 2020

sky-joker commented Jun 17, 2020

sky-joker commented Jun 17, 2020

sky-joker commented Jun 17, 2020

sky-joker commented Jun 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goneri commented Jun 29, 2020

sky-joker commented Jun 30, 2020 • edited Loading

sky-joker commented Jun 5, 2020 •

edited by goneri

Loading

sky-joker Jun 24, 2020 •

edited

Loading

sky-joker Jun 16, 2020 •

edited

Loading

xenlo Jun 16, 2020 •

edited

Loading

sky-joker Jun 16, 2020 •

edited

Loading

sky-joker commented Jun 30, 2020 •

edited

Loading