Skip to content
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.

Sign up for GitHub

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): wait for pvc and volume deleted when cleaning up statefulset #2187

Merged
merged 2 commits into from
Dec 13, 2024
+42 −8
Merged

test(robot): wait for pvc and volume deleted when cleaning up statefulset #2187

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70c989c
test(robot): wait for pvc and volume deleted when cleaning up statefu…
yangchiu Dec 10, 2024
73c16c5
Merge branch 'master' into refine-statefulset-cleanup
yangchiu Dec 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions e2e/libs/keywords/persistentvolumeclaim_keywords.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ def __init__(self):
def cleanup_persistentvolumeclaims(self):
claims = self.claim.list(label_selector=f"{LABEL_TEST}={LABEL_TEST_VALUE}")

logging(f'Cleaning up {len(claims.items)} persistentvolumeclaims')
for claim in claims.items:
logging(f'Cleaning up {len(claims)} persistentvolumeclaims')
for claim in claims:
self.delete_persistentvolumeclaim(claim.metadata.name)

def create_persistentvolumeclaim(self, name, volume_type="RWO", sc_name="longhorn", storage_size="3GiB"):
Expand Down
4 changes: 2 additions & 2 deletions e2e/libs/keywords/volume_keywords.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ def __init__(self):
def cleanup_volumes(self):
volumes = self.volume.list(label_selector=f"{LABEL_TEST}={LABEL_TEST_VALUE}")

logging(f'Cleaning up {len(volumes["items"])} volumes')
for volume in volumes['items']:
logging(f'Cleaning up {len(volumes)} volumes')
for volume in volumes:
self.delete_volume(volume['metadata']['name'])

def create_volume(self, volume_name, size="2Gi", numberOfReplicas=3, frontend="blockdev", migratable=False, dataLocality="disabled", accessMode="RWO", dataEngine="v1", backingImage="", Standby=False, fromBackup=""):
Expand Down
2 changes: 1 addition & 1 deletion e2e/libs/persistentvolumeclaim/crd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def list(self, claim_namespace="default", label_selector=None):
return self.core_v1_api.list_namespaced_persistent_volume_claim(
namespace=claim_namespace,
label_selector=label_selector
)
).items

def set_label(self, claim_name, label_key, label_value, claim_namespace="default"):
for i in range(self.retry_count):
Expand Down
2 changes: 1 addition & 1 deletion e2e/libs/volume/crd.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ def list(self, label_selector=None):
namespace="longhorn-system",
plural="volumes",
label_selector=label_selector
)
)["items"]

def set_annotation(self, volume_name, annotation_key, annotation_value):
# retry conflict error
Expand Down
4 changes: 4 additions & 0 deletions e2e/libs/volume/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ def get_replica_name_on_node(self, volume_name, node_name):
return r.name

def wait_for_replica_count(self, volume_name, node_name, replica_count):
condition_met = False
for i in range(self.retry_count):
running_replica_count = 0
volume = get_longhorn_client().by_id_volume(volume_name)
Expand All @@ -248,10 +249,13 @@ def wait_for_replica_count(self, volume_name, node_name, replica_count):
running_replica_count += 1
logging(f"Waiting for {replica_count if replica_count else ''} replicas for volume {volume_name} running on {node_name if node_name else 'nodes'}, currently it's {running_replica_count} ... ({i})")
if replica_count and running_replica_count == int(replica_count):
condition_met = True
break
elif not replica_count and running_replica_count:
condition_met = True
break
time.sleep(self.retry_interval)
assert condition_met, f"Waiting for {replica_count if replica_count else ''} replicas for volume {volume_name} running on {node_name if node_name else 'nodes'} failed. There are only {running_replica_count} running replicas"
return running_replica_count

def wait_for_replica_rebuilding_complete(self, volume_name, node_name=None):
Expand Down
2 changes: 1 addition & 1 deletion e2e/libs/volume/volume.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ def list(self, label_selector=None):
return self.volume.list(label_selector=label_selector)

