Implement request cancellation handling to prevent unnecessary VM spi… #687
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements intelligent request cancellation handling to prevent unnecessary VM provisioning retries. The changes introduce a configurable retry limit with automatic detection of permanent vs. transient errors, preventing infinite retry loops when configuration issues exist.
- Adds
max_vm_retriesconfiguration parameter (default: 3) to limit retry attempts for failed VM provisioning - Implements error classification logic to distinguish between permanent configuration errors (invalid templates, permission issues) and transient failures (network timeouts, socket errors)
- Stores detailed error information during VM clone failures to enable informed retry decisions
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vmpooler.yaml.example | Adds documentation and default configuration for the new max_vm_retries parameter |
| lib/vmpooler/pool_manager.rb | Implements retry logic in handle_timed_out_vm, adds permanent_error? method for error classification, and enhances error capture in _clone_vm |
| spec/unit/pool_manager_spec.rb | Adds comprehensive test coverage for retry logic, permanent error detection, and various failure scenarios |
| Gemfile.lock | Adds arm64-darwin-23 platform support (Apple Silicon macOS 14) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| redis.zadd('vmpooler__odcreate__task', 1, "#{pool_alias}:#{pool}:1:#{request_id}") | ||
| # Check retry count and max retry limit before retrying | ||
| retry_count = (redis.hget("vmpooler__odrequest__#{request_id}", 'retry_count') || '0').to_i | ||
| max_retries = $config[:config]['max_vm_retries'] || 3 |
There was a problem hiding this comment.
[nitpick] Consider using the config method instead of the global $config variable for better encapsulation. While the codebase uses both patterns, the config method (defined at line 51) is the preferred approach. For example:
max_retries = config[:config]['max_vm_retries'] || 3This would be more consistent with Ruby best practices and some other parts of the codebase (e.g., lines 798, 806).
| max_retries = $config[:config]['max_vm_retries'] || 3 | |
| max_retries = config[:config]['max_vm_retries'] || 3 |
| dns_plugin.create_or_replace_record(new_vmname) unless dns_plugin_class_name == 'dynamic-dns' | ||
| rescue StandardError | ||
| rescue StandardError => e | ||
| # Store error details for retry decision making |
There was a problem hiding this comment.
The phrase "decision making" should be hyphenated as "decision-making" when used as a compound adjective modifying "retry".
| # Store error details for retry decision making | |
| # Store error details for retry decision-making |
…n-up