Feat/ansible perf#1917
Conversation
…ne in file via loop
|
Caution Review failedThe pull request is closed. WalkthroughConsolidates and generalizes multiple Ansible playbooks: combines multi-package apt installs into single async tasks with async_status checks; consolidates sysctl and limit files into looped/blocked tasks and a single sysctl reload; centralizes proxy env management with blockinfile and adds strategy/gather_facts adjustments; refactors WireGuard to use slurp/base64 decoding and more idempotent checks; updates an image tag. Changes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
services/ansibler/server/ansible-playbooks/node-limits.yml (1)
52-53: Improve idempotency handling forsysctl --system.Using the command module without idempotency guards will likely report the task as changed every run, even when no actual sysctl state changes occur. Since the configuration is already written to
/etc/sysctl.d/99-claudie-custom.conf, consider marking this task as not changed:- name: Reload sysctl at once with new settings ansible.builtin.command: sysctl --system + changed_when: falseAlternatively, make the task conditional to run only when the config file changes (using a handler triggered by the blockinfile task) for finer control.
services/ansibler/server/ansible-playbooks/proxy/populate-proxy-envs.yml (1)
37-37: Minor inconsistency: usecreate: trueinstead ofcreate: yes.For consistency with line 3 of deploy-envoy.goyml (which changed
yestotrue), use the booleantrueinstead ofyes.Apply this diff:
- create: yes + create: trueservices/ansibler/templates/uninstall-nginx.goyml (1)
22-23: Add async_status check to verify completion.The async apt task runs in the background with
poll: 0, but there's no subsequentasync_statuscheck to verify it completes. This is inconsistent with the PR's stated approach of using "async with poll: 0 to run non-blocking tasks, with subsequent state checks using async_status."While the impact is lower for an uninstall playbook with
ignore_errors: true, completing the async pattern ensures consistent behavior and confirms the operation finished before the playbook exits.Apply this diff to add the async_status check:
- name: Uninstall nginx apt: name: - nginx - nginx-common - libnginx-mod-stream state: absent purge: yes autoremove: yes + register: async_job async: 600 poll: 0 ignore_errors: true - name: Remove NGINX configuration directory file: path: /etc/nginx state: absent ignore_errors: true - name: Remove NGINX log directory file: path: /var/log/nginx state: absent ignore_errors: true - name: Remove NGINX cache directory file: path: /var/cache/nginx state: absent ignore_errors: true + + - name: Check nginx uninstall completion + async_status: + jid: "{{ async_job.ansible_job_id }}" + register: result + until: result.finished + retries: 30 + delay: 5 + ignore_errors: trueservices/ansibler/server/ansible-playbooks/wireguard/tasks/kill_unattended_upgrades.yml (1)
3-9: Optional: Simplify the masking logic.When masking a service, the
state: stoppedandenabled: falseparameters are redundant since masking prevents the service from being started by any means. The task could be simplified to only mask the service.Consider this simplified approach:
-- name: Mask unattended-upgrades service if it exists - ansible.builtin.systemd: - name: unattended-upgrades.service - state: stopped - enabled: false - masked: true - failed_when: false +- name: Mask unattended-upgrades service if it exists + ansible.builtin.systemd: + name: unattended-upgrades.service + masked: true + failed_when: false
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
services/ansibler/server/ansible-playbooks/longhorn-req.yml(3 hunks)services/ansibler/server/ansible-playbooks/node-limits.yml(1 hunks)services/ansibler/server/ansible-playbooks/proxy/commit-proxy-envs-changes.yml(2 hunks)services/ansibler/server/ansible-playbooks/proxy/populate-proxy-envs.yml(2 hunks)services/ansibler/server/ansible-playbooks/proxy/remove-proxy-envs.yml(1 hunks)services/ansibler/server/ansible-playbooks/wireguard.yml(0 hunks)services/ansibler/server/ansible-playbooks/wireguard/tasks/configure.yml(2 hunks)services/ansibler/server/ansible-playbooks/wireguard/tasks/kill_unattended_upgrades.yml(1 hunks)services/ansibler/server/ansible-playbooks/wireguard/tasks/main.yml(1 hunks)services/ansibler/server/ansible-playbooks/wireguard/templates/wg-dynamic.conf.j2(1 hunks)services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2(1 hunks)services/ansibler/templates/deploy-envoy.goyml(1 hunks)services/ansibler/templates/uninstall-nginx.goyml(1 hunks)
💤 Files with no reviewable changes (1)
- services/ansibler/server/ansible-playbooks/wireguard.yml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/deploy-envoy.goyml:14-16
Timestamp: 2025-06-10T08:11:39.185Z
Learning: In Ansible playbook templates within the services/ansibler/templates/ directory, relative paths for include_tasks are intentional and should not be changed to use playbook_dir variable, as the file structure is replicated inside containers to match these relative paths.
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/envoy.goyml:8-8
Timestamp: 2025-06-10T08:08:21.087Z
Learning: In Claudie's Envoy configuration, the placeholder `wireguard_private_ip_replace_me` in `services/ansibler/templates/envoy.goyml` is intentionally left as a literal string. The `deploy-envoy.goyml` Ansible playbook contains a task that uses `ansible.builtin.replace` to replace this placeholder with `{{ private_ip }}` (the actual private IP of each node) during runtime execution. This is a two-stage templating pattern where Go templates generate configs with placeholders, then Ansible performs node-specific replacements.
📚 Learning: 2025-06-10T08:08:21.087Z
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/envoy.goyml:8-8
Timestamp: 2025-06-10T08:08:21.087Z
Learning: In Claudie's Envoy configuration, the placeholder `wireguard_private_ip_replace_me` in `services/ansibler/templates/envoy.goyml` is intentionally left as a literal string. The `deploy-envoy.goyml` Ansible playbook contains a task that uses `ansible.builtin.replace` to replace this placeholder with `{{ private_ip }}` (the actual private IP of each node) during runtime execution. This is a two-stage templating pattern where Go templates generate configs with placeholders, then Ansible performs node-specific replacements.
Applied to files:
services/ansibler/server/ansible-playbooks/wireguard/tasks/configure.ymlservices/ansibler/templates/deploy-envoy.goymlservices/ansibler/server/ansible-playbooks/wireguard/templates/wg-dynamic.conf.j2services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2
📚 Learning: 2025-06-10T08:08:21.087Z
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/envoy.goyml:8-8
Timestamp: 2025-06-10T08:08:21.087Z
Learning: In Claudie's Envoy configuration templates, placeholders like `wireguard_private_ip_replace_me` are intentionally left as literal strings to be replaced by Ansible playbooks during runtime execution, not by Go template variables. This is a two-stage templating pattern where Go templates generate configs with placeholders, then Ansible performs string replacement with actual runtime values.
Applied to files:
services/ansibler/server/ansible-playbooks/wireguard/tasks/configure.ymlservices/ansibler/server/ansible-playbooks/wireguard/templates/wg-dynamic.conf.j2services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2
📚 Learning: 2025-06-10T08:11:39.185Z
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/deploy-envoy.goyml:14-16
Timestamp: 2025-06-10T08:11:39.185Z
Learning: In Ansible playbook templates within the services/ansibler/templates/ directory, relative paths for include_tasks are intentional and should not be changed to use playbook_dir variable, as the file structure is replicated inside containers to match these relative paths.
Applied to files:
services/ansibler/server/ansible-playbooks/proxy/populate-proxy-envs.ymlservices/ansibler/server/ansible-playbooks/node-limits.ymlservices/ansibler/server/ansible-playbooks/wireguard/tasks/main.yml
📚 Learning: 2025-06-10T08:22:31.991Z
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/server/domain/usecases/setup_loadbalancers.go:112-118
Timestamp: 2025-06-10T08:22:31.991Z
Learning: In services/ansibler/server/domain/usecases/setup_loadbalancers.go, the uninstallNginx function comment about "errors will be ignored" refers specifically to the Ansible playbook tasks having ignore_errors directives for idempotency, not the Go function ignoring template generation or execution errors. The Go function should still return errors for template loading/generation failures and overall Ansible execution issues.
Applied to files:
services/ansibler/templates/uninstall-nginx.goyml
🔇 Additional comments (19)
services/ansibler/server/ansible-playbooks/node-limits.yml (3)
13-20: Good consolidation using loop for PAM limits.The refactoring from separate tasks to a single looped
lineinfiletask reduces redundancy and improves playbook efficiency, aligning with the PR objectives for performance optimization.
22-29: Good consolidation using loop for systemd limits.Consolidating the two systemd config files into a single looped task is effective and maintains clarity. This reduces playbook execution overhead.
35-50: Solid blockinfile consolidation for kernel parameters.Replacing multiple individual
sysctlorlineinfiletasks with a singleblockinfileoperation is an efficient approach. The explicit ownership, group, and mode settings (root/root/0644) are appropriate for a system config file, andcreate: yesensures the file exists.services/ansibler/templates/deploy-envoy.goyml (1)
3-6: LGTM! Good performance optimization.Limiting fact gathering to only the required distribution facts reduces overhead while ensuring the necessary
ansible_facts['distribution_release']andansible_facts['distribution']are available for lines 28-29.services/ansibler/server/ansible-playbooks/proxy/commit-proxy-envs-changes.yml (2)
47-47: LGTM! Appropriate fact gathering optimization.The tasks in this play don't require Ansible facts, so disabling fact gathering improves performance.
72-72: LGTM! Appropriate fact gathering optimization.The tasks in this play don't require Ansible facts, so disabling fact gathering improves performance.
services/ansibler/server/ansible-playbooks/proxy/remove-proxy-envs.yml (1)
18-22: LGTM! Cleaner block-based removal approach.The blockinfile approach with matching marker (verified against populate-proxy-envs.yml line 38) is more maintainable than per-key lineinfile deletions.
services/ansibler/server/ansible-playbooks/proxy/populate-proxy-envs.yml (3)
2-2: LGTM! Good parallelization strategy.The
strategy: freeallows each host to execute tasks independently without waiting, which is appropriate for proxy setup since there are no cross-host dependencies in this playbook.
6-15: LGTM! Cleaner task consolidation.Using a loop to create multiple systemd drop-in directories is more maintainable than separate tasks. Note that
/etc/apt/apt.conf.d/is not explicitly created here, but it should exist by default on Debian/Ubuntu systems.
34-45: LGTM! Improved maintainability with blockinfile.The blockinfile approach with a defined marker is more maintainable than managing individual proxy variables with lineinfile. This correctly coordinates with the removal logic in remove-proxy-envs.yml.
services/ansibler/server/ansible-playbooks/longhorn-req.yml (2)
15-26: Nice refactoring—consolidates package installation efficiently.Combining both packages into a single async task improves efficiency and follows the async pattern correctly. The
retriesanddelayon this task appropriately handle transient failures when starting the async job, while the separateasync_statuscheck handles failures during the actual package installation.
3-3: > Likely an incorrect or invalid review comment.services/ansibler/server/ansible-playbooks/wireguard/templates/wg-dynamic.conf.j2 (1)
1-22: LGTM! Consistent migration to slurp-based key handling.The template correctly migrates from stdout-based key handling to using
slurpmodule withcontent | b64decode. All key references (PrivateKey, PublicKey comparisons, and peer PublicKey outputs) are consistently updated across both dynamic and static peer groups.services/ansibler/server/ansible-playbooks/wireguard/tasks/configure.yml (6)
8-12: LGTM! Improved idempotency for key generation.The addition of the
createsparameter ensures the keypair is only generated when it doesn't exist, preventing unnecessary regeneration and improving idempotency. Theumask 077also ensures proper file permissions.
14-22: LGTM! Proper use of slurp module for reading keys.Replacing shell
catcommands with theansible.builtin.slurpmodule is a best practice. The slurp module is idempotent, returns structured data, and aligns with the template changes that usecontent | b64decodefor key handling.
24-32: LGTM! Cleaner interface existence check.Using
ansible.builtin.staton/sys/class/net/wg0is more reliable than parsing shell command output. The conditional logic correctly creates the interface only when it doesn't exist.
34-43: LGTM! Idempotent IP address assignment.The task correctly checks for existing IP addresses before assignment, improving idempotency. The use of
changed_when: falseon the check is appropriate.Note: The string match on line 43 (
private_ip not in wg0_ip_info.stdout) assumes the IP format matches exactly. This should work correctly as long asprivate_ipis formatted consistently (e.g., "10.0.0.1" without CIDR notation).
58-60: LGTM! Modernized MTU setting command.Replacing
ifconfigwithip link set mtuis a good modernization. Theipcommand is the standard tool in modern Linux distributions.
61-70: LGTM! Improved DHCP config persistence with file creation.Adding
create: yesensures/etc/dhcp/dhclient.confexists before attempting to add the MTU configuration block. Theinsertbefore: "^request"pattern correctly targets the appropriate location in the dhclient configuration.
Despire
left a comment
Despire
left a comment
There was a problem hiding this comment.
I think these changes are Ok, also address the comments, as you have some typos, mainly with wireguard
Increase async_status retries to match the async timeout.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
services/ansibler/server/ansible-playbooks/longhorn-req.yml (1)
50-56: Async polling window now reasonably matches the async timeout (good improvement vs the earlier 10×5s wait).
🧹 Nitpick comments (3)
services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2 (2)
6-14: Guard against missing key vars (async timing) and avoid repeatedb64decodein loops.
Ifpublickey/hostvars[host].publickeyisn’t present yet, this template will error (or behave unexpectedly) during rendering—this seems plausible given the PR’s async/non-blocking changes and the note about needing host key synchronization. Consider precomputinglocal_public_keyonce and defaulting missing remote keys to''to fail fast with a clearer message or safely skip.[Interface] Address = {{ private_ip }}/24 PrivateKey = {{ privatekey.content | b64decode }} ListenPort = {{ wg_listen_port }} +{% set local_public_key = (publickey.content | b64decode) %} + {% for host in groups['dynamic'] %} -{% if publickey.content | b64decode != hostvars[host].publickey.content | b64decode %} +{% set remote_public_key = ((hostvars[host].publickey.content | default('')) | b64decode) %} +{% if local_public_key != remote_public_key %} [Peer] -PublicKey = {{ hostvars[host].publickey.content | b64decode }} +PublicKey = {{ remote_public_key }} Endpoint = {{ hostvars[host].ansible_host }}:{{ wg_listen_port }} AllowedIps = {{ hostvars[host].private_ip }}/32 PersistentKeepalive = 60 {% endif %} {% endfor %} {% for host in groups['static'] %} -{% if publickey.content | b64decode != hostvars[host].publickey.content | b64decode %} +{% set remote_public_key = ((hostvars[host].publickey.content | default('')) | b64decode) %} +{% if local_public_key != remote_public_key %} [Peer] -PublicKey = {{ hostvars[host].publickey.content | b64decode}} +PublicKey = {{ remote_public_key }} Endpoint = {{ hostvars[host].ansible_host }}:{{ wg_listen_port }} AllowedIps = {{ hostvars[host].private_ip }}/32 {% endif %} {% endfor %}Also applies to: 16-23
19-19: Minor: fix whitespace (b64decode }}) for consistency.-PublicKey = {{ hostvars[host].publickey.content | b64decode}} +PublicKey = {{ hostvars[host].publickey.content | b64decode }}services/ansibler/server/ansible-playbooks/longhorn-req.yml (1)
15-27: Async apt install: consider addingcache_valid_timeto avoid forced updates on every run.
update_cache: truecan dominate runtime on repeated runs; if acceptable, prefercache_valid_time: <seconds>for better performance while staying correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
services/ansibler/server/ansible-playbooks/longhorn-req.yml(3 hunks)services/ansibler/server/ansible-playbooks/wireguard/tasks/main.yml(1 hunks)services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- services/ansibler/server/ansible-playbooks/wireguard/tasks/main.yml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/envoy.goyml:8-8
Timestamp: 2025-06-10T08:08:21.087Z
Learning: In Claudie's Envoy configuration, the placeholder `wireguard_private_ip_replace_me` in `services/ansibler/templates/envoy.goyml` is intentionally left as a literal string. The `deploy-envoy.goyml` Ansible playbook contains a task that uses `ansible.builtin.replace` to replace this placeholder with `{{ private_ip }}` (the actual private IP of each node) during runtime execution. This is a two-stage templating pattern where Go templates generate configs with placeholders, then Ansible performs node-specific replacements.
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/envoy.goyml:8-8
Timestamp: 2025-06-10T08:08:21.087Z
Learning: In Claudie's Envoy configuration templates, placeholders like `wireguard_private_ip_replace_me` are intentionally left as literal strings to be replaced by Ansible playbooks during runtime execution, not by Go template variables. This is a two-stage templating pattern where Go templates generate configs with placeholders, then Ansible performs string replacement with actual runtime values.
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/deploy-envoy.goyml:14-16
Timestamp: 2025-06-10T08:11:39.185Z
Learning: In Ansible playbook templates within the services/ansibler/templates/ directory, relative paths for include_tasks are intentional and should not be changed to use playbook_dir variable, as the file structure is replicated inside containers to match these relative paths.
📚 Learning: 2025-06-10T08:08:21.087Z
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/envoy.goyml:8-8
Timestamp: 2025-06-10T08:08:21.087Z
Learning: In Claudie's Envoy configuration templates, placeholders like `wireguard_private_ip_replace_me` are intentionally left as literal strings to be replaced by Ansible playbooks during runtime execution, not by Go template variables. This is a two-stage templating pattern where Go templates generate configs with placeholders, then Ansible performs string replacement with actual runtime values.
Applied to files:
services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2
📚 Learning: 2025-06-10T08:08:21.087Z
Learnt from: Despire
Repo: berops/claudie PR: 1735
File: services/ansibler/templates/envoy.goyml:8-8
Timestamp: 2025-06-10T08:08:21.087Z
Learning: In Claudie's Envoy configuration, the placeholder `wireguard_private_ip_replace_me` in `services/ansibler/templates/envoy.goyml` is intentionally left as a literal string. The `deploy-envoy.goyml` Ansible playbook contains a task that uses `ansible.builtin.replace` to replace this placeholder with `{{ private_ip }}` (the actual private IP of each node) during runtime execution. This is a two-stage templating pattern where Go templates generate configs with placeholders, then Ansible performs node-specific replacements.
Applied to files:
services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2
📚 Learning: 2025-08-25T06:50:32.150Z
Learnt from: Despire
Repo: berops/claudie PR: 1838
File: services/terraformer/server/domain/utils/tofu/terraform.go:23-25
Timestamp: 2025-08-25T06:50:32.150Z
Learning: In the Claudie codebase, Despire prefers to use environment variable values as-is without additional runtime validation, trusting operators to configure reasonable values rather than adding defensive programming checks.
Applied to files:
services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2
📚 Learning: 2025-08-25T06:51:04.662Z
Learnt from: Despire
Repo: berops/claudie PR: 1838
File: services/ansibler/server/domain/usecases/usecases.go:17-19
Timestamp: 2025-08-25T06:51:04.662Z
Learning: In services/ansibler/server/domain/usecases/usecases.go, Despire prefers to use environment variable values as-is without additional validation guards for SpawnProcessLimit, trusting the deployment configuration to provide appropriate values.
Applied to files:
services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2
🔇 Additional comments (1)
services/ansibler/server/ansible-playbooks/wireguard/templates/wg-static.conf.j2 (1)
3-3: Good:PrivateKeynow correctly usesprivatekey.content | b64decode(fixes prior critical).
m-brando
requested review from
jakubhlavacka
and removed request for
jakubhlavacka
2 participants
This PR addresses issue #1831.
Improvements:
asyncwithpoll: 0to run non-blocking tasks, combined with later state checks usingasync_status.overall speedup of execution is ~20%
The WireGuard playbook remains the slowest part of the workflow. It could be parallelized further, but doing so would require synchronizing host facts, particularly the public key values used when rendering the WireGuard template in this task:
Summary by CodeRabbit
Performance Improvements
Stability & Reliability
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.