-
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_replica_rebuild_per_volume_limit #2159
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -128,6 +128,13 @@ def delete_replica_on_nodes(self, volume_name, replica_locality): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logging(f"Deleting volume {volume_name}'s replica on node {node_id}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.volume.delete_replica(volume_name, node_id) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def delete_replicas(self, volume_name, count): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
replica_list = self.replica.get(volume_name, node_name="") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
replica_names = [replica['metadata']['name'] for replica in replica_list] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for i in range(int(count)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logging(f"Deleting volume {volume_name} replica volume {replica_names[i]}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.volume.delete_replica_by_name(volume_name, replica_names[i]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+131
to
+137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add input validation and error handling. The method needs additional safeguards:
Consider this implementation: def delete_replicas(self, volume_name, count):
+ try:
+ count = int(count)
+ if count <= 0:
+ raise ValueError("Count must be positive")
+
replica_list = self.replica.get(volume_name, node_name="")
replica_names = [replica['metadata']['name'] for replica in replica_list]
+ if count > len(replica_names):
+ raise ValueError(f"Cannot delete {count} replicas, only {len(replica_names)} available")
+
for i in range(int(count)):
logging(f"Deleting volume {volume_name} replica volume {replica_names[i]}")
- self.volume.delete_replica_by_name(volume_name, replica_names[i])
+ try:
+ self.volume.delete_replica_by_name(volume_name, replica_names[i])
+ except Exception as e:
+ logging(f"Failed to delete replica {replica_names[i]}: {str(e)}")
+ raise
+ except ValueError as e:
+ logging(f"Invalid input: {str(e)}")
+ raise
+ except Exception as e:
+ logging(f"Failed to delete replicas: {str(e)}")
+ raise 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def set_annotation(self, volume_name, annotation_key, annotation_value): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.volume.set_annotation(volume_name, annotation_key, annotation_value) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -152,11 +159,7 @@ def wait_for_replica_rebuilding_to_complete_on_node(self, volume_name, replica_l | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.volume.wait_for_replica_rebuilding_complete(volume_name, node_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def wait_for_replica_rebuilding_to_complete(self, volume_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
for node_name in self.node.list_node_names_by_role("worker"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if self.node.is_node_schedulable(node_name) == "False": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logging(f"Waiting for volume {volume_name}'s replica on node {node_name} rebuilding completed") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.volume.wait_for_replica_rebuilding_complete(volume_name, node_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.volume.wait_for_replica_rebuilding_complete(volume_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
async def only_one_replica_rebuilding_will_start_at_a_time_on_node(self, volume_name_0, volume_name_1, replica_locality): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -203,6 +206,21 @@ async def wait_for_both_replica_rebuildings(): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
assert self.volume.is_replica_rebuilding_in_progress(volume_name_0, node_id) and self.volume.is_replica_rebuilding_in_progress(volume_name_1, node_id), \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
f"Expect {volume_name_0} and {volume_name_1} replica rebuilding at the same time" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
async def only_one_replica_rebuilding_will_start_at_a_time(self, volume_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
async def wait_for_replica_rebuilding(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
tasks = [ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
asyncio.create_task(self.volume.wait_for_replica_rebuilding_start(volume_name), name=volume_name), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
done, pending = await asyncio.wait(tasks, return_when=asyncio.ALL_COMPLETED) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logging(f"Observed {done.pop().get_name()} started replica rebuilding") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await wait_for_replica_rebuilding() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
assert self.volume.is_replica_rebuilding_in_progress(volume_name), \ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
f"Expect {volume_name} replica rebuilding in progress" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def crash_replica_processes(self, volume_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.volume.crash_replica_processes(volume_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -420,6 +420,23 @@ def delete_replica(self, volume_name, node_name): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
name=replica_list['items'][0]['metadata']['name'] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def delete_replica_by_name(self, volume_name, replica_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
replica = self.obj_api.get_namespaced_custom_object( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
group="longhorn.io", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
version="v1beta2", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
namespace="longhorn-system", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
plural="replicas", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
name=replica_name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
logging(f"Deleting replica {replica['metadata']['name']}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self.obj_api.delete_namespaced_custom_object( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
group="longhorn.io", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
version="v1beta2", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
namespace="longhorn-system", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
plural="replicas", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
name=replica['metadata']['name'] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+423
to
+439
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling and optimize replica deletion. The implementation could be improved in the following ways:
Consider this implementation: def delete_replica_by_name(self, volume_name, replica_name):
- replica = self.obj_api.get_namespaced_custom_object(
- group="longhorn.io",
- version="v1beta2",
- namespace="longhorn-system",
- plural="replicas",
- name=replica_name
- )
- logging(f"Deleting replica {replica['metadata']['name']}")
- self.obj_api.delete_namespaced_custom_object(
- group="longhorn.io",
- version="v1beta2",
- namespace="longhorn-system",
- plural="replicas",
- name=replica['metadata']['name']
- )
+ try:
+ logging(f"Deleting replica {replica_name}")
+ self.obj_api.delete_namespaced_custom_object(
+ group="longhorn.io",
+ version="v1beta2",
+ namespace="longhorn-system",
+ plural="replicas",
+ name=replica_name
+ )
+ except ApiException as e:
+ if e.status == 404:
+ logging(f"Replica {replica_name} not found")
+ else:
+ raise e 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def wait_for_replica_rebuilding_start(self, volume_name, node_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Rest().wait_for_replica_rebuilding_start(volume_name, node_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -432,7 +449,7 @@ def crash_replica_processes(self, volume_name): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def crash_node_replica_process(self, volume_name, node_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Rest().crash_node_replica_process(volume_name, node_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def wait_for_replica_rebuilding_complete(self, volume_name, node_name): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def wait_for_replica_rebuilding_complete(self, volume_name, node_name=None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Rest().wait_for_replica_rebuilding_complete(volume_name, node_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def check_data_checksum(self, volume_name, data_id): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,10 +101,13 @@ def keep_writing_data(self, volume_name): | |
def delete_replica(self, volume_name, node_name): | ||
return self.volume.delete_replica(volume_name, node_name) | ||
|
||
def wait_for_replica_rebuilding_start(self, volume_name, node_name): | ||
def delete_replica_by_name(self, volume_name, replica_name): | ||
return self.volume.delete_replica_by_name(volume_name, replica_name) | ||
|
||
def wait_for_replica_rebuilding_start(self, volume_name, node_name=None): | ||
return self.volume.wait_for_replica_rebuilding_start(volume_name, node_name) | ||
|
||
def is_replica_rebuilding_in_progress(self, volume_name, node_name): | ||
def is_replica_rebuilding_in_progress(self, volume_name, node_name=None): | ||
return self.volume.is_replica_rebuilding_in_progress(volume_name, node_name) | ||
|
||
def crash_replica_processes(self, volume_name): | ||
|
@@ -122,7 +125,7 @@ def wait_for_replica_running(self, volume_name, node_name): | |
def get_replica_name_on_node(self, volume_name, node_name): | ||
return self.volume.get_replica_name_on_node(volume_name, node_name) | ||
|
||
def wait_for_replica_rebuilding_complete(self, volume_name, node_name): | ||
def wait_for_replica_rebuilding_complete(self, volume_name, node_name=None): | ||
return self.volume.wait_for_replica_rebuilding_complete(volume_name, node_name) | ||
Comment on lines
+128
to
129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Inconsistency found in node_name parameter handling The verification reveals inconsistencies in the node_name parameter handling across implementations:
The base class and CRD implementation need to be updated to maintain consistency with the optional node_name parameter pattern. 🔗 Analysis chainAdd docstring and verify implementation consistency. Add a docstring to explain the behavior with and without node_name parameter. def wait_for_replica_rebuilding_complete(self, volume_name, node_name=None):
+ """Wait for replica rebuilding to complete.
+
+ Args:
+ volume_name: Name of the volume
+ node_name: Optional. If provided, wait for rebuilding completion on specific node.
+ If None, wait for any replica rebuilding completion.
+ """
return self.volume.wait_for_replica_rebuilding_complete(volume_name, node_name) Let's verify that the underlying implementations in CRD and Rest classes handle the optional node_name consistently: 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if CRD and Rest implementations handle optional node_name consistently
# Test: Search for the implementation of these methods in CRD and Rest classes
echo "Checking implementations in CRD and Rest classes..."
rg -A 10 "def (wait_for_replica_rebuilding_start|is_replica_rebuilding_in_progress|wait_for_replica_rebuilding_complete)" "e2e/libs/volume/"
Length of output: 7703 |
||
|
||
def check_data_checksum(self, volume_name, data_id): | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,51 @@ | ||||||||||||||
*** Settings *** | ||||||||||||||
Documentation Replica Test Cases | ||||||||||||||
|
||||||||||||||
Test Tags regression | ||||||||||||||
|
||||||||||||||
Resource ../keywords/common.resource | ||||||||||||||
Resource ../keywords/volume.resource | ||||||||||||||
Resource ../keywords/setting.resource | ||||||||||||||
Resource ../keywords/deployment.resource | ||||||||||||||
Resource ../keywords/persistentvolumeclaim.resource | ||||||||||||||
Resource ../keywords/workload.resource | ||||||||||||||
|
||||||||||||||
Test Setup Set test environment | ||||||||||||||
Test Teardown Cleanup test resources | ||||||||||||||
|
||||||||||||||
*** Variables *** | ||||||||||||||
${LOOP_COUNT} 1 | ||||||||||||||
${RETRY_COUNT} 300 | ||||||||||||||
${RETRY_INTERVAL} 1 | ||||||||||||||
${DATA_ENGINE} v1 | ||||||||||||||
|
||||||||||||||
*** Test Cases *** | ||||||||||||||
Test Replica Rebuilding Per Volume Limit | ||||||||||||||
[Tags] coretest | ||||||||||||||
[Documentation] Test the volume always only have one replica scheduled for rebuild | ||||||||||||||
... | ||||||||||||||
... 1. Set soft anti-affinity to `true`. | ||||||||||||||
... 2. Create a volume with 1 replica. | ||||||||||||||
... 3. Attach the volume and write a few hundreds MB data to it. | ||||||||||||||
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Documentation inconsistency with implementation. The documentation states "Create a volume with 1 replica" but the implementation creates a volume with 3 replicas. Either update the documentation or adjust the implementation to match: - ... 2. Create a volume with 1 replica.
+ ... 2. Create a volume with 3 replicas. 📝 Committable suggestion
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with this comment for making test description and test code more consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chriscchien, glad to hear that you agree. Updating the documentation step to match the implementation will ensure consistency. Here's the updated step: ... 2. Create a volume with 3 replicas. Let me know if you need any further assistance. |
||||||||||||||
... 4. Scale the volume replica to 5. | ||||||||||||||
... 5. Monitor the volume replica list to make sure there should be only 1 replica in WO state. | ||||||||||||||
... 6. Wait for the volume to complete rebuilding. Then remove 4 of the 5 replicas. | ||||||||||||||
... 7. Monitoring the volume replica list again. | ||||||||||||||
... 8. Once the rebuild was completed again, verify the data checksum. | ||||||||||||||
Given Set setting replica-soft-anti-affinity to true | ||||||||||||||
And Create volume 0 with numberOfReplicas=3 dataEngine=${DATA_ENGINE} | ||||||||||||||
And Attach volume 0 | ||||||||||||||
And Wait for volume 0 healthy | ||||||||||||||
And Write data to volume 0 | ||||||||||||||
|
||||||||||||||
Comment on lines
+39
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add validation for data write operation. The test writes data but doesn't verify the write operation's success before proceeding with replica operations. And Write data to volume 0
+And Verify data write succeeded on volume 0 📝 Committable suggestion
Suggested change
|
||||||||||||||
When Update volume 0 replica count to 5 | ||||||||||||||
Then Only one replica rebuilding will start at a time for volume 0 | ||||||||||||||
And Monitor only one replica rebuilding will start at a time for volume 0 | ||||||||||||||
And Wait until volume 0 replicas rebuilding completed | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to change the keyword name There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chriscchien Could you suggest an appropriate name? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about Then Only one replica rebuilding will start at a time for volume 0
And Wait until volume 0 replicas rebuilding completed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @chriscchien How about adding another step
This aligns with the description in the test skeleton. |
||||||||||||||
|
||||||||||||||
When Delete 4 replicas of volume 0 | ||||||||||||||
Then Only one replica rebuilding will start at a time for volume 0 | ||||||||||||||
And Monitor only one replica rebuilding will start at a time for volume 0 | ||||||||||||||
And Wait until volume 0 replicas rebuilding completed | ||||||||||||||
And Wait for volume 0 healthy | ||||||||||||||
And Check volume 0 data is intact |
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.
Keyword implementation may not match its intended purpose.
The keyword name suggests monitoring that only one replica rebuilds at a time, but the implementation only waits for rebuilding completion. This might not effectively validate the "one at a time" constraint.
Consider implementing proper monitoring logic: