Modify port would remove and re-add ports on a modify which can results in an error (#53341)

* Revert "changes to clusteR"

This reverts commit 33ee1b71e4bc8435fb315762a871f8c4cb6c5f80.

* Revert "changes to clusteR"

This reverts commit 33ee1b71e4bc8435fb315762a871f8c4cb6c5f80.

* Revert "Revert "changes to clusteR""

This reverts commit f1104a37b4.

* Revert "Revert "changes to clusteR""

This reverts commit f1104a37b4.

* documentation changes

* Revert "documentation changes"

This reverts commit 02c369d0414fdff492d90865c903bdade3174261.

* Issue with port being removed and added on modify

lib/ansible/modules/storage/netapp/na_ontap_net_ifgrp.py

@@ -1,6 +1,6 @@
 #!/usr/bin/python
 
-# (c) 2018, NetApp, Inc
+# (c) 2018-2019, NetApp, Inc
 # GNU General Public License v3.0+ (see COPYING or https://www.gnu.org/licenses/gpl-3.0.txt)
 
 from __future__ import absolute_import, division, print_function
@@ -248,10 +248,12 @@ class NetAppOntapIfGrp(object):
                                   exception=traceback.format_exc())
 
     def modify_ports(self, current_ports):
-        for port in current_ports:
-            self.remove_port_to_if_grp(port)
-        for port in self.parameters['ports']:
+        add_ports = set(self.parameters['ports']) - set(current_ports)
+        remove_ports = set(current_ports) - set(self.parameters['ports'])
+        for port in add_ports:
             self.add_port_to_if_grp(port)
+        for port in remove_ports:
+            self.remove_port_to_if_grp(port)
 
     def remove_port_to_if_grp(self, port):
         """
@@ -275,9 +277,9 @@ class NetAppOntapIfGrp(object):
 
     def apply(self):
         self.autosupport_log()
-        current = self.get_if_grp()
+        current, modify = self.get_if_grp(), None
         cd_action = self.na_helper.get_cd_action(current, self.parameters)
-        if current and self.parameters['state'] == 'present':
+        if cd_action is None and self.parameters['state'] == 'present':
             current_ports = self.get_if_grp_ports()
             modify = self.na_helper.get_modified_attributes(current_ports, self.parameters)
         if self.na_helper.changed:

test/units/modules/storage/netapp/test_na_ontap_net_ifgrp.py

@@ -17,7 +17,7 @@ from ansible.modules.storage.netapp.na_ontap_net_ifgrp \
     import NetAppOntapIfGrp as ifgrp_module  # module under test
 
 if not netapp_utils.has_netapp_lib():
-    pytestmark = pytest.skip('skipping as missing required netapp_lib')
+    pytestmark = pytest.mark.skip('skipping as missing required netapp_lib')
 
 
 def set_module_args(args):
@@ -249,13 +249,25 @@ class TestMyModule(unittest.TestCase):
 
     @patch('ansible.modules.storage.netapp.na_ontap_net_ifgrp.NetAppOntapIfGrp.remove_port_to_if_grp')
     @patch('ansible.modules.storage.netapp.na_ontap_net_ifgrp.NetAppOntapIfGrp.add_port_to_if_grp')
-    def test_modify_ports_calls(self, add_port, remove_port):
+    def test_modify_ports_calls_remove_existing_ports(self, add_port, remove_port):
+        ''' Test if already existing ports are not being added again '''
         data = self.mock_args()
-        data['ports'] = ['1', '2', '3']
+        data['ports'] = ['1', '2']
+        set_module_args(data)
+        self.get_ifgrp_mock_object('ifgrp').modify_ports(current_ports=['1', '2', '3'])
+        assert remove_port.call_count == 1
+        assert add_port.call_count == 0
+
+    @patch('ansible.modules.storage.netapp.na_ontap_net_ifgrp.NetAppOntapIfGrp.remove_port_to_if_grp')
+    @patch('ansible.modules.storage.netapp.na_ontap_net_ifgrp.NetAppOntapIfGrp.add_port_to_if_grp')
+    def test_modify_ports_calls_add_new_ports(self, add_port, remove_port):
+        ''' Test new ports are added '''
+        data = self.mock_args()
+        data['ports'] = ['1', '2', '3', '4']
         set_module_args(data)
         self.get_ifgrp_mock_object('ifgrp').modify_ports(current_ports=['1', '2'])
-        assert remove_port.call_count == 2
-        assert add_port.call_count == 3
+        assert remove_port.call_count == 0
+        assert add_port.call_count == 2
 
     def test_get_ports_returns_none(self):
         set_module_args(self.mock_args())