From 5fc769f6b1535942f53f3ef877b9343433419f99 Mon Sep 17 00:00:00 2001 From: Strahinja Kustudic Date: Wed, 24 Jul 2019 16:04:15 +0200 Subject: [PATCH] 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. --- .../sysctl-check-file-and-system.yaml | 2 + lib/ansible/modules/system/sysctl.py | 11 +- .../integration/targets/sysctl/tasks/main.yml | 395 +++++++++++------- 3 files changed, 249 insertions(+), 159 deletions(-) create mode 100644 changelogs/fragments/sysctl-check-file-and-system.yaml diff --git a/changelogs/fragments/sysctl-check-file-and-system.yaml b/changelogs/fragments/sysctl-check-file-and-system.yaml new file mode 100644 index 00000000000..8bf33ed4934 --- /dev/null +++ b/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) diff --git a/lib/ansible/modules/system/sysctl.py b/lib/ansible/modules/system/sysctl.py index 34b1d00ecee..443367dca4c 100644 --- a/lib/ansible/modules/system/sysctl.py +++ b/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']) diff --git a/test/integration/targets/sysctl/tasks/main.yml b/test/integration/targets/sysctl/tasks/main.yml index e23b2ccee66..d72fd524482 100644 --- a/test/integration/targets/sysctl/tasks/main.yml +++ b/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: - output_dir_test: "{{ output_dir }}/test_sysctl" +- name: Test inside Docker + when: + - ansible_facts.virtualization_type == 'docker' + block: + - set_fact: + output_dir_test: "{{ output_dir }}/test_sysctl" -- name: make sure our testing sub-directory does not exist - file: - path: "{{ output_dir_test }}" - state: absent + - name: make sure our testing sub-directory does not exist + file: + path: "{{ output_dir_test }}" + state: absent -- name: create our testing sub-directory - file: - path: "{{ output_dir_test }}" - state: directory + - name: create our testing sub-directory + file: + path: "{{ output_dir_test }}" + state: directory -## -## sysctl - file manipulation -## + ## + ## sysctl - file manipulation + ## -- name: copy the example conf to the test dir - copy: - src: sysctl.conf - dest: "{{ output_dir_test }}" + - name: copy the example conf to the test dir + copy: + src: sysctl.conf + dest: "{{ output_dir_test }}" -- name: Set vm.swappiness to 5 - sysctl: - name: vm.swappiness - value: 5 - state: present - reload: no - sysctl_file: "{{ output_dir_test }}/sysctl.conf" - register: sysctl_test0 + - name: Set vm.swappiness to 5 + sysctl: + name: vm.swappiness + value: 5 + state: present + reload: no + sysctl_file: "{{ output_dir_test }}/sysctl.conf" + register: sysctl_test0 -- debug: - var: sysctl_test0 - verbosity: 1 + - debug: + var: sysctl_test0 + verbosity: 1 -- name: get file content - shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#" - register: sysctl_content0 + - name: get file content + shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#" + register: sysctl_content0 -- debug: - var: sysctl_content0 - verbosity: 1 + - debug: + var: sysctl_content0 + verbosity: 1 -- name: Set vm.swappiness to 5 again - sysctl: - name: vm.swappiness - value: 5 - state: present - reload: no - sysctl_file: "{{ output_dir_test }}/sysctl.conf" - register: sysctl_test1 + - name: Set vm.swappiness to 5 again + sysctl: + name: vm.swappiness + value: 5 + state: present + reload: no + sysctl_file: "{{ output_dir_test }}/sysctl.conf" + register: sysctl_test1 -- name: validate results - assert: - that: - - sysctl_test0 is changed - - sysctl_test1 is not changed - - 'sysctl_content0.stdout_lines[sysctl_content0.stdout_lines.index("vm.swappiness=5")] == "vm.swappiness=5"' + - name: validate results + assert: + that: + - sysctl_test0 is changed + - sysctl_test1 is not changed + - 'sysctl_content0.stdout_lines[sysctl_content0.stdout_lines.index("vm.swappiness=5")] == "vm.swappiness=5"' -- name: Remove kernel.panic - sysctl: - name: kernel.panic - value: 2 - reload: no - state: absent - sysctl_file: "{{ output_dir_test }}/sysctl.conf" - register: sysctl_test2 + - name: Remove kernel.panic + sysctl: + name: kernel.panic + value: 2 + reload: no + state: absent + sysctl_file: "{{ output_dir_test }}/sysctl.conf" + register: sysctl_test2 -- name: get file content - shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#" - register: sysctl_content2 + - name: get file content + shell: "cat {{ output_dir_test }}/sysctl.conf | egrep -v ^\\#" + register: sysctl_content2 -- debug: - var: item - verbosity: 1 - with_items: - - "{{ sysctl_test2 }}" - - "{{ sysctl_content2 }}" + - debug: + var: item + verbosity: 1 + with_items: + - "{{ sysctl_test2 }}" + - "{{ sysctl_content2 }}" -- name: Validate results for key removal - assert: - that: - - sysctl_test2 is changed - - "'kernel.panic' not in sysctl_content2.stdout_lines" + - name: Validate results for key removal + assert: + that: + - sysctl_test2 is changed + - "'kernel.panic' not in sysctl_content2.stdout_lines" -- name: Test remove kernel.panic again - sysctl: - name: kernel.panic - value: 2 - state: absent - reload: no - sysctl_file: "{{ output_dir_test }}/sysctl.conf" - register: sysctl_test2_change_test + - name: Test remove kernel.panic again + sysctl: + name: kernel.panic + value: 2 + state: absent + reload: no + sysctl_file: "{{ output_dir_test }}/sysctl.conf" + register: sysctl_test2_change_test -- name: Assert that no change was made - assert: - that: - - sysctl_test2_change_test is not changed + - name: Assert that no change was made + assert: + that: + - sysctl_test2_change_test is not changed -- name: Try sysctl with an invalid value - sysctl: - name: net.ipv4.ip_forward - value: foo - register: sysctl_test3 - ignore_errors: yes + - name: Try sysctl with an invalid value + sysctl: + name: net.ipv4.ip_forward + value: foo + register: sysctl_test3 + ignore_errors: yes -- debug: - var: sysctl_test3 - verbosity: 1 + - debug: + var: sysctl_test3 + verbosity: 1 -- name: validate results for test 3 - assert: - that: - - sysctl_test3 is failed + - name: validate results for test 3 + assert: + that: + - sysctl_test3 is failed -## -## sysctl - sysctl_set -## + ## + ## sysctl - sysctl_set + ## -- name: set net.ipv4.ip_forward - sysctl: - name: net.ipv4.ip_forward - value: 1 - sysctl_set: yes - reload: no - register: sysctl_test3 + - name: set net.ipv4.ip_forward + sysctl: + name: net.ipv4.ip_forward + value: 1 + sysctl_set: yes + reload: no + register: sysctl_test3 -- name: check with sysctl command - shell: sysctl net.ipv4.ip_forward - register: sysctl_check3 + - name: check with sysctl command + shell: sysctl net.ipv4.ip_forward + register: sysctl_check3 -- debug: - var: item - verbosity: 1 - with_items: - - "{{ sysctl_test3 }}" - - "{{ sysctl_check3 }}" + - debug: + var: item + verbosity: 1 + with_items: + - "{{ sysctl_test3 }}" + - "{{ sysctl_check3 }}" -- name: validate results for test 3 - assert: - that: - - sysctl_test3 is changed - - 'sysctl_check3.stdout_lines == ["net.ipv4.ip_forward = 1"]' + - name: validate results for test 3 + assert: + that: + - sysctl_test3 is changed + - 'sysctl_check3.stdout_lines == ["net.ipv4.ip_forward = 1"]' -- name: Try sysctl with no name - sysctl: - name: - value: 1 - sysctl_set: yes - ignore_errors: True - register: sysctl_no_name + - name: Try sysctl with no name + sysctl: + name: + value: 1 + sysctl_set: yes + ignore_errors: True + register: sysctl_no_name -- name: validate nameless results - assert: - that: - - sysctl_no_name is failed - - "sysctl_no_name.msg == 'name cannot be None'" + - name: validate nameless results + assert: + that: + - sysctl_no_name is failed + - "sysctl_no_name.msg == 'name cannot be None'" -- name: Try sysctl with no value - sysctl: - name: Foo - value: - sysctl_set: yes - ignore_errors: True - register: sysctl_no_value + - name: Try sysctl with no value + sysctl: + name: Foo + value: + sysctl_set: yes + ignore_errors: True + register: sysctl_no_value -- name: validate nameless results - assert: - that: - - sysctl_no_value is failed - - "sysctl_no_value.msg == 'value cannot be None'" + - name: validate nameless results + assert: + that: + - sysctl_no_value is failed + - "sysctl_no_value.msg == 'value cannot be None'" -- name: Try sysctl with an invalid value - sysctl: - name: net.ipv4.ip_forward - value: foo - sysctl_set: yes - register: sysctl_test4 - ignore_errors: yes + - name: Try sysctl with an invalid value + sysctl: + name: net.ipv4.ip_forward + value: foo + sysctl_set: yes + register: sysctl_test4 + ignore_errors: yes -- debug: - var: sysctl_test4 - verbosity: 1 + - debug: + var: sysctl_test4 + verbosity: 1 -- name: validate results for test 4 - assert: - that: - - 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'