test(robot): migrate test_replica_auto_balance_disk_in_pressure #2167
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,7 +4,7 @@ | |
| class Base(ABC): | ||
|
|
||
| @abstractmethod | ||
| def get(self, volume_name, node_name, disk_uuid): | ||
|
There was a problem hiding this comment. 💡 Codebase verification Implementation inconsistency found in derived classes The verification reveals that not all implementing classes have been properly updated with the new
The abstract base class requires a mandatory 🔗 Analysis chainLGTM! Verify implementations in derived classes. The addition of Let's verify that all implementing classes have been updated with the new parameter: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if all implementations of the get() method include the disk_uuid parameter
# Expected: All implementing classes should have the updated signature
# Search for class definitions that inherit from Base
ast-grep --pattern 'class $_ (Base):'
# Search for get method implementations
ast-grep --pattern 'def get(self, volume_name, node_name, $_):'
Length of output: 105 Script: #!/bin/bash
# Let's try a different approach using ripgrep to find implementations
# First, find the base class definition
echo "=== Base class definition ==="
rg -l "class.*Base.*:" e2e/libs/replica/
# Then find all implementations of get method in replica-related files
echo -e "\n=== Get method implementations ==="
rg "def get\(self, volume_name, node_name" -A 1 e2e/libs/replica/
Length of output: 822 |
||
| return NotImplemented | ||
|
|
||
| @abstractmethod | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,12 +10,14 @@ class CRD(Base): | |
| def __init__(self): | ||
| self.obj_api = client.CustomObjectsApi() | ||
|
|
||
| def get(self, volume_name=None, node_name=None, disk_uuid=None): | ||
| label_selector = [] | ||
| if volume_name: | ||
| label_selector.append(f"longhornvolume={volume_name}") | ||
| if node_name: | ||
| label_selector.append(f"longhornnode={node_name}") | ||
| if disk_uuid: | ||
| label_selector.append(f"longhorndiskuuid={disk_uuid}") | ||
|
|
||
| replicas = self.obj_api.list_namespaced_custom_object( | ||
| group="longhorn.io", | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,8 +16,8 @@ def __init__(self): | |
| def delete(self, volume_name="", node_name=""): | ||
| return self.replica.delete(volume_name, node_name) | ||
|
|
||
| def get(self, volume_name, node_name, disk_uuid=None): | ||
| return self.replica.get(volume_name, node_name, disk_uuid) | ||
|
|
||
| def wait_for_rebuilding_start(self, volume_name, node_name): | ||
| return self.replica.wait_for_rebuilding_start(volume_name,node_name) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,7 @@ class Rest(Base): | |
| def __init__(self): | ||
| pass | ||
|
|
||
| def get(self, volume_name, node_name, disk_uuid): | ||
| return NotImplemented | ||
|
Comment on lines
+15
to
16
There was a problem hiding this comment. 💡 Codebase verification Breaking change detected in method signature The verification reveals that while the base class
🔗 Analysis chainVerify implementations of the updated method signature The addition of the 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Find all implementations of the Rest class to verify they implement
# the updated get method signature
# Search for classes that inherit from Rest
ast-grep --pattern 'class $_ (Rest):'
# Search for get method implementations with the new signature
ast-grep --pattern 'def get(self, volume_name, node_name, disk_uuid):'
Length of output: 112 Script: #!/bin/bash
# Let's try a different approach using ripgrep to find implementations
# Search for class definitions inheriting from Rest
rg "class.*\(Rest\)" -l
# Search for all get method implementations in the codebase
rg "def get\(.*volume_name.*node_name.*\):" -l
# Get content of the base class to understand the inheritance
cat e2e/libs/replica/rest.py
Length of output: 3259 Script: #!/bin/bash
# Let's examine the implementation in other files that might be affected
# Check the content of files that might have the get method implementation
cat e2e/libs/replica/crd.py
cat e2e/libs/replica/base.py
cat e2e/libs/replica/replica.py
Length of output: 3895 |
||
|
|
||
| def delete(self, volume_name, node_name): | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |||||||||||||||||||||||||||||
| from utility.utility import logging | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| def create_statefulset(statefulset_name, volume_type, sc_name, size): | ||||||||||||||||||||||||||||||
| filepath = "./templates/workload/statefulset.yaml" | ||||||||||||||||||||||||||||||
| with open(filepath, 'r') as f: | ||||||||||||||||||||||||||||||
| namespace = 'default' | ||||||||||||||||||||||||||||||
|
|
@@ -30,6 +30,10 @@ def create_statefulset(statefulset_name, volume_type, sc_name): | |||||||||||||||||||||||||||||
| if volume_type == 'RWX': | ||||||||||||||||||||||||||||||
| manifest_dict['spec']['volumeClaimTemplates'][0]['spec']['accessModes'][0] = 'ReadWriteMany' | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| # correct request storage size | ||||||||||||||||||||||||||||||
| if size: | ||||||||||||||||||||||||||||||
| manifest_dict['spec']['volumeClaimTemplates'][0]['spec']['resources']['requests']['storage'] = size | ||||||||||||||||||||||||||||||
|
Comment on lines
+33
to
+35
There was a problem hiding this comment. 🛠️ Refactor suggestion Add validation and improve error handling for storage size. The current implementation could be more robust:
Consider this safer implementation: - # correct request storage size
- if size:
- manifest_dict['spec']['volumeClaimTemplates'][0]['spec']['resources']['requests']['storage'] = size
+ # Set PVC storage size if specified
+ if size:
+ # Validate size format
+ if not isinstance(size, str) or not any(size.endswith(unit) for unit in ['Ki', 'Mi', 'Gi', 'Ti']):
+ raise ValueError(f"Invalid storage size format: {size}. Expected format: '<number>Ki|Mi|Gi|Ti'")
+
+ try:
+ template = manifest_dict['spec']['volumeClaimTemplates'][0]
+ template.setdefault('spec', {}).setdefault('resources', {}).setdefault('requests', {})['storage'] = size
+ except (KeyError, IndexError) as e:
+ raise ValueError(f"Invalid manifest structure: {str(e)}")📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| api = client.AppsV1Api() | ||||||||||||||||||||||||||||||
| statefulset = api.create_namespaced_stateful_set( | ||||||||||||||||||||||||||||||
| body=manifest_dict, | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Handle potential exceptions during disk mounting.
The
mount_diskmethod executes system commands that may fail. It's advisable to handle exceptions or check the result of each command to ensure robustness.Apply this diff to handle exceptions:
def mount_disk(self, disk_name, node_name): mount_path = os.path.join(self.DEFAULT_DISK_PATH, disk_name) device_path = os.path.join(self.DEFAULT_VOLUME_PATH, disk_name) cmd = f"mkdir -p {mount_path}" - NodeExec(node_name).issue_cmd(cmd) + if not NodeExec(node_name).issue_cmd(cmd): + raise Exception(f"Failed to create mount path {mount_path} on node {node_name}") cmd = f"mkfs.ext4 {device_path}" - NodeExec(node_name).issue_cmd(cmd) + if not NodeExec(node_name).issue_cmd(cmd): + raise Exception(f"Failed to format device {device_path} on node {node_name}") cmd = f"mount {device_path} {mount_path}" - NodeExec(node_name).issue_cmd(cmd) + if not NodeExec(node_name).issue_cmd(cmd): + raise Exception(f"Failed to mount {device_path} to {mount_path} on node {node_name}") return mount_path📝 Committable suggestion
🧰 Tools
🪛 Ruff
31-31: Local variable
resis assigned to but never usedRemove assignment to unused variable
res(F841)