Move CSI attach detach test and rewrite script in Python #412
Conversation
alyssa1303
force-pushed
the
alyssa/move-csi-test
branch
from
9e1bd29
to
4464115
Compare
alyssa1303
force-pushed
the
alyssa/move-csi-test
branch
from
4464115
to
ab2ae37
Compare
alyssa1303
force-pushed
the
alyssa/move-csi-test
branch
2 times, most recently
from
101c9a7
to
10e0296
Compare
alyssa1303
force-pushed
the
alyssa/move-csi-test
branch
from
10e0296
to
514b28f
Compare
alyssa1303
changed the title
alyssa1303
force-pushed
the
alyssa/move-csi-test
branch
from
9d8e600
to
5652571
Compare
| p99 = disk_number * 99 // 100 | ||
| return p50, p90, p99, disk_number | ||
|
|
||
| def create_statefulset(namespace, replicas, storage_class): |
There was a problem hiding this comment.
I think this could be inside of KubernetesClient. Therefore, we can re-use it other scenarios
There was a problem hiding this comment.
I'm debating this because lots of spec define here are solely used in this test so not sure if it's worth putting in KubernetesClient. That's why I have a get_app_client() so at least I can re-use the client
| def log_duration(description, start_time, log_file): | ||
| """Log the time duration of an operation.""" | ||
| end_time = datetime.now() | ||
| duration = int((end_time - start_time).total_seconds()) | ||
| with open(log_file, "a") as f: | ||
| f.write(f"{description}: {duration}\n") | ||
| print(f"{description}: {duration}s") | ||
|
|
||
| def wait_for_condition(check_function, target, comparison="gte", interval=1): | ||
| """Wait for a condition using a given check function.""" | ||
| while True: | ||
| current_list = check_function() | ||
| current = len(current_list) | ||
| print(f"Current: {current}, Target: {target}") | ||
| if (comparison == "gte" and current >= target) or (comparison == "lte" and current <= target): | ||
| return current | ||
| time.sleep(interval) |
There was a problem hiding this comment.
- These functions could be in some utils file
- Add to the
wait_for_conditiondescription what thecheck_functionshould return
There was a problem hiding this comment.
Do you mean util file under csi folder? I don't think these are useful for other folders like clusterloader2
There was a problem hiding this comment.
They might still be useful for other tests in the future. Note: this is not critical. Feel free to address it in another PR.
| end_time = datetime.now() | ||
| duration = int((end_time - start_time).total_seconds()) | ||
| with open(log_file, "a") as f: | ||
| f.write(f"{description}: {duration}\n") |
There was a problem hiding this comment.
Lets put quotes ("") in the key and value or add a validation to check if the description does not contains :, what would break the parser
There was a problem hiding this comment.
The description and duration are passed inside the test only though. I don't think that check is necessary
There was a problem hiding this comment.
Yes, but someone can change the description in the future and don't notice that it breaks this logic
alyssa1303
force-pushed
the
alyssa/move-csi-test
branch
from
ed79eb6
to
0632933
Compare
alyssa1303
force-pushed
the
alyssa/move-csi-test
branch
from
25de0d8
to
1c1be6f
Compare
|
|
||
| ns = self.client.create_namespace(name) | ||
| self.assertEqual(ns.metadata.name, mock_read_namespace.return_value.metadata.name) | ||
| mock_read_namespace.assert_called_once_with(name) |
There was a problem hiding this comment.
Instead of it, assert that the "kubernetes.client.CoreV1Api.create_namespace" was not called
| self.assertEqual(len(returned_pods), len(expected_pods)) | ||
| self.assertEqual(returned_pods, expected_pods) |
There was a problem hiding this comment.
minor: you can replace those with only self.assertCountEqual(expected_pods, returned_pods)
Summary