add modify to snapshot policy (#59149)

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

@@ -20,11 +20,11 @@ extends_documentation_fragment:
 version_added: '2.8'
 author: NetApp Ansible Team (@carchi8py) <ng-ansibleteam@netapp.com>
 description:
-- Create/Delete ONTAP snapshot policies
+- Create/Modify/Delete ONTAP snapshot policies
 options:
   state:
     description:
-    - If you want to create or delete a snapshot policy.
+    - If you want to create, modify or delete a snapshot policy.
     choices: ['present', 'absent']
     default: present
   name:
@@ -46,41 +46,81 @@ options:
     type: list
   schedule:
     description:
-    - schedule to be added inside the policy.
+    - Schedule to be added inside the policy.
     type: list
+  snapmirror_label:
+    description:
+    - SnapMirror label assigned to each schedule inside the policy. Use an empty
+      string ('') for no label.
+    type: list
+    required: false
+    version_added: '2.9'
+  vserver:
+    description:
+    - The name of the vserver to use. In a multi-tenanted environment, assigning a
+      Snapshot Policy to a vserver will restrict its use to that vserver.
+    required: false
+    version_added: '2.9'
 '''
 EXAMPLES = """
-    - name: create Snapshot policy
+    - name: Create Snapshot policy
       na_ontap_snapshot_policy:
         state: present
         name: ansible2
         schedule: hourly
         count: 150
         enabled: True
-        username: "{{ netapp username }}"
+        username: "{{ netapp_username }}"
-        password: "{{ netapp password }}"
+        password: "{{ netapp_password }}"
-        hostname: "{{ netapp hostname }}"
+        hostname: "{{ netapp_hostname }}"
         https: False
 
     - name: Create Snapshot policy with multiple schedules
       na_ontap_snapshot_policy:
         state: present
         name: ansible2
-        schedule: ['hourly', 'daily', 'weekly', monthly', '5min']
+        schedule: ['hourly', 'daily', 'weekly', 'monthly', '5min']
         count: [1, 2, 3, 4, 5]
         enabled: True
-        username: "{{ netapp username }}"
+        username: "{{ netapp_username }}"
-        password: "{{ netapp password }}"
+        password: "{{ netapp_password }}"
-        hostname: "{{ netapp hostname }}"
+        hostname: "{{ netapp_hostname }}"
         https: False
 
-    - name: delete Snapshot policy
+    - name: Create Snapshot policy owned by a vserver
+      na_ontap_snapshot_policy:
+        state: present
+        name: ansible3
+        vserver: ansible
+        schedule: ['hourly', 'daily', 'weekly', 'monthly', '5min']
+        count: [1, 2, 3, 4, 5]
+        snapmirror_label: ['hourly', 'daily', 'weekly', 'monthly', '']
+        enabled: True
+        username: "{{ netapp_username }}"
+        password: "{{ netapp_password }}"
+        hostname: "{{ netapp_hostname }}"
+        https: False
+
+    - name: Modify Snapshot policy with multiple schedules
+      na_ontap_snapshot_policy:
+        state: present
+        name: ansible2
+        schedule: ['daily', 'weekly']
+        count: [20, 30]
+        snapmirror_label: ['daily', 'weekly']
+        enabled: True
+        username: "{{ netapp_username }}"
+        password: "{{ netapp_password }}"
+        hostname: "{{ netapp_hostname }}"
+        https: False
+
+    - name: Delete Snapshot policy
       na_ontap_snapshot_policy:
         state: absent
         name: ansible2
-        username: "{{ netapp username }}"
+        username: "{{ netapp_username }}"
-        password: "{{ netapp password }}"
+        password: "{{ netapp_password }}"
-        hostname: "{{ netapp hostname }}"
+        hostname: "{{ netapp_hostname }}"
         https: False
 """
 
@@ -111,7 +151,9 @@ class NetAppOntapSnapshotPolicy(object):
             # count is a list of integers
             count=dict(required=False, type="list", elements="int"),
             comment=dict(required=False, type="str"),
-            schedule=dict(required=False, type="list", elements="str")
+            schedule=dict(required=False, type="list", elements="str"),
+            snapmirror_label=dict(required=False, type="list", elements="str"),
+            vserver=dict(required=False, type="str")
         ))
         self.module = AnsibleModule(
             argument_spec=self.argument_spec,
@@ -128,7 +170,10 @@ class NetAppOntapSnapshotPolicy(object):
             self.module.fail_json(
                 msg="the python NetApp-Lib module is required")
         else:
-            self.server = netapp_utils.setup_na_ontap_zapi(module=self.module)
+            if 'vserver' in self.parameters:
+                self.server = netapp_utils.setup_na_ontap_zapi(module=self.module, vserver=self.parameters['vserver'])
+            else:
+                self.server = netapp_utils.setup_na_ontap_zapi(module=self.module)
         return
 
     def get_snapshot_policy(self):
@@ -141,13 +186,30 @@ class NetAppOntapSnapshotPolicy(object):
         query = netapp_utils.zapi.NaElement("query")
         snapshot_info_obj = netapp_utils.zapi.NaElement("snapshot-policy-info")
         snapshot_info_obj.add_new_child("policy", self.parameters['name'])
+        if 'vserver' in self.parameters:
+            snapshot_info_obj.add_new_child("vserver-name", self.parameters['vserver'])
         query.add_child_elem(snapshot_info_obj)
         snapshot_obj.add_child_elem(query)
         try:
             result = self.server.invoke_successfully(snapshot_obj, True)
             if result.get_child_by_name('num-records') and \
                     int(result.get_child_content('num-records')) == 1:
-                return result
+                snapshot_policy = result.get_child_by_name('attributes-list').get_child_by_name('snapshot-policy-info')
+                current = {}
+                current['name'] = snapshot_policy.get_child_content('policy')
+                current['vserver'] = snapshot_policy.get_child_content('vserver-name')
+                current['enabled'] = False if snapshot_policy.get_child_content('enabled').lower() == 'false' else True
+                current['comment'] = snapshot_policy.get_child_content('comment') or ''
+                current['schedule'], current['count'], current['snapmirror_label'] = [], [], []
+                if snapshot_policy.get_child_by_name('snapshot-policy-schedules'):
+                    for schedule in snapshot_policy['snapshot-policy-schedules'].get_children():
+                        current['schedule'].append(schedule.get_child_content('schedule'))
+                        current['count'].append(int(schedule.get_child_content('count')))
+                        snapmirror_label = schedule.get_child_content('snapmirror-label')
+                        if snapmirror_label is None or snapmirror_label == '-':
+                            snapmirror_label = ''
+                        current['snapmirror_label'].append(snapmirror_label)
+                return current
         except netapp_utils.zapi.NaApiError as error:
             self.module.fail_json(msg=to_native(error), exception=traceback.format_exc())
         return None
@@ -157,10 +219,136 @@ class NetAppOntapSnapshotPolicy(object):
         Validate if each schedule has a count associated
         :return: None
         """
-        if len(self.parameters['count']) > 5 or len(self.parameters['schedule']) > 5 or \
+        if 'count' not in self.parameters or 'schedule' not in self.parameters or \
+                len(self.parameters['count']) > 5 or len(self.parameters['schedule']) > 5 or \
+                len(self.parameters['count']) < 1 or len(self.parameters['schedule']) < 1 or \
                 len(self.parameters['count']) != len(self.parameters['schedule']):
-            self.module.fail_json(msg="Error: A Snapshot policy can have up to a maximum of 5 schedules,"
+            self.module.fail_json(msg="Error: A Snapshot policy must have at least 1 "
-                                      "and a count representing maximum number of Snapshot copies for each schedule")
+                                      "schedule and can have up to a maximum of 5 schedules, with a count "
+                                      "representing the maximum number of Snapshot copies for each schedule")
+
+        if 'snapmirror_label' in self.parameters:
+            if len(self.parameters['snapmirror_label']) != len(self.parameters['schedule']):
+                self.module.fail_json(msg="Error: Each Snapshot Policy schedule must have an "
+                                          "accompanying SnapMirror Label")
+
+    def modify_snapshot_policy(self, current):
+        """
+        Modifies an existing snapshot policy
+        """
+        # Set up required variables to modify snapshot policy
+        options = {'policy': self.parameters['name']}
+        modify = False
+
+        # Set up optional variables to modify snapshot policy
+        if 'enabled' in self.parameters and self.parameters['enabled'] != current['enabled']:
+            options['enabled'] = str(self.parameters['enabled'])
+            modify = True
+        if 'comment' in self.parameters and self.parameters['comment'] != current['comment']:
+            options['comment'] = self.parameters['comment']
+            modify = True
+
+        if modify:
+            snapshot_obj = netapp_utils.zapi.NaElement.create_node_with_children('snapshot-policy-modify', **options)
+            try:
+                self.server.invoke_successfully(snapshot_obj, True)
+            except netapp_utils.zapi.NaApiError as error:
+                self.module.fail_json(msg='Error modifying snapshot policy %s: %s' %
+                                      (self.parameters['name'], to_native(error)),
+                                      exception=traceback.format_exc())
+
+    def modify_snapshot_policy_schedules(self, current):
+        """
+        Modify existing schedules in snapshot policy
+        :return: None
+        """
+        self.validate_parameters()
+
+        delete_schedules, modify_schedules, add_schedules = [], [], []
+
+        if 'snapmirror_label' in self.parameters:
+            snapmirror_labels = self.parameters['snapmirror_label']
+        else:
+            # User hasn't supplied any snapmirror labels.
+            snapmirror_labels = [None] * len(self.parameters['schedule'])
+
+        # Identify schedules for deletion
+        for schedule in current['schedule']:
+            schedule = schedule.strip()
+            if schedule not in [item.strip() for item in self.parameters['schedule']]:
+                options = {'policy': current['name'],
+                           'schedule': schedule}
+                delete_schedules.append(options)
+
+        # Identify schedules to be modified or added
+        for schedule, count, snapmirror_label in zip(self.parameters['schedule'], self.parameters['count'], snapmirror_labels):
+            schedule = schedule.strip()
+            if snapmirror_label is not None:
+                snapmirror_label = snapmirror_label.strip()
+
+            options = {'policy': current['name'],
+                       'schedule': schedule}
+
+            if schedule in current['schedule']:
+                # Schedule exists. Only modify if it has changed.
+                modify = False
+                schedule_index = current['schedule'].index(schedule)
+
+                if count != current['count'][schedule_index]:
+                    options['new-count'] = str(count)
+                    modify = True
+
+                if snapmirror_label is not None:
+                    if snapmirror_label != current['snapmirror_label'][schedule_index]:
+                        options['new-snapmirror-label'] = snapmirror_label
+                        modify = True
+
+                if modify:
+                    modify_schedules.append(options)
+            else:
+                # New schedule
+                options['count'] = str(count)
+                if snapmirror_label is not None and snapmirror_label != '':
+                    options['snapmirror-label'] = snapmirror_label
+                add_schedules.append(options)
+
+        # Delete N-1 schedules no longer required. Must leave 1 schedule in policy
+        # at any one time. Delete last one afterwards.
+        while len(delete_schedules) > 1:
+            options = delete_schedules.pop()
+            self.modify_snapshot_policy_schedule(options, 'snapshot-policy-remove-schedule')
+
+        # Modify schedules.
+        while len(modify_schedules) > 0:
+            options = modify_schedules.pop()
+            self.modify_snapshot_policy_schedule(options, 'snapshot-policy-modify-schedule')
+
+        # Add N-1 new schedules. Add last one after last schedule has been deleted.
+        while len(add_schedules) > 1:
+            options = add_schedules.pop()
+            self.modify_snapshot_policy_schedule(options, 'snapshot-policy-add-schedule')
+
+        # Delete last schedule no longer required.
+        while len(delete_schedules) > 0:
+            options = delete_schedules.pop()
+            self.modify_snapshot_policy_schedule(options, 'snapshot-policy-remove-schedule')
+
+        # Add last new schedule.
+        while len(add_schedules) > 0:
+            options = add_schedules.pop()
+            self.modify_snapshot_policy_schedule(options, 'snapshot-policy-add-schedule')
+
+    def modify_snapshot_policy_schedule(self, options, zapi):
+        """
+        Add, modify or remove a schedule to/from a snapshot policy
+        """
+        snapshot_obj = netapp_utils.zapi.NaElement.create_node_with_children(zapi, **options)
+        try:
+            self.server.invoke_successfully(snapshot_obj, enable_tunneling=True)
+        except netapp_utils.zapi.NaApiError as error:
+            self.module.fail_json(msg='Error modifying snapshot policy schedule %s: %s' %
+                                  (self.parameters['name'], to_native(error)),
+                                  exception=traceback.format_exc())
 
     def create_snapshot_policy(self):
         """
@@ -171,11 +359,23 @@ class NetAppOntapSnapshotPolicy(object):
         options = {'policy': self.parameters['name'],
                    'enabled': str(self.parameters['enabled']),
                    }
+
+        if 'snapmirror_label' in self.parameters:
+            snapmirror_labels = self.parameters['snapmirror_label']
+        else:
+            # User hasn't supplied any snapmirror labels.
+            snapmirror_labels = [None] * len(self.parameters['schedule'])
+
         # zapi attribute for first schedule is schedule1, second is schedule2 and so on
         positions = [str(i) for i in range(1, len(self.parameters['schedule']) + 1)]
-        for schedule, count, position in zip(self.parameters['schedule'], self.parameters['count'], positions):
+        for schedule, count, snapmirror_label, position in zip(self.parameters['schedule'], self.parameters['count'], snapmirror_labels, positions):
+            schedule = schedule.strip()
             options['count' + position] = str(count)
             options['schedule' + position] = schedule
+            if snapmirror_label is not None:
+                snapmirror_label = snapmirror_label.strip()
+                if snapmirror_label != '':
+                    options['snapmirror-label' + position] = snapmirror_label
         snapshot_obj = netapp_utils.zapi.NaElement.create_node_with_children('snapshot-policy-create', **options)
 
         # Set up optional variables to create a snapshot policy
@@ -220,7 +420,13 @@ class NetAppOntapSnapshotPolicy(object):
         """
         self.asup_log_for_cserver("na_ontap_snapshot_policy")
         current = self.get_snapshot_policy()
+        modify = None
         cd_action = self.na_helper.get_cd_action(current, self.parameters)
+        if cd_action is None and self.parameters['state'] == 'present':
+            # Don't sort schedule/count/snapmirror_label lists as it can
+            # mess up the intended parameter order.
+            modify = self.na_helper.get_modified_attributes(current, self.parameters)
+
         if self.na_helper.changed:
             if self.module.check_mode:
                 pass
@@ -229,6 +435,9 @@ class NetAppOntapSnapshotPolicy(object):
                     self.create_snapshot_policy()
                 elif cd_action == 'delete':
                     self.delete_snapshot_policy()
+                if modify:
+                    self.modify_snapshot_policy(current)
+                    self.modify_snapshot_policy_schedules(current)
         self.module.exit_json(changed=self.na_helper.changed)
 
 

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

@@ -63,6 +63,16 @@ class MockONTAPConnection(object):
         self.xml_in = xml
         if self.type == 'policy':
             xml = self.build_snapshot_policy_info()
+        elif self.type == 'snapshot_policy_info_policy_disabled':
+            xml = self.build_snapshot_policy_info_policy_disabled()
+        elif self.type == 'snapshot_policy_info_comment_modified':
+            xml = self.build_snapshot_policy_info_comment_modified()
+        elif self.type == 'snapshot_policy_info_schedules_added':
+            xml = self.build_snapshot_policy_info_schedules_added()
+        elif self.type == 'snapshot_policy_info_schedules_deleted':
+            xml = self.build_snapshot_policy_info_schedules_deleted()
+        elif self.type == 'snapshot_policy_info_modified_schedule_counts':
+            xml = self.build_snapshot_policy_info_modified_schedule_counts()
         elif self.type == 'policy_fail':
             raise netapp_utils.zapi.NaApiError(code='TEST', message="This exception is from the unit test")
         self.xml_out = xml
@@ -77,7 +87,170 @@ class MockONTAPConnection(object):
         ''' build xml data for snapshot-policy-info '''
         xml = netapp_utils.zapi.NaElement('xml')
         data = {'num-records': 1,
-                'attributes-list': {'snapshot-policy-info': {'policy': 'ansible'}}}
+                'attributes-list': {
+                    'snapshot-policy-info': {
+                        'comment': 'new comment',
+                        'enabled': 'true',
+                        'policy': 'ansible',
+                        'snapshot-policy-schedules': {
+                            'snapshot-schedule-info': {
+                                'count': 100,
+                                'schedule': 'hourly',
+                                'snapmirror-label': ''
+                            }
+                        },
+                        'vserver-name': 'hostname'
+                    }
+                }}
+        xml.translate_struct(data)
+        return xml
+
+    @staticmethod
+    def build_snapshot_policy_info_comment_modified():
+        ''' build xml data for snapshot-policy-info '''
+        xml = netapp_utils.zapi.NaElement('xml')
+        data = {'num-records': 1,
+                'attributes-list': {
+                    'snapshot-policy-info': {
+                        'comment': 'modified comment',
+                        'enabled': 'true',
+                        'policy': 'ansible',
+                        'snapshot-policy-schedules': {
+                            'snapshot-schedule-info': {
+                                'count': 100,
+                                'schedule': 'hourly',
+                                'snapmirror-label': ''
+                            }
+                        },
+                        'vserver-name': 'hostname'
+                    }
+                }}
+        xml.translate_struct(data)
+        return xml
+
+    @staticmethod
+    def build_snapshot_policy_info_policy_disabled():
+        ''' build xml data for snapshot-policy-info '''
+        xml = netapp_utils.zapi.NaElement('xml')
+        data = {'num-records': 1,
+                'attributes-list': {
+                    'snapshot-policy-info': {
+                        'comment': 'new comment',
+                        'enabled': 'false',
+                        'policy': 'ansible',
+                        'snapshot-policy-schedules': {
+                            'snapshot-schedule-info': {
+                                'count': 100,
+                                'schedule': 'hourly',
+                                'snapmirror-label': ''
+                            }
+                        },
+                        'vserver-name': 'hostname'
+                    }
+                }}
+        xml.translate_struct(data)
+        return xml
+
+    @staticmethod
+    def build_snapshot_policy_info_schedules_added():
+        ''' build xml data for snapshot-policy-info '''
+        xml = netapp_utils.zapi.NaElement('xml')
+        data = {'num-records': 1,
+                'attributes-list': {
+                    'snapshot-policy-info': {
+                        'comment': 'new comment',
+                        'enabled': 'true',
+                        'policy': 'ansible',
+                        'snapshot-policy-schedules': [
+                            {
+                                'snapshot-schedule-info': {
+                                    'count': 100,
+                                    'schedule': 'hourly',
+                                    'snapmirror-label': ''
+                                }
+                            },
+                            {
+                                'snapshot-schedule-info': {
+                                    'count': 5,
+                                    'schedule': 'daily',
+                                    'snapmirror-label': 'daily'
+                                }
+                            },
+                            {
+                                'snapshot-schedule-info': {
+                                    'count': 10,
+                                    'schedule': 'weekly',
+                                    'snapmirror-label': ''
+                                }
+                            }
+                        ],
+                        'vserver-name': 'hostname'
+                    }
+                }}
+        xml.translate_struct(data)
+        return xml
+
+    @staticmethod
+    def build_snapshot_policy_info_schedules_deleted():
+        ''' build xml data for snapshot-policy-info '''
+        xml = netapp_utils.zapi.NaElement('xml')
+        data = {'num-records': 1,
+                'attributes-list': {
+                    'snapshot-policy-info': {
+                        'comment': 'new comment',
+                        'enabled': 'true',
+                        'policy': 'ansible',
+                        'snapshot-policy-schedules': [
+                            {
+                                'snapshot-schedule-info': {
+                                    'schedule': 'daily',
+                                    'count': 5,
+                                    'snapmirror-label': 'daily'
+                                }
+                            }
+                        ],
+                        'vserver-name': 'hostname'
+                    }
+                }}
+        xml.translate_struct(data)
+        return xml
+
+    @staticmethod
+    def build_snapshot_policy_info_modified_schedule_counts():
+        ''' build xml data for snapshot-policy-info '''
+        xml = netapp_utils.zapi.NaElement('xml')
+        data = {'num-records': 1,
+                'attributes-list': {
+                    'snapshot-policy-info': {
+                        'comment': 'new comment',
+                        'enabled': 'true',
+                        'policy': 'ansible',
+                        'snapshot-policy-schedules': [
+                            {
+                                'snapshot-schedule-info': {
+                                    'count': 10,
+                                    'schedule': 'hourly',
+                                    'snapmirror-label': ''
+                                }
+                            },
+                            {
+                                'snapshot-schedule-info': {
+                                    'count': 50,
+                                    'schedule': 'daily',
+                                    'snapmirror-label': 'daily'
+                                }
+                            },
+                            {
+                                'snapshot-schedule-info': {
+                                    'count': 100,
+                                    'schedule': 'weekly',
+                                    'snapmirror-label': ''
+                                }
+                            }
+                        ],
+                        'vserver-name': 'hostname'
+                    }
+                }}
         xml.translate_struct(data)
         return xml
 
@@ -124,6 +297,18 @@ class TestMyModule(unittest.TestCase):
             'comment': comment
         })
 
+    def set_default_current(self):
+        default_args = self.set_default_args()
+        return dict({
+            'name': default_args['name'],
+            'enabled': default_args['enabled'],
+            'count': [default_args['count']],
+            'schedule': [default_args['schedule']],
+            'snapmirror_label': [''],
+            'comment': default_args['comment'],
+            'vserver': default_args['hostname']
+        })
+
     def test_module_fail_when_required_args_missing(self):
         ''' required arguments are reported as errors '''
         with pytest.raises(AnsibleFailJson) as exc:
@@ -166,20 +351,154 @@ class TestMyModule(unittest.TestCase):
             my_obj.apply()
         assert not exc.value.args[0]['changed']
 
-    def test_validate_params(self):
+    @patch('ansible.modules.storage.netapp.na_ontap_snapshot_policy.NetAppOntapSnapshotPolicy.modify_snapshot_policy')
+    def test_successful_modify_comment(self, modify_snapshot):
+        ''' modifying snapshot policy comment and testing idempotency '''
         data = self.set_default_args()
-        data['schedule'] = ['s1', 's2']
+        data['comment'] = 'modified comment'
-        data['count'] = [1, 2, 3]
         set_module_args(data)
         my_obj = my_module()
         my_obj.asup_log_for_cserver = Mock(return_value=None)
         if not self.onbox:
-            my_obj.server = self.server
+            my_obj.server = MockONTAPConnection('policy')
-        with pytest.raises(AnsibleFailJson) as exc:
+        with pytest.raises(AnsibleExitJson) as exc:
-            my_obj.create_snapshot_policy()
+            my_obj.apply()
-        msg = 'Error: A Snapshot policy can have up to a maximum of 5 schedules,and a ' \
+        assert exc.value.args[0]['changed']
-              'count representing maximum number of Snapshot copies for each schedule'
+        current = self.set_default_current()
-        assert exc.value.args[0]['msg'] == msg
+        modify_snapshot.assert_called_with(current)
+        # to reset na_helper from remembering the previous 'changed' value
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('snapshot_policy_info_comment_modified')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert not exc.value.args[0]['changed']
+
+    @patch('ansible.modules.storage.netapp.na_ontap_snapshot_policy.NetAppOntapSnapshotPolicy.modify_snapshot_policy')
+    def test_successful_disable_policy(self, modify_snapshot):
+        ''' disabling snapshot policy and testing idempotency '''
+        data = self.set_default_args()
+        data['enabled'] = False
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('policy')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert exc.value.args[0]['changed']
+        current = self.set_default_current()
+        modify_snapshot.assert_called_with(current)
+        # to reset na_helper from remembering the previous 'changed' value
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('snapshot_policy_info_policy_disabled')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert not exc.value.args[0]['changed']
+
+    @patch('ansible.modules.storage.netapp.na_ontap_snapshot_policy.NetAppOntapSnapshotPolicy.modify_snapshot_policy')
+    def test_successful_enable_policy(self, modify_snapshot):
+        ''' enabling snapshot policy and testing idempotency '''
+        data = self.set_default_args()
+        data['enabled'] = True
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('snapshot_policy_info_policy_disabled')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert exc.value.args[0]['changed']
+        current = self.set_default_current()
+        current['enabled'] = False
+        modify_snapshot.assert_called_with(current)
+        # to reset na_helper from remembering the previous 'changed' value
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('policy')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert not exc.value.args[0]['changed']
+
+    @patch('ansible.modules.storage.netapp.na_ontap_snapshot_policy.NetAppOntapSnapshotPolicy.modify_snapshot_policy')
+    def test_successful_modify_schedules_add(self, modify_snapshot):
+        ''' adding snapshot policy schedules and testing idempotency '''
+        data = self.set_default_args()
+        data['schedule'] = ['hourly', 'daily', 'weekly']
+        data['count'] = [100, 5, 10]
+        data['snapmirror_label'] = ['', 'daily', '']
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('policy')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert exc.value.args[0]['changed']
+        current = self.set_default_current()
+        modify_snapshot.assert_called_with(current)
+        # to reset na_helper from remembering the previous 'changed' value
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('snapshot_policy_info_schedules_added')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert not exc.value.args[0]['changed']
+
+    @patch('ansible.modules.storage.netapp.na_ontap_snapshot_policy.NetAppOntapSnapshotPolicy.modify_snapshot_policy')
+    def test_successful_modify_schedules_delete(self, modify_snapshot):
+        ''' deleting snapshot policy schedules and testing idempotency '''
+        data = self.set_default_args()
+        data['schedule'] = ['daily']
+        data['count'] = [5]
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('policy')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert exc.value.args[0]['changed']
+        current = self.set_default_current()
+        modify_snapshot.assert_called_with(current)
+        # to reset na_helper from remembering the previous 'changed' value
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('snapshot_policy_info_schedules_deleted')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert not exc.value.args[0]['changed']
+
+    @patch('ansible.modules.storage.netapp.na_ontap_snapshot_policy.NetAppOntapSnapshotPolicy.modify_snapshot_policy')
+    def test_successful_modify_schedules(self, modify_snapshot):
+        ''' modifying snapshot policy schedule counts and testing idempotency '''
+        data = self.set_default_args()
+        data['schedule'] = ['hourly', 'daily', 'weekly']
+        data['count'] = [10, 50, 100]
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('policy')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert exc.value.args[0]['changed']
+        current = self.set_default_current()
+        modify_snapshot.assert_called_with(current)
+        # to reset na_helper from remembering the previous 'changed' value
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = MockONTAPConnection('snapshot_policy_info_modified_schedule_counts')
+        with pytest.raises(AnsibleExitJson) as exc:
+            my_obj.apply()
+        assert not exc.value.args[0]['changed']
 
     @patch('ansible.modules.storage.netapp.na_ontap_snapshot_policy.NetAppOntapSnapshotPolicy.delete_snapshot_policy')
     def test_successful_delete(self, delete_snapshot):
@@ -205,7 +524,7 @@ class TestMyModule(unittest.TestCase):
         assert not exc.value.args[0]['changed']
 
     def test_valid_schedule_count(self):
-        ''' validate error when schedule has more than 5 elements '''
+        ''' validate when schedule has same number of elements '''
         data = self.set_default_args()
         data['schedule'] = ['hourly', 'daily', 'weekly', 'monthly', '5min']
         data['count'] = [1, 2, 3, 4, 5]
@@ -219,6 +538,40 @@ class TestMyModule(unittest.TestCase):
         assert data['count'][2] == int(create_xml['count3'])
         assert data['schedule'][4] == create_xml['schedule5']
 
+    def test_valid_schedule_count_with_snapmirror_labels(self):
+        ''' validate when schedule has same number of elements with snapmirror labels '''
+        data = self.set_default_args()
+        data['schedule'] = ['hourly', 'daily', 'weekly', 'monthly', '5min']
+        data['count'] = [1, 2, 3, 4, 5]
+        data['snapmirror_label'] = ['hourly', 'daily', 'weekly', 'monthly', '5min']
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = self.server
+        my_obj.create_snapshot_policy()
+        create_xml = my_obj.server.xml_in
+        assert data['count'][2] == int(create_xml['count3'])
+        assert data['schedule'][4] == create_xml['schedule5']
+        assert data['snapmirror_label'][3] == create_xml['snapmirror-label4']
+
+    def test_invalid_params(self):
+        ''' validate error when schedule does not have same number of elements '''
+        data = self.set_default_args()
+        data['schedule'] = ['s1', 's2']
+        data['count'] = [1, 2, 3]
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = self.server
+        with pytest.raises(AnsibleFailJson) as exc:
+            my_obj.create_snapshot_policy()
+        msg = 'Error: A Snapshot policy must have at least 1 ' \
+              'schedule and can have up to a maximum of 5 schedules, with a count ' \
+              'representing the maximum number of Snapshot copies for each schedule'
+        assert exc.value.args[0]['msg'] == msg
+
     def test_invalid_schedule_count(self):
         ''' validate error when schedule has more than 5 elements '''
         data = self.set_default_args()
@@ -231,8 +584,59 @@ class TestMyModule(unittest.TestCase):
             my_obj.server = self.server
         with pytest.raises(AnsibleFailJson) as exc:
             my_obj.create_snapshot_policy()
-        msg = 'Error: A Snapshot policy can have up to a maximum of 5 schedules,and a ' \
+        msg = 'Error: A Snapshot policy must have at least 1 ' \
-              'count representing maximum number of Snapshot copies for each schedule'
+              'schedule and can have up to a maximum of 5 schedules, with a count ' \
+              'representing the maximum number of Snapshot copies for each schedule'
+        assert exc.value.args[0]['msg'] == msg
+
+    def test_invalid_schedule_count_less_than_one(self):
+        ''' validate error when schedule has less than 1 element '''
+        data = self.set_default_args()
+        data['schedule'] = []
+        data['count'] = []
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = self.server
+        with pytest.raises(AnsibleFailJson) as exc:
+            my_obj.create_snapshot_policy()
+        msg = 'Error: A Snapshot policy must have at least 1 ' \
+              'schedule and can have up to a maximum of 5 schedules, with a count ' \
+              'representing the maximum number of Snapshot copies for each schedule'
+        assert exc.value.args[0]['msg'] == msg
+
+    def test_invalid_schedule_count_is_none(self):
+        ''' validate error when schedule is None '''
+        data = self.set_default_args()
+        data['schedule'] = None
+        data['count'] = None
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = self.server
+        with pytest.raises(AnsibleFailJson) as exc:
+            my_obj.create_snapshot_policy()
+        msg = 'Error: A Snapshot policy must have at least 1 ' \
+              'schedule and can have up to a maximum of 5 schedules, with a count ' \
+              'representing the maximum number of Snapshot copies for each schedule'
+        assert exc.value.args[0]['msg'] == msg
+
+    def test_invalid_schedule_count_with_snapmirror_labels(self):
+        ''' validate error when schedule with snapmirror labels does not have same number of elements '''
+        data = self.set_default_args()
+        data['schedule'] = ['s1', 's2', 's3']
+        data['count'] = [1, 2, 3]
+        data['snapmirror_label'] = ['sm1', 'sm2']
+        set_module_args(data)
+        my_obj = my_module()
+        my_obj.asup_log_for_cserver = Mock(return_value=None)
+        if not self.onbox:
+            my_obj.server = self.server
+        with pytest.raises(AnsibleFailJson) as exc:
+            my_obj.create_snapshot_policy()
+        msg = 'Error: Each Snapshot Policy schedule must have an accompanying SnapMirror Label'
         assert exc.value.args[0]['msg'] == msg
 
     def test_if_all_methods_catch_exception(self):