-
Notifications
You must be signed in to change notification settings - Fork 54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(robot): migrate test case test_migration_rollback #2179
test(robot): migrate test case test_migration_rollback #2179
Conversation
Signed-off-by: Yang Chiu <[email protected]>
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on renaming keywords and methods related to volume migration for clarity and consistency. Key changes include renaming existing methods and keywords, adding new ones for rollback functionality, and updating logging messages. The changes span various components, including the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
e2e/libs/engine/crd.py (2)
Line range hint
1-1
: Remove duplicate logging importThe
logging
module is imported twice, which causes a redefinition warning. The second import shadows the first one.Apply this diff to fix the issue:
import logging from kubernetes import client from engine.base import Base -from utility.utility import logging +from utility.utility import logging as utility_loggingAlso applies to: 6-6
🧰 Tools
🪛 Ruff (0.8.0)
6-6: Redefinition of unused
logging
from line 1(F811)
14-15
: Improved logging messages and conditional checksThe changes to logging messages and conditional checks improve code clarity. However, ensure consistent usage of the logging function throughout the file after fixing the duplicate import issue.
After fixing the duplicate import, update all logging calls to use the renamed import:
- logging(f"Getting all engines of {volume_name}") + utility_logging(f"Getting all engines of {volume_name}")Also applies to: 17-17, 20-20, 34-34, 38-38, 44-44, 46-46, 57-57
e2e/libs/volume/base.py (1)
84-92
: LGTM! Consider adding docstrings for better documentation.The abstract methods are well-defined with clear naming conventions. The addition of the rollback method completes the migration lifecycle handling.
Consider adding docstrings to describe the expected behavior and parameters:
@abstractmethod def wait_for_volume_migration_to_be_ready(self, volume_name): + """Wait for volume to be ready for migration. + + Args: + volume_name (str): Name of the volume to check + """ return NotImplementede2e/libs/keywords/volume_keywords.py (1)
271-281
: LGTM! Consider standardizing log message format.The implementation provides good logging and proper delegation to the Volume class. For consistency with other log messages in the file, consider standardizing the format:
- logging(f'Waiting for volume {volume_name} migration to be ready') + logging(f'Waiting for volume {volume_name} to be ready for migration') - logging(f'Waiting for volume {volume_name} migration to node {node_name} complete') + logging(f'Waiting for volume {volume_name} to complete migration to node {node_name}') - logging(f'Waiting for volume {volume_name} migration to rollback to node {node_name}') + logging(f'Waiting for volume {volume_name} to rollback migration to node {node_name}')e2e/libs/volume/crd.py (1)
331-343
: Consider refactoring to reduce code duplicationThe rollback check implementation is nearly identical to the migration completion check. Consider extracting the common logic into a helper method.
+ def _verify_engine_on_node(self, volume_name, node_name, operation_name): + verified = False + for i in range(self.retry_count): + logging(f"Waiting for volume {volume_name} {operation_name} to node {node_name} ({i}) ...") + try: + engines = self.engine.get_engines(volume_name) + verified = len(engines) == 1 and engines[0]['status']['endpoint'] and engines[0]['status']['ownerID'] == node_name + if verified: + break + except Exception as e: + logging(f"Getting volume {volume_name} engines error: {e}") + time.sleep(self.retry_interval) + assert verified def wait_for_volume_migration_complete(self, volume_name, node_name): - complete = False - for i in range(self.retry_count): - logging(f"Waiting for volume {volume_name} migration to node {node_name} complete ({i}) ...") - try: - engines = self.engine.get_engines(volume_name) - complete = len(engines) == 1 and engines[0]['status']['endpoint'] and engines[0]['status']['ownerID'] == node_name - if complete: - break - except Exception as e: - logging(f"Getting volume {volume_name} engines error: {e}") - time.sleep(self.retry_interval) - assert complete + self._verify_engine_on_node(volume_name, node_name, "migration") def wait_for_volume_migration_to_rollback(self, volume_name, node_name): - rollback = False - for i in range(self.retry_count): - logging(f"Waiting for volume {volume_name} migration to rollback to node {node_name} ({i}) ...") - try: - engines = self.engine.get_engines(volume_name) - rollback = len(engines) == 1 and engines[0]['status']['endpoint'] and engines[0]['status']['ownerID'] == node_name - if rollback: - break - except Exception as e: - logging(f"Getting volume {volume_name} engines error: {e}") - time.sleep(self.retry_interval) - assert rollback + self._verify_engine_on_node(volume_name, node_name, "rollback")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
e2e/keywords/volume.resource
(1 hunks)e2e/libs/engine/crd.py
(3 hunks)e2e/libs/keywords/volume_keywords.py
(1 hunks)e2e/libs/volume/base.py
(1 hunks)e2e/libs/volume/crd.py
(2 hunks)e2e/libs/volume/rest.py
(1 hunks)e2e/libs/volume/volume.py
(1 hunks)e2e/tests/regression/test_migration.robot
(1 hunks)e2e/tests/regression/test_v2.robot
(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
e2e/libs/engine/crd.py
6-6: Redefinition of unused logging
from line 1
(F811)
🔇 Additional comments (10)
e2e/tests/regression/test_v2.robot (1)
Line range hint 58-74
: Well-structured test case with comprehensive validation
The new test case is well-designed with:
- Clear documentation of test purpose
- Proper setup with storage class and PVC creation
- Comprehensive validation steps including failure and success scenarios
- Good use of loops for repeated validation
e2e/tests/regression/test_migration.robot (2)
42-46
: Improved clarity in migration confirmation steps
The updates to the test steps improve clarity and add important health and data integrity checks.
48-71
: Well-implemented migration rollback test
The new test case successfully implements the migration rollback scenario with:
- Clear documentation of test steps and expected behavior
- Proper validation of volume state and data integrity
- Logical flow from migration setup to rollback verification
This implementation aligns well with the PR objective to migrate the test_migration_rollback test case.
e2e/libs/volume/volume.py (1)
72-79
: LGTM! Clean implementation of the migration methods.
The implementation correctly delegates to the strategy pattern and maintains consistency with the base class interface.
e2e/keywords/volume.resource (3)
170-172
: Improved keyword naming for better clarity
The renaming from "migration ready" to "migration to be ready" enhances readability and follows proper grammatical structure.
174-177
: Enhanced keyword clarity with better phrasing
The renaming from "migrated to node" to "to migrate to node" improves readability and maintains consistent verb tense.
179-182
: Well-structured addition of rollback functionality
The new keyword follows the established pattern and completes the migration-related functionality by adding rollback support.
e2e/libs/volume/rest.py (1)
74-81
: Verify implementation in concrete classes
These NotImplemented stubs need concrete implementations. Let's verify the implementation exists in the appropriate concrete classes.
✅ Verification successful
Implementation exists in concrete CRD class
The NotImplemented stubs in rest.py
are part of the interface pattern, and concrete implementations are found in:
e2e/libs/volume/crd.py
- Contains full implementations with retry logice2e/libs/volume/base.py
- Defines the abstract interfacee2e/libs/volume/volume.py
- Provides delegation to the concrete implementation
The pattern is correctly implemented with proper inheritance and concrete implementations available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for concrete implementations of these methods
# Test: Search for concrete implementations of the migration methods
rg -l "def (wait_for_volume_migration_to_be_ready|wait_for_volume_migration_complete|wait_for_volume_migration_to_rollback)" --type python
Length of output: 171
Script:
#!/bin/bash
# Search for concrete implementations of the migration methods without file type restriction
rg -l "def (wait_for_volume_migration_to_be_ready|wait_for_volume_migration_complete|wait_for_volume_migration_to_rollback)"
# Also search for files that might contain these implementations
fd -e py | grep -i "volume.*rest"
Length of output: 320
Script:
#!/bin/bash
# Check the actual implementations in the found files
rg "def (wait_for_volume_migration_to_be_ready|wait_for_volume_migration_complete|wait_for_volume_migration_to_rollback)" -A 5 e2e/libs/volume/volume.py e2e/libs/volume/base.py e2e/libs/volume/crd.py e2e/libs/keywords/volume_keywords.py
Length of output: 4774
e2e/libs/volume/crd.py (2)
Line range hint 301-315
: Well-implemented migration readiness check
The implementation correctly verifies migration readiness by checking for two engines with valid endpoints.
317-329
: Robust migration completion verification
The implementation properly verifies migration completion by checking for a single engine with the correct node ownership.
Which issue(s) this PR fixes:
Issue longhorn/longhorn#9909
What this PR does / why we need it:
migrate test case test_migration_rollback
Special notes for your reviewer:
Additional documentation or context
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes