diff options
| author | Colin Wilk <colin@wilk.cx> | 2026-07-21 19:10:19 +0200 |
|---|---|---|
| committer | Colin Wilk <colin@wilk.cx> | 2026-07-21 19:11:08 +0200 |
| commit | 0055971afd5524f0df00fc8d2203a80f828885f7 (patch) | |
| tree | 23019217d6a692aa83a5b09ede12bb3ebbc9da69 | |
| parent | d9b5c59a50ac9967dc365f2c8586869d6ef5d9bd (diff) | |
| download | ansible-role-prometheus-target-0055971afd5524f0df00fc8d2203a80f828885f7.tar.gz ansible-role-prometheus-target-0055971afd5524f0df00fc8d2203a80f828885f7.zip | |
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.
| -rw-r--r-- | molecule/default/converge.yml | 15 | ||||
| -rw-r--r-- | molecule/default/tests/test_yaml_strategy.py | 17 | ||||
| -rw-r--r-- | tasks/main.yml | 7 | ||||
| -rw-r--r-- | 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 }} |