From 0055971afd5524f0df00fc8d2203a80f828885f7 Mon Sep 17 00:00:00 2001 From: Colin Wilk Date: Tue, 21 Jul 2026 19:10:19 +0200 Subject: fix: prevent lost YAML targets during parallel writes The YAML strategy performed a separate read-modify-write operation for every managed host. Because these tasks were delegated to the same Prometheus host, parallel Ansible forks could read the same original file before any fork wrote its update. Each fork then generated YAML containing only its own target. Although the copy module atomically replaced the file, it did not make the full read-modify-write sequence atomic. The last fork to write therefore overwrote targets added by earlier forks. Collect exporter updates from all hosts through hostvars, process them in one run_once operation, and write each distinct target file once. This preserves every target without requiring users to set serial. Add parallel coverage for multiple target files and label groups, and reset the fixtures on every convergence so stale files cannot hide the race. --- molecule/default/converge.yml | 15 ++- molecule/default/tests/test_yaml_strategy.py | 17 +++ tasks/main.yml | 7 +- tasks/strategy_yaml.yml | 150 +++++++++++++++++---------- 4 files changed, 133 insertions(+), 56 deletions(-) diff --git a/molecule/default/converge.yml b/molecule/default/converge.yml index 4e379da..f882c44 100644 --- a/molecule/default/converge.yml +++ b/molecule/default/converge.yml @@ -185,16 +185,19 @@ pre_tasks: - name: Create yaml parallel target ansible.builtin.copy: - dest: /opt/yaml_parallel.yml + dest: "{{ item }}" mode: "0644" content: | - labels: job: node targets: - existing:9100 - force: false + force: true delegate_to: "{{ prometheus_target_host }}" run_once: true + loop: + - /opt/yaml_parallel.yml + - /opt/yaml_parallel_second.yml vars: prometheus_target_host: prometheus @@ -207,6 +210,14 @@ host: "{{ inventory_hostname }}:9100" labels: job: node + - path: /opt/yaml_parallel_second.yml + host: "{{ inventory_hostname }}:9200" + labels: + job: node + - path: /opt/yaml_parallel.yml + host: "{{ inventory_hostname }}:9300" + labels: + job: alternate ################################################################################ ################################################################################ diff --git a/molecule/default/tests/test_yaml_strategy.py b/molecule/default/tests/test_yaml_strategy.py index 89dbdeb..7ba834e 100644 --- a/molecule/default/tests/test_yaml_strategy.py +++ b/molecule/default/tests/test_yaml_strategy.py @@ -44,6 +44,22 @@ def test_yaml_parallel_writes_keep_all_hosts_in_same_group(host): "application4:9100", ] + assert get_group_by_labels(groups, {"job": "alternate"})["targets"] == [ + "application:9300", + "application2:9300", + "application3:9300", + "application4:9300", + ] + + second_groups = read_yaml_file(host, "/opt/yaml_parallel_second.yml") + assert get_group_by_labels(second_groups, {"job": "node"})["targets"] == [ + "existing:9100", + "application:9200", + "application2:9200", + "application3:9200", + "application4:9200", + ] + def test_yaml_user_edited_file_is_reparsed_and_keeps_expected_semantics(host): groups = read_yaml_file(host, "/opt/yaml_user_edited.yml") @@ -70,6 +86,7 @@ def test_yaml_output_remains_parseable_after_all_operations(host): for path in [ "/opt/yaml_bootstrap.yml", "/opt/yaml_parallel.yml", + "/opt/yaml_parallel_second.yml", "/opt/yaml_user_edited.yml", "/opt/yaml_move_remove_last.yml", "/opt/yaml_branch_matrix.yml", diff --git a/tasks/main.yml b/tasks/main.yml index 10f54f7..ecffd79 100644 --- a/tasks/main.yml +++ b/tasks/main.yml @@ -1,11 +1,16 @@ --- -- name: Process all exporters +- name: Process YAML exporters + ansible.builtin.include_tasks: strategy_yaml.yml + when: prometheus_target_strategy == "yaml" + +- name: Process all lineinfile exporters ansible.builtin.include_tasks: file: process_exporter.yml loop: "{{ prometheus_target_exporter + ([] if prometheus_target_skip_default_exporters else prometheus_target_default_exporters) }}" loop_control: loop_var: item + when: prometheus_target_strategy != "yaml" - name: Run handlers ansible.builtin.include_tasks: diff --git a/tasks/strategy_yaml.yml b/tasks/strategy_yaml.yml index 8a8e38b..24cd6c8 100644 --- a/tasks/strategy_yaml.yml +++ b/tasks/strategy_yaml.yml @@ -1,71 +1,115 @@ --- -- name: Check if target file exists - ansible.builtin.stat: - path: "{{ _target_path }}" - register: _target_file_stat - delegate_to: "{{ prometheus_target_host }}" +- name: Prepare YAML exporter updates + ansible.builtin.set_fact: + _prometheus_target_yaml_updates: >- + {%- set ns = namespace(updates=[]) -%} + {%- set exporters = prometheus_target_exporter + + ([] if prometheus_target_skip_default_exporters else prometheus_target_default_exporters) -%} + {%- for exporter in exporters -%} + {%- set defaults = prometheus_target_exporter_defaults[exporter.id] | default({}) + if exporter.id is defined else {} -%} + {%- set path = + (exporter.path_prefix if exporter.path_prefix is defined else + (defaults.path_prefix | default(prometheus_target_exporter_target_prefix))) ~ + ((exporter.path if exporter.path is defined else defaults.path) | mandatory) -%} + {%- set host = (exporter.host if exporter.host is defined else defaults.host) | mandatory -%} + {%- set labels = (defaults.labels | default({})) | combine(exporter.labels | default({})) -%} + {%- set _ = ns.updates.append({'path': path, 'host': host, 'labels': labels}) -%} + {%- endfor -%} + {{ ns.updates }} + +# Every play host prepares its values above. Applying all updates from one host +# prevents parallel Ansible forks from overwriting each other's YAML changes. +- name: Aggregate YAML exporter updates + ansible.builtin.set_fact: + _prometheus_target_yaml_updates_all: >- + {%- set ns = namespace(updates=[]) -%} + {%- for target_host in ansible_play_batch -%} + {%- for update in hostvars[target_host]._prometheus_target_yaml_updates -%} + {%- set _ = ns.updates.append(update) -%} + {%- endfor -%} + {%- endfor -%} + {{ ns.updates }} + run_once: true -- name: Read target file +- name: Collect YAML target paths + ansible.builtin.set_fact: + _prometheus_target_yaml_paths: >- + {{ _prometheus_target_yaml_updates_all | map(attribute='path') | unique | list }} + run_once: true + +- name: Read YAML target files ansible.builtin.slurp: - src: "{{ _target_path }}" - register: _target_file_content + src: "{{ _yaml_path }}" delegate_to: "{{ prometheus_target_host }}" - when: _target_file_stat.stat.exists + loop: "{{ _prometheus_target_yaml_paths }}" + loop_control: + loop_var: _yaml_path + register: _yaml_target_files + failed_when: false + run_once: true -- name: Parse YAML and add host to matching target group +# Build each destination completely in memory and write it once. Besides +# avoiding lost updates, this keeps updates to different files independent. +- name: Build updated YAML target files ansible.builtin.set_fact: - _updated_targets: >- - {%- set targets = (_target_file_content.content | b64decode | from_yaml) if _target_file_stat.stat.exists else [] -%} - {%- set targets = targets if targets is not none else [] -%} - {%- set ns = namespace(found=false, result=[], match=true) -%} - {%- for group in targets -%} - {%- set group_labels = group.labels | default({}) -%} - {%- set ns.match = true -%} - {%- if _target_labels | length != group_labels | length -%} - {%- set ns.match = false -%} - {%- else -%} - {%- for key, value in _target_labels.items() -%} - {%- if group_labels.get(key) != value -%} - {%- set ns.match = false -%} + _prometheus_target_yaml_files: >- + {%- set output = namespace(files=[]) -%} + {%- for file in _yaml_target_files.results -%} + {%- set state = namespace(groups=(file.content | b64decode | from_yaml) + if file.content is defined else []) -%} + {%- set state.groups = state.groups if state.groups is not none else [] -%} + {%- for update in _prometheus_target_yaml_updates_all if update.path == file._yaml_path -%} + {%- set next = namespace(found=false, groups=[]) -%} + {%- for group in state.groups -%} + {%- set group_labels = group.labels | default({}) -%} + {%- if group_labels == update.labels -%} + {%- if update.host not in group.targets -%} + {%- set _ = group.targets.append(update.host) -%} + {%- endif -%} + {%- set next.found = true -%} + {%- set _ = next.groups.append(group) -%} + {%- else -%} + {%- set filtered = group.targets | reject('equalto', update.host) | list -%} + {%- if filtered | length > 0 -%} + {%- set replacement = {'targets': filtered} -%} + {%- if group_labels | length > 0 -%} + {%- set _ = replacement.update({'labels': group_labels}) -%} + {%- endif -%} + {%- set _ = next.groups.append(replacement) -%} + {%- endif -%} {%- endif -%} {%- endfor -%} - {%- endif -%} - {%- if ns.match -%} - {%- if _target_host not in group.targets -%} - {%- set _ = group.targets.append(_target_host) -%} - {%- endif -%} - {%- set ns.found = true -%} - {%- set _ = ns.result.append(group) -%} - {%- else -%} - {%- set filtered_targets = group.targets | reject("equalto", _target_host) | list -%} - {%- if filtered_targets | length > 0 -%} - {%- if group_labels | length > 0 -%} - {%- set _ = ns.result.append({"labels": group_labels, "targets": filtered_targets}) -%} - {%- else -%} - {%- set _ = ns.result.append({"targets": filtered_targets}) -%} + {%- if not next.found -%} + {%- set new_group = {'targets': [update.host]} -%} + {%- if update.labels | length > 0 -%} + {%- set _ = new_group.update({'labels': update.labels}) -%} {%- endif -%} + {%- set _ = next.groups.append(new_group) -%} {%- endif -%} - {%- endif -%} + {%- set state.groups = next.groups -%} + {%- endfor -%} + {%- set _ = output.files.append({'path': file._yaml_path, 'groups': state.groups}) -%} {%- endfor -%} - {%- if not ns.found -%} - {%- if _target_labels | length > 0 -%} - {%- set new_group = {"labels": _target_labels, "targets": [_target_host]} -%} - {%- else -%} - {%- set new_group = {"targets": [_target_host]} -%} - {%- endif -%} - {%- set _ = ns.result.append(new_group) -%} - {%- endif -%} - {{ ns.result }} + {{ output.files }} + run_once: true -- name: Write updated target file +- name: Write updated YAML target files ansible.builtin.copy: - content: "{{ _updated_targets | to_nice_yaml(indent=2, width=1337) }}" - dest: "{{ _target_path }}" + content: "{{ _yaml_file.groups | to_nice_yaml(indent=2, width=1337) }}" + dest: "{{ _yaml_file.path }}" mode: "0644" delegate_to: "{{ prometheus_target_host }}" become: true - register: _yaml_result + loop: "{{ _prometheus_target_yaml_files }}" + loop_control: + loop_var: _yaml_file + label: "{{ _yaml_file.path }}" + register: _yaml_results + run_once: true -- name: Track changes +- name: Track aggregated YAML changes on each host ansible.builtin.set_fact: - changed: "{{ changed | default(false) or _yaml_result.changed }}" + changed: >- + {{ hostvars[ansible_play_batch | first]._yaml_results.results | + selectattr('changed') | list | length > 0 }} -- cgit v1.2.3