sysctl - Reload also when current system values differ (#56153)

Previously if `sysctl_set=no` (which is the default) this module only
checked for changes in the sysctl.conf file to decide whether it should
reload it or not. This means that if the values in the conf file are the
same as they are set with the module, but the current values on the
system are different, that this module wouldn't apply the changes on the
system and thus the value set with the module wouldn't be applied on the
OS. This isn't obvious and it doesn't make sense that the module works
like that by default, especially because there is a separate option
`reload`. Now sysctl will also check if the current value differs on the
system and if it does, it will reload the file again.

changelogs/fragments/sysctl-check-file-and-system.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+  - sysctl - check system values, not just sysctl.conf file, when determing state (https://github.com/ansible/ansible/pull/56153#issuecomment-514384922)

lib/ansible/modules/system/sysctl.py

@@ -169,9 +169,16 @@ class SysctlModule(object):
         elif self.file_values[thisname] != self.args['value']:
             self.changed = True
             self.write_file = True
+        # with reload=yes we should check if the current system values are
+        # correct, so that we know if we should reload
+        elif self.args['reload']:
+            if self.proc_value is None:
+                self.changed = True
+            elif not self._values_is_equal(self.proc_value, self.args['value']):
+                self.changed = True
 
         # use the sysctl command or not?
-        if self.args['sysctl_set']:
+        if self.args['sysctl_set'] and self.args['state'] == "present":
             if self.proc_value is None:
                 self.changed = True
             elif not self._values_is_equal(self.proc_value, self.args['value']):
@@ -182,7 +189,7 @@ class SysctlModule(object):
         if not self.module.check_mode:
             if self.write_file:
                 self.write_sysctl()
-            if self.write_file and self.args['reload']:
+            if self.changed and self.args['reload']:
                 self.reload_sysctl()
             if self.set_proc:
                 self.set_token_value(self.args['name'], self.args['value'])

test/integration/targets/sysctl/tasks/main.yml

@@ -20,191 +20,272 @@
 # apply sysctl, or it will always fail, because of that in most cases (except
 # those when it should fail) we have to use `reload=no`.
 
-- set_fact:
+- name: Test inside Docker
-    output_dir_test: "{{ output_dir }}/test_sysctl"
+  when:
-
+    - ansible_facts.virtualization_type == 'docker'
-- name: make sure our testing sub-directory does not exist
+  block:
-  file:
+    - set_fact:
-    path: "{{ output_dir_test }}"
+        output_dir_test: "{{ output_dir }}/test_sysctl"
-    state: absent
+
-
+    - name: make sure our testing sub-directory does not exist
-- name: create our testing sub-directory
+      file:
-  file:
+        path: "{{ output_dir_test }}"
-    path: "{{ output_dir_test }}"
+        state: absent
-    state: directory
+
-
+    - name: create our testing sub-directory
-##
+      file:
-## sysctl - file manipulation
+        path: "{{ output_dir_test }}"
-##
+        state: directory
-
+
-- name: copy the example conf to the test dir
+    ##
-  copy:
+    ## sysctl - file manipulation
-    src: sysctl.conf
+    ##
-    dest: "{{ output_dir_test }}"
+
-
+    - name: copy the example conf to the test dir
-- name: Set vm.swappiness to 5
+      copy:
-  sysctl:
+        src: sysctl.conf
-    name: vm.swappiness
+        dest: "{{ output_dir_test }}"
-    value: 5
+
-    state: present
+    - name: Set vm.swappiness to 5
-    reload: no
+      sysctl:
-    sysctl_file: "{{ output_dir_test }}/sysctl.conf"
+        name: vm.swappiness
-  register: sysctl_test0
+        value: 5
-
+        state: present
-- debug:
+        reload: no
-    var: sysctl_test0
+        sysctl_file: "{{ output_dir_test }}/sysctl.conf"
-    verbosity: 1
+      register: sysctl_test0
-
+
-- name: get file content
+    - debug:
-  shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#"
+        var: sysctl_test0
-  register: sysctl_content0
+        verbosity: 1
-
+
-- debug:
+    - name: get file content
-    var: sysctl_content0
+      shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#"
-    verbosity: 1
+      register: sysctl_content0
-
+
-- name: Set vm.swappiness to 5 again
+    - debug:
-  sysctl:
+        var: sysctl_content0
-      name: vm.swappiness
+        verbosity: 1
-      value: 5
+
-      state: present
+    - name: Set vm.swappiness to 5 again
-      reload: no
+      sysctl:
-      sysctl_file: "{{ output_dir_test }}/sysctl.conf"
+          name: vm.swappiness
-  register: sysctl_test1
+          value: 5
-
+          state: present
-- name: validate results
+          reload: no
-  assert:
+          sysctl_file: "{{ output_dir_test }}/sysctl.conf"
-      that:
+      register: sysctl_test1
-        - sysctl_test0 is changed
+
-        - sysctl_test1 is not changed
+    - name: validate results
-        - 'sysctl_content0.stdout_lines[sysctl_content0.stdout_lines.index("vm.swappiness=5")] == "vm.swappiness=5"'
+      assert:
-
+          that:
-- name: Remove kernel.panic
+            - sysctl_test0 is changed
-  sysctl:
+            - sysctl_test1 is not changed
-    name: kernel.panic
+            - 'sysctl_content0.stdout_lines[sysctl_content0.stdout_lines.index("vm.swappiness=5")] == "vm.swappiness=5"'
-    value: 2
+
-    reload: no
+    - name: Remove kernel.panic
-    state: absent
+      sysctl:
-    sysctl_file: "{{ output_dir_test }}/sysctl.conf"
+        name: kernel.panic
-  register: sysctl_test2
+        value: 2
-
+        reload: no
-- name: get file content
+        state: absent
-  shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#"
+        sysctl_file: "{{ output_dir_test }}/sysctl.conf"
-  register: sysctl_content2
+      register: sysctl_test2
-
+
-- debug:
+    - name: get file content
-    var: item
+      shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#"
-    verbosity: 1
+      register: sysctl_content2
-  with_items:
+
-    - "{{ sysctl_test2 }}"
+    - debug:
-    - "{{ sysctl_content2 }}"
+        var: item
-
+        verbosity: 1
-- name: Validate results for key removal
+      with_items:
-  assert:
+        - "{{ sysctl_test2 }}"
-    that:
+        - "{{ sysctl_content2 }}"
-      - sysctl_test2 is changed
+
-      - "'kernel.panic' not in sysctl_content2.stdout_lines"
+    - name: Validate results for key removal
-
+      assert:
-- name: Test remove kernel.panic again
+        that:
-  sysctl:
+          - sysctl_test2 is changed
-    name: kernel.panic
+          - "'kernel.panic' not in sysctl_content2.stdout_lines"
-    value: 2
+
-    state: absent
+    - name: Test remove kernel.panic again
-    reload: no
+      sysctl:
-    sysctl_file: "{{ output_dir_test }}/sysctl.conf"
+        name: kernel.panic
-  register: sysctl_test2_change_test
+        value: 2
-
+        state: absent
-- name: Assert that no change was made
+        reload: no
-  assert:
+        sysctl_file: "{{ output_dir_test }}/sysctl.conf"
-    that:
+      register: sysctl_test2_change_test
-      - sysctl_test2_change_test is not changed
+
-
+    - name: Assert that no change was made
-- name: Try sysctl with an invalid value
+      assert:
-  sysctl:
+        that:
-    name: net.ipv4.ip_forward
+          - sysctl_test2_change_test is not changed
-    value: foo
+
-  register: sysctl_test3
+    - name: Try sysctl with an invalid value
-  ignore_errors: yes
+      sysctl:
-
+        name: net.ipv4.ip_forward
-- debug:
+        value: foo
-    var: sysctl_test3
+      register: sysctl_test3
-    verbosity: 1
+      ignore_errors: yes
-
+
-- name: validate results for test 3
+    - debug:
-  assert:
+        var: sysctl_test3
-    that:
+        verbosity: 1
-      - sysctl_test3 is failed
+
-
+    - name: validate results for test 3
-##
+      assert:
-## sysctl - sysctl_set
+        that:
-##
+          - sysctl_test3 is failed
-
+
-- name: set net.ipv4.ip_forward
+    ##
-  sysctl:
+    ## sysctl - sysctl_set
-    name: net.ipv4.ip_forward
+    ##
-    value: 1
+
-    sysctl_set: yes
+    - name: set net.ipv4.ip_forward
-    reload: no
+      sysctl:
-  register: sysctl_test3
+        name: net.ipv4.ip_forward
-
+        value: 1
-- name: check with sysctl command
+        sysctl_set: yes
-  shell: sysctl net.ipv4.ip_forward
+        reload: no
-  register: sysctl_check3
+      register: sysctl_test3
-
+
-- debug:
+    - name: check with sysctl command
-    var: item
+      shell: sysctl net.ipv4.ip_forward
-    verbosity: 1
+      register: sysctl_check3
-  with_items:
+
-    - "{{ sysctl_test3 }}"
+    - debug:
-    - "{{ sysctl_check3 }}"
+        var: item
-
+        verbosity: 1
-- name: validate results for test 3
+      with_items:
-  assert:
+        - "{{ sysctl_test3 }}"
-    that:
+        - "{{ sysctl_check3 }}"
-      - sysctl_test3 is changed
+
-      - 'sysctl_check3.stdout_lines == ["net.ipv4.ip_forward = 1"]'
+    - name: validate results for test 3
-
+      assert:
-- name: Try sysctl with no name
+        that:
-  sysctl:
+          - sysctl_test3 is changed
-    name:
+          - 'sysctl_check3.stdout_lines == ["net.ipv4.ip_forward = 1"]'
-    value: 1
+
-    sysctl_set: yes
+    - name: Try sysctl with no name
-  ignore_errors: True
+      sysctl:
-  register: sysctl_no_name
+        name:
-
+        value: 1
-- name: validate nameless results
+        sysctl_set: yes
-  assert:
+      ignore_errors: True
-    that:
+      register: sysctl_no_name
-      - sysctl_no_name is failed
+
-      - "sysctl_no_name.msg == 'name cannot be None'"
+    - name: validate nameless results
-
+      assert:
-- name: Try sysctl with no value
+        that:
-  sysctl:
+          - sysctl_no_name is failed
-    name: Foo
+          - "sysctl_no_name.msg == 'name cannot be None'"
-    value:
+
-    sysctl_set: yes
+    - name: Try sysctl with no value
-  ignore_errors: True
+      sysctl:
-  register: sysctl_no_value
+        name: Foo
-
+        value:
-- name: validate nameless results
+        sysctl_set: yes
-  assert:
+      ignore_errors: True
-    that:
+      register: sysctl_no_value
-      - sysctl_no_value is failed
+
-      - "sysctl_no_value.msg == 'value cannot be None'"
+    - name: validate nameless results
-
+      assert:
-- name: Try sysctl with an invalid value
+        that:
-  sysctl:
+          - sysctl_no_value is failed
-    name: net.ipv4.ip_forward
+          - "sysctl_no_value.msg == 'value cannot be None'"
-    value: foo
+
-    sysctl_set: yes
+    - name: Try sysctl with an invalid value
-  register: sysctl_test4
+      sysctl:
-  ignore_errors: yes
+        name: net.ipv4.ip_forward
-
+        value: foo
-- debug:
+        sysctl_set: yes
-    var: sysctl_test4
+      register: sysctl_test4
-    verbosity: 1
+      ignore_errors: yes
-
+
-- name: validate results for test 4
+    - debug:
-  assert:
+        var: sysctl_test4
-    that:
+        verbosity: 1
-      - sysctl_test4 is failed
+
+    - name: validate results for test 4
+      assert:
+        that:
+          - sysctl_test4 is failed
+
+
+- name: Test on RHEL VMs
+  when:
+    - ansible_facts.virtualization_type != 'docker'
+    - ansible_facts.distribution == 'RedHat'
+  block:
+    # Test reload: yes
+    - name: Set sysctl property using module
+      sysctl:
+        name: vm.swappiness
+        value: '22'
+        state: present
+        reload: yes
+      register: sysctl_set1
+
+    - name: Change sysctl property using command
+      command: sysctl vm.swappiness=33
+
+    - name: Set sysctl property using module
+      sysctl:
+        name: vm.swappiness
+        value: '22'
+        state: present
+        reload: yes
+      register: sysctl_set2
+
+    - name: Read /etc/sysctl.conf
+      command: 'egrep -v ^# /etc/sysctl.conf'
+      register: sysctl_conf_content
+
+    - name: Get current value of vm.swappiness
+      command: sysctl -n vm.swappiness
+      register: sysctl_current_vm_swappiness
+
+    - name: Ensure changes were made appropriately
+      assert:
+        that:
+          - sysctl_set1 is changed
+          - sysctl_set2 is changed
+          - "'vm.swappiness=22' in sysctl_conf_content.stdout_lines"
+          - sysctl_current_vm_swappiness.stdout == '22'
+
+    # Test reload: yes in check mode
+    - name: Set the same value using module in check mode
+      sysctl:
+        name: vm.swappiness
+        value: '22'
+        state: present
+        reload: yes
+      check_mode: yes
+      register: sysctl_check_mode1
+
+    - name: Set a different value using module in check mode
+      sysctl:
+        name: vm.swappiness
+        value: '44'
+        state: present
+        reload: yes
+      check_mode: yes
+      register: sysctl_check_mode2
+
+    - name: Read /etc/sysctl.conf
+      command: 'egrep -v ^# /etc/sysctl.conf'
+      register: sysctl_check_mode_conf_content
+
+    - name: Get current value of vm.swappiness
+      command: sysctl -n vm.swappiness
+      register: sysctl_check_mode_current_vm_swappiness
+
+    - name: Ensure no changes were made in check mode
+      assert:
+        that:
+          - sysctl_check_mode1 is success
+          - sysctl_check_mode2 is changed
+          - "'vm.swappiness=22' in sysctl_check_mode_conf_content.stdout_lines"
+          - sysctl_check_mode_current_vm_swappiness.stdout == '22'