From d171a4a7b9d96ef1ad2f1eb0df3f27c99fd16f75 Mon Sep 17 00:00:00 2001 From: Jose Date: Tue, 18 Nov 2025 20:24:43 +0100 Subject: [PATCH] =?UTF-8?q?refactor=20=E2=99=BB=EF=B8=8F:=20Refactored=20a?= =?UTF-8?q?ll=20task=20files=20to=20use=20centralized=20helper=20functions?= =?UTF-8?q?=20from=20`tasks/helpers.yml`,=20improving=20code=20consistency?= =?UTF-8?q?,=20maintainability,=20and=20idempotency.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All task files now use centralized helper functions, ensuring idempotency across all stages. Code is cleaner, more maintainable, and no breaking changes were introduced. --- README.md | 1 + REFACTORING_SUMMARY.md | 242 ----------------------------------------- defaults/main.yml | 1 + tasks/configure-vm.yml | 24 ++-- tasks/helpers.yml | 17 +++ 5 files changed, 33 insertions(+), 252 deletions(-) delete mode 100644 REFACTORING_SUMMARY.md diff --git a/README.md b/README.md index 05ac709..7e305ae 100644 --- a/README.md +++ b/README.md @@ -196,6 +196,7 @@ All task files use centralized helper functions for consistency and maintainabil | `check_vm_exists` | Check if VM config file exists | `vm_exists` | | `check_template` | Check if VM is a template | `is_template` | | `check_vm_status` | Get current VM status (running/stopped) | `vm_status` | +| `check_disk_attached` | Check if disk is attached via `qm config` | `disk_attached` | | `check_storage` | Check available storage space | `storage_available` | | `validate_vm_id` | Validate VM ID format (100-999999) | (assertions only) | | `get_vm_info` | Read and parse VM config file | `vm_info` | diff --git a/REFACTORING_SUMMARY.md b/REFACTORING_SUMMARY.md deleted file mode 100644 index 932bd9d..0000000 --- a/REFACTORING_SUMMARY.md +++ /dev/null @@ -1,242 +0,0 @@ -# Refactoring Summary: Helper Functions Integration - -## Overview -Refactored all task files to use centralized helper functions from `tasks/helpers.yml`, improving code consistency, maintainability, and idempotency. - -## Changes Made - -### 1. **tasks/create-vm.yml** ✅ REFACTORED -**Before:** -- Used inline `stat` task to check if VM exists -- Repeated file path checks throughout - -**After:** -- ✅ Uses `check_vm_exists` helper function -- ✅ Cleaner, DRY code -- ✅ Consistent fact variable: `vm_exists` -- ✅ Idempotent: Skips VM creation if already exists - -**Key improvement:** -```yaml -# Old (inline): -- ansible.builtin.stat: path: "/etc/pve/qemu-server/{{ vm_id }}.conf" - register: vm_conf - -# New (helper): -- ansible.builtin.include_tasks: helpers.yml - vars: - helper_task: check_vm_exists - target_vm_id: "{{ vm_id }}" -# Now use: vm_exists (boolean fact) -``` - ---- - -### 2. **tasks/create-template.yml** ✅ REFACTORED -**Before:** -- Used inline `shell` commands with grep -- Return code checking mixed throughout -- Inconsistent status checking - -**After:** -- ✅ Uses `check_template` helper for template status -- ✅ Uses `check_vm_status` helper for VM state -- ✅ Cleaner logic with proper facts: `is_template`, `vm_status` -- ✅ Idempotent: Skips conversion if already template -- ✅ Verifies VM is stopped before conversion - -**Key improvements:** -```yaml -# Check if template -- include_tasks: helpers.yml - vars: - helper_task: check_template - target_vm_id: "{{ vm_id }}" -# Use: is_template (boolean) - -# Check VM status -- include_tasks: helpers.yml - vars: - helper_task: check_vm_status - target_vm_id: "{{ vm_id }}" -# Use: vm_status (string: 'running'/'stopped') -``` - ---- - -### 3. **tasks/create-clones.yml** ✅ REFACTORED -**Before:** -- Used inline `stat` for clone existence checks -- Repeated file path checks in loop -- Inconsistent status tracking - -**After:** -- ✅ Uses `check_vm_exists` helper (per-clone checking) -- ✅ Consistent fact: `vm_exists` reused for each clone -- ✅ Idempotent: Skips existing clones -- ✅ Enhanced output shows clone status (EXISTS vs WILL BE CREATED) - -**Key improvements:** -- Per-clone idempotency checking -- Cleaner loop variable handling -- Conditional messaging in output - ---- - -### 4. **tasks/configure-vm.yml** ✅ REFACTORED -**Before:** -- No cleanup of old snippets -- Could conflict with old configurations on re-runs - -**After:** -- ✅ Added `cleanup_snippets` helper call -- ✅ Removes old snippets before creating new ones -- ✅ Safe to re-run without conflicts -- ✅ Improved Cloud-Init configuration message - -**Key improvement:** -```yaml -- name: "[CONFIG] Clean up old Cloud-Init snippets (if any)" - ansible.builtin.include_tasks: helpers.yml - vars: - helper_task: cleanup_snippets - target_vm_id: "{{ vm_id }}" -``` - ---- - -## Helper Functions Now Being Used - -| Helper | Used In | Purpose | -|--------|---------|---------| -| `check_vm_exists` | create-vm.yml, create-clones.yml | Check if VM config exists | -| `check_template` | create-template.yml | Check if VM is template | -| `check_vm_status` | create-template.yml | Get VM status (running/stopped) | -| `cleanup_snippets` | configure-vm.yml | Remove old Cloud-Init snippets | - -## Idempotency Improvements - -### ✅ All 6 Stages Now Fully Idempotent - -| Stage | Task | Idempotency Method | -|-------|------|-------------------| -| 1 | preflight-checks.yml | Validation only (no state changes) | -| 2 | download-image.yml | File existence check | -| 3 | create-vm.yml | **VM existence check** (helper) | -| 4 | configure-vm.yml | **Snippet cleanup + re-apply** (helper) | -| 5 | create-template.yml | **Template status check** (helper) | -| 6 | create-clones.yml | **Per-clone existence checks** (helper) | - -### Safe Re-runs -```bash -# First run: Creates everything -ansible-playbook tasks/main.yml - -# Second run: Much faster, skips already-created resources -ansible-playbook tasks/main.yml -# Output: All tasks show "SKIPPED" where appropriate -``` - ---- - -## Code Quality Improvements - -✅ **DRY Principle** - Eliminated duplicated code patterns -✅ **Consistency** - All tasks use same helpers, same variable names -✅ **Maintainability** - Single source of truth for common operations -✅ **Testability** - Helpers can be tested independently -✅ **Clarity** - Task intent is clearer with helper names - ---- - -## README Updates - -Enhanced documentation with: -- **Helper Functions Table** - Lists all 8 available helpers -- **Usage Example** - Shows how to call helpers -- **Idempotency Details** - Per-stage idempotency guarantees -- **Feature Updates** - Clarifies modular design - ---- - -## Backward Compatibility - -✅ **No Breaking Changes** -- All task names unchanged -- All variables unchanged -- Playbook interface unchanged -- Results identical to previous version - ---- - -## Testing Recommendations - -1. **Validate syntax:** - ```bash - ansible-playbook tasks/main.yml --syntax-check - ``` - -2. **Dry run:** - ```bash - ansible-playbook tasks/main.yml --check -vv - ``` - -3. **First run:** - ```bash - ansible-playbook tasks/main.yml -vv - ``` - -4. **Idempotency test (re-run):** - ```bash - ansible-playbook tasks/main.yml -vv - # Should skip all operations except validation - ``` - -5. **Per-tag testing:** - ```bash - ansible-playbook tasks/main.yml --tags preflight -vv - ansible-playbook tasks/main.yml --tags image -vv - ansible-playbook tasks/main.yml --tags vm -vv - ``` - ---- - -## Files Modified - -1. ✅ `tasks/create-vm.yml` - Uses `check_vm_exists` helper -2. ✅ `tasks/create-template.yml` - Uses `check_template` and `check_vm_status` helpers -3. ✅ `tasks/create-clones.yml` - Uses `check_vm_exists` helper -4. ✅ `tasks/configure-vm.yml` - Uses `cleanup_snippets` helper -5. ✅ `README.md` - Updated with helper functions documentation - -## Files NOT Modified - -- `tasks/helpers.yml` - Already well-written -- `tasks/main.yml` - No changes needed -- `tasks/preflight-checks.yml` - Already using direct checks (validation stage) -- `tasks/download-image.yml` - Already using file existence checks (simple, doesn't need helper) -- `defaults/main.yml` - Already has all necessary variables -- Templates - No changes needed - ---- - -## Future Enhancements - -Potential improvements for future versions: -- Add `update_clone_count` helper to track deployment statistics -- Add `validate_clone_config` helper for pre-flight clone validation -- Add logging/audit trail helper for operations tracking -- Consider adding backup helper for VM snapshots before conversions - ---- - -## Conclusion - -✅ **Refactoring Complete** -- All task files now use centralized helper functions -- Idempotency guaranteed across all stages -- Code is cleaner, more maintainable -- No breaking changes -- Full backward compatibility - -**Status:** Ready for production use ✅ diff --git a/defaults/main.yml b/defaults/main.yml index 9784ddf..f080a53 100644 --- a/defaults/main.yml +++ b/defaults/main.yml @@ -172,5 +172,6 @@ retry_delay: 5 # Timeout settings (in seconds) image_download_timeout: 300 + vm_boot_timeout: 60 cloud_init_timeout: 120 diff --git a/tasks/configure-vm.yml b/tasks/configure-vm.yml index ae1774e..8fef276 100644 --- a/tasks/configure-vm.yml +++ b/tasks/configure-vm.yml @@ -20,13 +20,17 @@ - name: "[CONFIG] Import and attach disk" block: - - name: "[CONFIG] Check if disk already exists" - ansible.builtin.stat: - path: "/var/lib/vz/images/{{ vm_id }}/vm-{{ vm_id }}-disk-0.qcow2" - register: disk_exists - changed_when: false + - name: "[CONFIG] Check if disk is already attached" + ansible.builtin.include_tasks: helpers.yml + vars: + helper_task: check_disk_attached + target_vm_id: "{{ vm_id }}" - - name: "[CONFIG] Import qcow2 disk" + - name: "[CONFIG] Display disk status" + ansible.builtin.debug: + msg: "Disk status for VM {{ vm_id }}: {{ 'ALREADY ATTACHED' if disk_attached else 'WILL BE IMPORTED & ATTACHED' }}" + + - name: "[CONFIG] Import qcow2 disk (if not already attached)" ansible.builtin.command: > qm importdisk {{ vm_id }} {{ debian_image_path }} @@ -35,13 +39,13 @@ retries: "{{ max_retries }}" delay: "{{ retry_delay }}" until: disk_import is succeeded - when: not disk_exists.stat.exists + when: not disk_attached - name: "[CONFIG] Verify disk import" ansible.builtin.fail: msg: "Disk import failed for VM {{ vm_id }}" when: - - not disk_exists.stat.exists + - not disk_attached - disk_import is failed - name: "[CONFIG] Attach imported disk" @@ -50,7 +54,7 @@ --scsihw virtio-scsi-pci --scsi0 {{ storage }}:vm-{{ vm_id }}-disk-0 register: disk_attach - when: not disk_exists.stat.exists + when: not disk_attached changed_when: disk_attach.rc == 0 - name: "[CONFIG] Enable serial console and set boot order" @@ -63,7 +67,7 @@ - name: "[CONFIG] Display disk configuration" ansible.builtin.debug: - msg: "✓ Disk configured and attached to VM {{ vm_id }}" + msg: "✓ Disk {{ 'attached' if disk_attached else 'imported and attached' }} to VM {{ vm_id }}" rescue: - name: "[CONFIG] Handle disk configuration error" diff --git a/tasks/helpers.yml b/tasks/helpers.yml index c705322..3cd5b66 100644 --- a/tasks/helpers.yml +++ b/tasks/helpers.yml @@ -147,3 +147,20 @@ - "/var/lib/vz/snippets/{{ target_vm_id }}-sshkey.pub" when: helper_task == "cleanup_snippets" + +################################################################## +# CHECK IF DISK IS ATTACHED +################################################################## +- name: "[HELPER] Check if disk is attached to VM" + block: + - name: "[HELPER] Query VM config for disk attachment" + ansible.builtin.shell: "qm config {{ target_vm_id }} | grep -q '^scsi0:'" + changed_when: false + failed_when: false + register: disk_check + + - name: "[HELPER] Set fact: disk_attached" + ansible.builtin.set_fact: + disk_attached: "{{ disk_check.rc == 0 }}" + + when: helper_task == "check_disk_attached"