-
Notifications
You must be signed in to change notification settings - Fork 23
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
Issue/662 default storage class #745
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
…_readwriteonce() Signed-off-by: Katharina Trentau <[email protected]>
…_readwriteonce() Signed-off-by: Katharina Trentau <[email protected]>
be2c002
to
7e9061a
Compare
Signed-off-by: Katharina Trentau <[email protected]>
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.
First round of review done, see inline comments.
Some general observations:
If the connection to the cluster fails, the script tries to continue anyway with the all the steps, which it shouldn't do.
The script still doesn't check if a provisioner such as cinder-csi is used.
@@ -1,8 +1,11 @@ | |||
aiohttp | |||
click | |||
fabric |
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.
fabric |
This seems like a leftover from the rebase. The fabric
dependency has been removed in main
via 18a3dae.
Please regenerate the requirements.txt
again afterwards.
kubernetes_asyncio | ||
python-dateutil | ||
PyYAML | ||
openstacksdk | ||
requests | ||
tomli | ||
|
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.
def cleanup(): | ||
logger.debug(f"delete pod:{pod_name}") | ||
api_response = k8s_api_instance.delete_namespaced_pod(pod_name, namespace) | ||
logger.debug(f"delete pvc:{pvc_name}") | ||
api_response = k8s_api_instance.delete_namespaced_persistent_volume_claim( | ||
pvc_name, namespace | ||
) |
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.
- Please move the
cleanup()
function to the module level and use function arguments - The
api_response
is not checked. If that's intended, leave it out. But it might be better to check it and report to the user. - Also note that, during cleanup, exceptions could occur, which are currently not handled.
Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py
Outdated
Show resolved
Hide resolved
Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py
Outdated
Show resolved
Hide resolved
@@ -172,28 +180,24 @@ def check_default_persistentvolumeclaim_readwriteonce(k8s_api_instance, storage_ | |||
if pv["status"]["phase"] != "Bound": | |||
raise SCSTestException( | |||
"Not able to bind pv to pvc", | |||
cleanup(), |
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.
Same as above.
return_code=41, | ||
) | ||
|
||
if "ReadWriteOnce" not in pv["spec"]["accessModes"]: | ||
raise SCSTestException( | ||
"access mode 'ReadWriteOnce' is not supported", | ||
cleanup(), |
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.
Same as above.
api_response = k8s_api_instance.delete_namespaced_persistent_volume_claim( | ||
pvc_name, namespace | ||
) | ||
cleanup() |
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.
There are four places were a cleanup()
call is inserted, but there should be one central place.
My suggestion would be to make use of the try ... finally
construct, doing the cleanup in the finally block.
Or you could build a context manager that supports the with
statement. An example of this can be found with the TestEnvironment
class in the IaaS entropy check (usage).
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.
thanks, i'll checkout the context manager, that seems to be pretty neat. i also intend to make a precheck, whether the environment is cleaned up or whether, there are left overs
directory_path = os.path.dirname(f"./{test_name}.result.yaml") | ||
os.makedirs(directory_path, exist_ok=True) | ||
|
||
with open(f"./{test_name}.result.yaml", "w") as file: | ||
yaml.dump(result_file, file) | ||
with open(f"./{test_name}.result.yaml", "w") as file: | ||
yaml.dump(result_file, 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.
This still creates a file hierarchy replicating the absolute path to the script file, e.g. for me it creates:
/home/martinmorgenstern/workspace/scs/standards/Tests/home/martinmorgenstern/workspace/scs/standards/Tests/kaas/k8s-default-storage-class
The reason is that __file__
(which is the absolute path to the script) is used as the test_file_name
argument passed to gen_sonobuoy_result_file
and used without further processing.
In main()
, you should use os.path.basename(__file__)
to get only the script filename without the directories.
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.
yes you're right. ithought i already changed that
@@ -56,19 +56,17 @@ def setup_k8s_client(kubeconfigfile=None): | |||
|
|||
|
|||
def gen_sonobuoy_result_file(error_n: int, error_msg: str, test_file_name: str): | |||
|
|||
test_name = test_file_name.replace(".py", "") |
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.
test_name = test_file_name.replace(".py", "") | |
test_name = test_file_name.removesuffix(".py") |
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
Signed-off-by: Katharina Trentau <[email protected]>
fixed two bugs: