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

Support "force_subnet_association" parameter on na_ontap_interface #52691

Merged
merged 3 commits into from
May 1, 2019

Conversation

angystardust
Copy link
Contributor

SUMMARY

This small patch adds a new parameter force_subnet_association to the na_ontap_interface module.
In our storage provisioning workflow, we're creating a subnet with a fixed network range so we need to force the ip association of a LIF to the provisioned subnet.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

na_ontap_interface.py

ADDITIONAL INFORMATION
ansible 2.7.6
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Oct 30 2018, 23:45:53) [GCC 4.8.5 20150623 (Red Hat 4.8.5-36)]

@angystardust
Copy link
Contributor Author

+label netapp
+label storage

@ansibot
Copy link
Contributor

ansibot commented Feb 21, 2019

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

lib/ansible/modules/storage/netapp/na_ontap_interface.py:0:0: E325 Argument 'force_subnet_association' in argument_spec defines type as <class 'bool'> but documentation defines type as 'bool'

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Feb 21, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 ci_verified Changes made in this PR are causing tests to fail. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. netapp storage support:certified This issue/PR relates to certified code. committer_review In order to be merged, this PR must follow the certified review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 21, 2019
Copy link
Contributor

@lonico lonico left a comment

Choose a reason for hiding this comment

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

shipit

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Feb 21, 2019
Copy link
Contributor

@carchi8py carchi8py left a comment

Choose a reason for hiding this comment

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

Shipit

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 1, 2019
Copy link
Contributor

@carchi8py carchi8py left a comment

Choose a reason for hiding this comment

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

Shipit

Copy link
Contributor

@lonico lonico left a comment

Choose a reason for hiding this comment

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

shipit

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels Mar 29, 2019
@gundalow
Copy link
Contributor

devel is now for Ansible 2.9.
Also branch needs rebase

@ansibot
Copy link
Contributor

ansibot commented Apr 26, 2019

@angystardust this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibot ansibot added merge_commit This PR contains at least one merge commit. Please resolve! and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Apr 26, 2019
@thedoubl3j
Copy link
Member

@angystardust looks like you might need to rebase, after that we should be good 👍

@ansibot ansibot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Apr 29, 2019
@angystardust
Copy link
Contributor Author

Rebase done, @thedoubl3j
Thanks for your review

@ansibot ansibot added committer_review In order to be merged, this PR must follow the certified review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 30, 2019
Copy link
Contributor

@lonico lonico left a comment

Choose a reason for hiding this comment

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

shipit

Copy link
Member

@thedoubl3j thedoubl3j left a comment

Choose a reason for hiding this comment

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

lgtm, shipit

@ansibot ansibot added shipit This PR is ready to be merged by Core and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels May 1, 2019
@thedoubl3j thedoubl3j merged commit ac657f6 into ansible:devel May 1, 2019
@ilfox
Copy link

ilfox commented May 15, 2019

The force_subnet_association did not work for me on ontap 9.5. Also if I go for the option to take an IP from the subnet list i.e. by using subnet_name the address and netmask parameters should not be required. In fact when verifying via command line, with -subnet-name the -address parameter will not be available.

@angystardust
Copy link
Contributor Author

Hi @ilfox I personally developed and tested the patch on NetApp Release 9.4P2 :(

@carchi8py
Copy link
Contributor

@ilfox @angystardust Let me add an internal story to test this on Ontap 9.1 through 9.6 and see which version are having issues (for force_subnet_assoication).

For subnet and address being required together, let me ask the team and get back to you.

@ilfox
Copy link

ilfox commented May 15, 2019

To be clear, at first I tried creating a lif by assigning a new IP which is not in the subnet list because if it's already in the list it will fail and since no gateway was being assigned, it looks like the force_subnet_assoication was not working. In the end the way I got it working as required was by adding the IPs when creating the subnet in the same order I would be creating the LIFs.

@angystardust
Copy link
Contributor Author

Ok so I suppose the issue you're experiencing is due to the fact that you tried to assign a ip address that is outside the defined subnet...
This is the workflow we defined in our playbook:


    - name: "create subnets for {{ cust|upper }}"
      na_ontap_net_subnet:
        state: present
        name: "{{ netapp_cluster_name_prefix }}-{{ item.name }}"
        subnet: "{{ item.subnet }}"
        ip_ranges: "{{ item.ip_ranges }}"
        ipspace: "{{ ipspace | default('Default') }}"
        broadcast_domain: "{{ netapp_cluster_name_prefix }}-{{ item.name }}"
        <<: *na_connection
      loop: "{{ svm_vlans }}"
      tags:
        - net
        - subnet

    - name: "create LIFs for {{ svm_name }}"
      na_ontap_interface:
        state: present
        interface_name: "{{ netapp_cluster_name }}_svm_{{ svm_allowed_protocols | join('_') }}_{{ item[0].name }}_{% if 'none' in item[1].protocols %}mgmt{% else %}{{ item[1].protocols | join('_') }}{% endif %}_lif_{{ item[1].node }}"
        home_port: "{{ parent_port }}-{{ item[0].id }}"
        home_node: "{{ item[1].node }}"
        role: data
        protocols: "{{ item[1].protocols }}"
        admin_status: up
        failover_policy: "{{ item[1].failover_policy }}"
        firewall_policy: "{{ item[1].firewall_policy }}"
        is_auto_revert: "{{ item[1].is_auto_revert | default(false) }}"
        force_subnet_association: true # Patch
        ddns_update: false # Patch
        address: "{{ item[1].address }}"
        netmask: "{{ item[0].subnet | ipaddr('netmask') }}"
        vserver: "{{ svm_name }}" # Computed
        <<: *na_connection
      with_subelements:
        - "{{ svm_vlans }}"
        - lifs
        - flags:
          skip_missing: True
      tags: lifs


This is the "svm_vlans" variable dictonary:

svm_vlans:
  - name: "{{ netprefix }}dmz-data"
    id: 40
    subnet: "192.168.208.0/26"
    ip_ranges: 
      - '192.168.208.18-192.168.208.19'
      - '192.168.208.21-192.168.208.22'
    lifs:
      - { address: 192.168.208.18, node: netapp-nac2h1, protocols: ['none'], failover_policy: system-defined, firewall_policy: mgmt }
      - { address: 192.168.208.19, node: netapp-nac2h2, protocols: ['none'], failover_policy: system-defined, firewall_policy: mgmt }
      - { address: 192.168.208.21, node: netapp-nac2h1, protocols: ['nfs'],  failover_policy: system-defined, firewall_policy: data }
      - { address: 192.168.208.22, node: netapp-nac2h2, protocols: ['nfs'],  failover_policy: system-defined, firewall_policy: data }

I hope I have helped you with my example :)

ndclt pushed a commit to ndclt/ansible that referenced this pull request Jun 13, 2019
…nsible#52691)

* Support "force_subnet_association" parameter on na_ontap_interface

* fix validate-modules issue

* - Fix spurious commit
@ansible ansible locked and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 feature This issue/PR relates to a feature request. module This issue/PR relates to a module. netapp shipit This PR is ready to be merged by Core storage support:certified This issue/PR relates to certified code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants