Conversation
WalkthroughThis update introduces new features and refactors for VMware vCenter orchestration scripts, including enhanced event handling, improved YAML migration plan structure, new server listing functionality, and more precise error handling. The documentation is extensively rewritten for clarity, and several scripts and modules are updated to align with the new configuration and workflow. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BashScript
participant PythonScript
participant VMwareConnection
participant vCenter
participant Redis
User->>BashScript: Run migration_plan.sh / restart_plan.sh
BashScript->>BashScript: Check for running processes / delays
BashScript->>PythonScript: Run migration_plan.py or restart_plan.py
PythonScript->>VMwareConnection: Connect to vCenter
PythonScript->>vCenter: Retrieve servers/VMs
PythonScript->>Redis: Push event (shutdown, migration, start, etc.)
PythonScript->>VMwareConnection: Perform action (stop VM, migrate, start VM, etc.)
PythonScript->>Redis: Update event state
PythonScript->>VMwareConnection: Disconnect
PythonScript->>User: Output result
sequenceDiagram
participant User
participant list_server.sh
participant list_server.py
participant VMwareConnection
participant vCenter
User->>list_server.sh: Run with connection parameters
list_server.sh->>list_server.py: Validate and execute
list_server.py->>VMwareConnection: Connect to vCenter
VMwareConnection->>vCenter: Retrieve all hosts
VMwareConnection->>list_server.py: Return hosts
list_server.py->>User: Output JSON server list
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
migration_plan.sh (1)
5-9: Improve process detection pattern specificity.The current
pgreppattern"python.*migration_plan\.py$"may match unintended processes or miss edge cases. Consider using more specific patterns or additional verification.-PID=$(pgrep -f "python.*migration_plan\.py$") +PID=$(pgrep -f "python.*migration_plan\.py$" | head -1)Also consider adding the current script's PID to avoid false positives if the script name contains similar patterns.
plans/migration-example.yml (1)
7-9: Good centralization of grace period configuration.The new
upssection centralizes grace period settings, which improves maintainability and clarity. The structure aligns well with the described parser changes.Consider adding comments to clarify the units (presumably seconds):
ups: + # Grace periods in seconds shutdownGrace: 60 restartGrace: 60list_server.py (1)
34-34: Fix copy-paste error in CLI description.The description mentions "Lister les VM d'un serveur" (List VMs of a server) but should describe server listing instead.
- parser = ArgumentParser(description="Lister les VM d'un serveur") + parser = ArgumentParser(description="Lister les serveurs d'un vCenter")list_server.sh (1)
35-39: Consider security improvement for command execution.While the current implementation works, using
evalwith user input could be a security concern. Consider using arrays for safer command construction.-CMD="python list_server.py --ip \"$IP\" --user \"$USER\" --password \"$PASSWORD\"" -if [[ -n "$PORT" ]]; then - CMD+=" --port \"$PORT\"" -fi -eval "$CMD" +CMD_ARGS=("python" "list_server.py" "--ip" "$IP" "--user" "$USER" "--password" "$PASSWORD") +if [[ -n "$PORT" ]]; then + CMD_ARGS+=("--port" "$PORT") +fi +"${CMD_ARGS[@]}"vm_start.py (1)
50-50: Consider using actual VM name instead of MOID.The function now accepts a
nameparameter for better logging, butmoidis being passed instead of the actual VM name. Consider retrieving the VM name for more meaningful logging.-return vm_start(vm, moid) +return vm_start(vm, vm.name if vm else moid)data_retriever/vm_ware_connection.py (1)
66-89: LGTM! Well-implemented host collection method with optimization opportunity.The new
get_all_hostsmethod follows the same pattern asget_all_vmsand properly handles recursive folder traversal. However, the conditional logic forComputeResourceandClusterComputeResourcecan be simplified.Apply this diff to address the static analysis hint:
- if isinstance(entity, vim.ComputeResource): - hosts.extend(entity.host) - elif isinstance(entity, vim.ClusterComputeResource): - hosts.extend(entity.host) + if isinstance(entity, (vim.ComputeResource, vim.ClusterComputeResource)): + hosts.extend(entity.host)vm_migration.py (1)
31-32: Breaking change: Explicit VM power state management requiredThe function now rejects migration of powered-on VMs instead of automatically powering them off. This is safer but may break existing workflows that relied on automatic power-off behavior.
Consider documenting this breaking change prominently in release notes or migration guides.
data_retriever/yaml_parser.py (1)
63-63: Use dictionary .get() method for cleaner codeStatic analysis correctly suggests using
.get()for optional dictionary keys.- port=data['vCenter']['port'] if 'port' in data['vCenter'] else 443, + port=data['vCenter'].get('port', 443),- destination = server['server']['destination'] if 'destination' in server['server'] else None + destination = server['server'].get('destination')Also applies to: 74-74
README.md (1)
13-13: Minor language improvementsConsider these style improvements for better readability.
-- Store migration events in Redis in order to roll back the operations afterwards. +- Store migration events in Redis to roll back the operations afterward.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.md(1 hunks)data_retriever/dto.py(1 hunks)data_retriever/event_queue.py(1 hunks)data_retriever/migration_event.py(1 hunks)data_retriever/vm_ware_connection.py(2 hunks)data_retriever/yaml_parser.py(2 hunks)list_server.py(1 hunks)list_server.sh(1 hunks)list_vm.py(2 hunks)migration_plan.py(4 hunks)migration_plan.sh(1 hunks)plans/migration-example.yml(2 hunks)restart_plan.py(2 hunks)restart_plan.sh(1 hunks)server_info.py(2 hunks)server_metrics.py(2 hunks)vm_metrics.py(2 hunks)vm_migration.py(5 hunks)vm_start.py(3 hunks)vm_stop.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
list_server.py (2)
data_retriever/dto.py (3)
result_message(14-28)output(5-11)servers_list_info(101-112)data_retriever/vm_ware_connection.py (4)
VMwareConnection(6-142)connect(11-29)get_all_hosts(66-89)disconnect(31-36)
restart_plan.py (7)
data_retriever/migration_event.py (4)
VMMigrationEvent(6-8)VMShutdownEvent(11-13)ServerShutdownEvent(21-25)VMStartedEvent(16-18)data_retriever/vm_ware_connection.py (4)
VMwareConnection(6-142)connect(11-29)get_vm(91-118)get_host_system(120-142)data_retriever/yaml_parser.py (3)
VCenter(28-32)load_plan_from_yaml(40-100)UpsGrace(35-37)vm_migration.py (1)
vm_migration(11-46)vm_start.py (1)
vm_start(9-30)vm_stop.py (1)
vm_stop(9-29)data_retriever/event_queue.py (4)
EventQueue(9-74)start_restart(61-66)get_event_list(28-38)finish_restart(68-74)
vm_migration.py (3)
vm_start.py (1)
vm_start(9-30)vm_stop.py (1)
vm_stop(9-29)data_retriever/dto.py (1)
result_message(14-28)
vm_start.py (1)
data_retriever/dto.py (1)
result_message(14-28)
🪛 Ruff (0.12.2)
data_retriever/vm_ware_connection.py
75-78: Combine if branches using logical or operator
Combine if branches
(SIM114)
data_retriever/yaml_parser.py
63-63: Use data['vCenter'].get('port', 443) instead of an if block
Replace with data['vCenter'].get('port', 443)
(SIM401)
74-74: Use server['server'].get('destination', None) instead of an if block
Replace with server['server'].get('destination', None)
(SIM401)
🪛 LanguageTool
README.md
[style] ~13-~13: Consider a more concise word here.
Context: ...YAML. - Store migration events in Redis in order to roll back the operations afterwards. - ...
(IN_ORDER_TO_PREMIUM)
[locale-violation] ~13-~13: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects.
Context: ...is in order to roll back the operations afterwards. - Convenience shell scripts that wrap ...
(AFTERWARDS_US)
🪛 markdownlint-cli2 (0.17.2)
README.md
18-18: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (43)
data_retriever/event_queue.py (1)
40-45: Well-implemented method following established patterns.The
grace_shutdownmethod correctly follows the same pattern as other state management methods in the class. The error handling is consistent, and the state value is descriptive and appropriate for the shutdown grace period workflow.vm_stop.py (2)
34-40: Good documentation clarification.The updated docstring correctly specifies that the connection is to a vCenter server, improving clarity for users and aligning with the broader standardization effort across the codebase.
62-65: Consistent CLI help text improvements.The updated help text correctly specifies vCenter parameters, providing better guidance to users about the expected connection target.
plans/migration-example.yml (1)
27-30: Simplified VM order structure improves readability.Moving
vmOrderdirectly under the server configuration eliminates unnecessary nesting and makes the configuration cleaner and more intuitive.server_info.py (2)
10-10: Consistent documentation improvements.The updated docstring correctly specifies vCenter connections, maintaining consistency with the broader standardization effort across the VMware orchestration scripts.
Also applies to: 13-16
40-43: Clear CLI help text updates.The updated help text provides better guidance to users by explicitly specifying that connection parameters are for the vCenter server.
list_vm.py (2)
12-15: LGTM: Clear documentation improvement.The docstring clarification specifying "VCenter" instead of generic server improves API documentation consistency across the codebase.
35-38: LGTM: Consistent CLI help text improvements.The French help text clarifications align with the codebase-wide standardization on vCenter-specific connection parameters.
restart_plan.sh (3)
5-9: LGTM: Good concurrency protection.The process check prevents multiple instances from running simultaneously, which is essential for restart plan operations.
11-15: LGTM: Proper configuration validation.The config file existence check ensures graceful failure if the migration plan is missing.
23-24: LGTM: Configuration-driven grace period.The sleep mechanism using the extracted grace period value provides configurable timing control.
data_retriever/dto.py (2)
101-112: LGTM: Well-structured server listing function.The function correctly follows the established pattern from
vms_list_info, reusing the existingserver_infofunction for consistency.
125-125: LGTM: Consistent data enrichment.Adding the managed object ID (
moid) to server data provides consistency with VM data structures and enables unique identification of hosts.list_server.py (2)
8-30: LGTM: Well-structured server listing implementation.The function correctly follows the established pattern from
list_vm.py, with proper error handling and resource cleanup.
35-38: LGTM: Consistent CLI parameter definitions.The CLI arguments follow the established pattern with proper French help text and consistent defaults.
server_metrics.py (2)
10-10: LGTM: Clear documentation improvement.The docstring clarification specifying "VCenter" improves API documentation consistency and aligns with the broader refactoring effort.
Also applies to: 13-16
40-43: LGTM: Consistent CLI help text improvements.The French help text clarifications maintain consistency with other modules in the codebase.
vm_metrics.py (2)
10-16: LGTM! Documentation clarity improvements.The updated docstring clearly specifies that connections are to vCenter servers, which aligns with the broader refactoring effort to clarify VMware infrastructure terminology throughout the codebase.
39-43: LGTM! Consistent CLI help text improvements.The argument parser help texts have been updated to consistently specify "vCenter" connections, maintaining consistency with the docstring changes and improving user experience.
data_retriever/migration_event.py (3)
13-13: LGTM! Logical addition of server context.Adding
server_moidtoVMShutdownEventprovides valuable context about which server the VM was running on, which is useful for tracking and orchestration purposes.
15-18: LGTM! Well-structured new event type.The new
VMStartedEventdataclass follows the established pattern and structure of existing event types, maintaining consistency in the codebase.
28-28: LGTM! Proper event type registration.The
EVENT_CLASSESdictionary is correctly updated to include the newVMStartedEvent, ensuring proper serialization and deserialization support.list_server.sh (3)
5-14: LGTM! Solid argument parsing implementation.The argument parsing loop correctly handles the required parameters and follows bash best practices with proper shift operations.
16-20: LGTM! Proper validation of required parameters.The validation correctly checks for required parameters and provides clear error messages with usage information.
22-33: LGTM! Comprehensive virtual environment validation.The script performs thorough checks for virtual environment existence, activation success, and validates that the correct environment is activated. The error messages are clear and helpful.
vm_start.py (4)
26-26: LGTM! Improved success message with dynamic VM name.The success message now includes the VM name, providing better feedback to users about which specific VM was started.
27-28: LGTM! Enhanced error handling for host-related issues.The specific exception handling for host-related errors (
NoCompatibleHost,InvalidHostState,HostNotConnected) provides better error categorization and user-friendly messages.
38-41: LGTM! Documentation clarity improvements.The updated docstring clearly specifies vCenter connections, maintaining consistency with the broader refactoring effort.
63-66: LGTM! Consistent CLI help text improvements.The argument parser help texts have been updated to consistently specify "vCenter" connections, improving user experience.
data_retriever/vm_ware_connection.py (2)
42-51: LGTM! Improved code organization with helper function.The refactoring to use a helper function
collect_vms_from_folderimproves code readability and maintainability by separating the recursive logic.
63-63: LGTM! Consistent usage of refactored helper function.The main VM collection logic now properly uses the new helper function, maintaining consistency.
restart_plan.py (4)
14-22: Good improvement: Configurable grace periodsThe refactoring to use
UpsGraceparameter instead of hardcoded delays improves flexibility and maintainability.
44-50: Correct rollback logic for migrated VMsThe
VMStartedEventhandler correctly stops VMs that were started after migration during the shutdown process, properly restoring the original state.
57-58: Improved error handling for event queue cleanupMoving
finish_restart()inside the try block ensures the restart is only marked as complete on success, preventing incorrect state updates on failure.
71-73: Proper validation of required configurationGood defensive programming by checking both
v_centerandups_graceare loaded before proceeding with restart.vm_migration.py (2)
43-44: Good addition: Specific exception handlingThe new handler for
InvalidHostStateprovides clearer error messages for host connectivity issues.
50-58: Clear documentation improvementsThe updated docstrings correctly clarify that connections are made to vCenter servers, improving accuracy and reducing confusion.
data_retriever/yaml_parser.py (2)
34-38: Well-designed grace period abstractionThe
UpsGracedataclass provides a clean abstraction for grace period configuration, improving code organization.
94-94: Good simplification of VM order structureThe simplified
vm_orderlist aligns well with the new global grace period configuration approach.migration_plan.py (3)
50-57: Well-implemented grace period logicThe grace period implementation properly signals the shutdown state and waits for potential power restoration before proceeding with VM shutdown.
77-95: Excellent sequential VM lifecycle managementThe new stop→migrate→start flow with event tracking for each step provides clear state management and enables proper rollback operations.
105-105: Consistent error handling improvementLike in
restart_plan.py, movingfinish_shutdown()to the try block ensures shutdown is only marked complete on success.README.md (1)
1-82: Excellent documentation overhaulThe comprehensive rewrite provides clear guidance on features, installation, and usage. The examples are particularly helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
migration_plan.sh (1)
11-30: Excellent process termination implementation.The graceful termination logic properly implements the SIGTERM → wait → SIGKILL pattern for both restart_plan.py and restart_plan.sh. This addresses the previous review concern about robust process termination.
README.md (1)
85-117: YAML example correctly updated to match new structure.The documentation example now properly reflects the new YAML format with the
upssection and simplifiedvmOrderstructure, addressing the previous review concern.
🧹 Nitpick comments (3)
data_retriever/yaml_parser.py (1)
70-70: Consider using dict.get() for cleaner code.The static analysis tool suggests a more concise approach for optional field access.
- destination = server['server']['destination'] if 'destination' in server['server'] else None + destination = server['server'].get('destination', None)README.md (2)
13-13: Consider minor style improvements.The documentation is comprehensive and well-written. A few minor style improvements could enhance readability.
-Store migration events in Redis in order to roll back the operations afterwards. +Store migration events in Redis to roll back the operations afterward.
18-25: Add language specification to fenced code block.The directory structure code block should specify the language for better syntax highlighting.
-``` +```text . ├── data_retriever/ # Connection helpers and data transfer objects ├── plans/ # Example migration plan in YAML ├── *.py # Command line utilities (server_start.py, vm_migration.py, ...) ├── *.sh # Shell wrappers for the Python tools └── requirements.txt # Python dependencies</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between cc3cb5f53a5b51d7e92bd507d20dc3a86eb960fe and b38aa01b551c817f4c7ee5541c1d076c1ffaa013. </details> <details> <summary>📒 Files selected for processing (7)</summary> * `README.md` (1 hunks) * `data_retriever/yaml_parser.py` (3 hunks) * `migration_plan.py` (4 hunks) * `migration_plan.sh` (1 hunks) * `restart_plan.py` (2 hunks) * `restart_plan.sh` (1 hunks) * `vm_migration.py` (5 hunks) </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary> * restart_plan.sh * vm_migration.py * migration_plan.py </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>README.md</summary> [style] ~13-~13: Consider a more concise word here. Context: ...YAML. - Store migration events in Redis in order to roll back the operations afterwards. - ... (IN_ORDER_TO_PREMIUM) --- [locale-violation] ~13-~13: In American English, ‘afterward’ is the preferred variant. ‘Afterwards’ is more commonly used in British English and other dialects. Context: ...is in order to roll back the operations afterwards. - Convenience shell scripts that wrap ... (AFTERWARDS_US) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>README.md</summary> 18-18: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> <details> <summary>🪛 Ruff (0.12.2)</summary> <details> <summary>data_retriever/yaml_parser.py</summary> 70-70: Use `server['server'].get('destination', None)` instead of an `if` block Replace with `server['server'].get('destination', None)` (SIM401) </details> </details> </details> <details> <summary>🔇 Additional comments (9)</summary><blockquote> <details> <summary>migration_plan.sh (1)</summary> `5-9`: **Well-implemented concurrency control.** The addition of this check prevents multiple migration plans from running simultaneously, which is essential for maintaining system consistency. </details> <details> <summary>restart_plan.py (4)</summary> `14-21`: **Function signature correctly updated for grace period configuration.** The addition of the `ups_grace` parameter and its usage to replace hardcoded delays improves configurability and aligns with the new YAML structure. --- `30-50`: **Event handling logic properly enhanced.** The reordered event handling and addition of `VMStartedEvent` support creates a more comprehensive restart workflow. The consistent pattern of waiting for host connection before VM operations is well implemented. --- `57-58`: **Improved completion logic placement.** Moving the completion logic inside the try block ensures it only executes when all events are successfully processed, which is more robust than the previous finally block placement. --- `71-75`: **Main function properly updated for new parameter structure.** The updated main function correctly loads and passes the `ups_grace` parameter, maintaining consistency with the new YAML structure. </details> <details> <summary>data_retriever/yaml_parser.py (3)</summary> `34-37`: **Well-designed UpsGrace dataclass.** The new `UpsGrace` dataclass properly centralizes grace period configuration, improving maintainability and reducing code duplication across the codebase. --- `52-53`: **Improved error handling approach.** Allowing exceptions to propagate instead of returning None values is actually better than the previous approach, as it ensures errors are handled explicitly by callers rather than potentially being silently ignored. --- `90-90`: **Simplified VM order parsing is well-implemented.** The direct parsing of `vmOrder` into a list of strings is much cleaner than the previous nested structure and aligns well with the new YAML format. </details> <details> <summary>README.md (1)</summary> `1-125`: **Excellent documentation overhaul.** The complete rewrite transforms the README into a comprehensive user guide that clearly explains the project's purpose, features, installation, and usage. The structured approach with clear sections makes it very accessible to new users. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
1 participant
Summary by CodeRabbit
New Features
Improvements
Documentation
Configuration