def list_names(self, label_selector=None):
return [item['metadata']['name'] for item in self.list(label_selector)['items']]
return [item['metadata']['name'] for item in self.list(label_selector)]

def set_annotation(self, volume_name, annotation_key, annotation_value):
return self.volume.set_annotation(volume_name, annotation_key, annotation_value)
Expand Down
32 changes: 31 additions & 1 deletion e2e/libs/workload/statefulset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
from kubernetes import client
from kubernetes.client.rest import ApiException

from persistentvolumeclaim import PersistentVolumeClaim
from volume import Volume

from utility.constant import LABEL_TEST
from utility.constant import LABEL_TEST_VALUE
from utility.utility import get_retry_count_and_interval
Expand Down Expand Up @@ -79,7 +82,9 @@ def delete_statefulset(name, namespace='default'):
assert e.status == 404

retry_count, retry_interval = get_retry_count_and_interval()
for _ in range(retry_count):
for i in range(retry_count):
time.sleep(retry_interval)
logging(f"Waiting for statefulset {name} deleted ... ({i})")
resp = api.list_namespaced_stateful_set(namespace=namespace)
deleted = True
for item in resp.items:
Expand All @@ -88,9 +93,34 @@ def delete_statefulset(name, namespace='default'):
break
if deleted:
break
assert deleted

claim = PersistentVolumeClaim()
volume_name = None
for i in range(retry_count):
time.sleep(retry_interval)
logging(f"Waiting for pvc of statefulset {name} deleted ... ({i})")
claims = claim.list(label_selector=f"app={name}")
deleted = False
if len(claims) == 0:
deleted = True
break
else:
volume_name = claims[0].spec.volume_name
Comment on lines +98 to +109
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Handle Multiple PersistentVolumeClaims and Associated Volumes

Currently, the code retrieves the volume_name from only the first PersistentVolumeClaim (claims[0].spec.volume_name). If the stateful set has multiple replicas, there may be multiple PVCs and associated volumes. Consider iterating over all claims to handle multiple PVCs and their volumes properly.

Suggested Change:

Modify the loop to handle all PVCs and collect their associated volume names:

 volume_names = []
 for i in range(retry_count):
     time.sleep(retry_interval)
     logging(f"Waiting for PVCs of statefulset {name} to be deleted... ({i})")
     claims = claim.list(label_selector=f"app={name}")
     if len(claims) == 0:
         deleted = True
         break
     else:
-        volume_name = claims[0].spec.volume_name
+        volume_names = [c.spec.volume_name for c in claims]
 assert deleted

Committable suggestion skipped: line range outside the PR's diff.

assert deleted

if volume_name:
volume = Volume()
for i in range(retry_count):
time.sleep(retry_interval)
logging(f"Waiting for volume {volume_name} deleted ... ({i})")
volumes = volume.list(label_selector=f"longhornvolume={volume_name}")
deleted = False
if len(volumes) == 0:
deleted = True
break
assert deleted

Comment on lines +112 to +123
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Handle Multiple Volumes During Deletion

Similar to the PVC deletion logic, the code is currently checking for the deletion of a single volume. If there are multiple volumes associated with the stateful set, they should all be checked and waited upon during the deletion process. Consider modifying the code to handle deletion checks for all relevant volumes.

Suggested Change:

Iterate over all collected volume_names to ensure each is deleted:

-if volume_name:
+if volume_names:
     volume = Volume()
     for volume_name in volume_names:
         for i in range(retry_count):
             time.sleep(retry_interval)
             logging(f"Waiting for volume {volume_name} to be deleted... ({i})")
             volumes = volume.list(label_selector=f"longhornvolume={volume_name}")
             if len(volumes) == 0:
                 deleted = True
                 break
             else:
                 deleted = False
         assert deleted

Committable suggestion skipped: line range outside the PR's diff.


def get_statefulset(name, namespace='default'):
api = client.AppsV1Api()
Expand Down
Loading