Issue/662 default storage class #745

fraugabel · 2024-09-09T11:27:11Z

fixed two bugs:

make sure sonobuoy result file can be written
cleanup test-pod at failure aswell, so the next run won't crash

martinmo

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.

Tests/requirements.in

martinmo · 2024-09-13T08:58:55Z

Tests/requirements.in

 kubernetes_asyncio
 python-dateutil
 PyYAML
 openstacksdk
 requests
 tomli
+


Suggested change

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

martinmo · 2024-09-13T10:00:28Z

Tests/kaas/k8s-default-storage-class/helper.py

+        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)


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.

yes you're right. ithought i already changed that

Tests/kaas/k8s-default-storage-class/helper.py

mbuechse

I'm not sure I can follow, but it does seem to be work in progress anyway.

mbuechse · 2024-10-25T11:22:10Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

-    namespace = "default"
-    pvc_name = "test-pvc"
-    pv_name = "test-pv"
-    pod_name = "test-pod"


please turn these into arguments with default values, i.e.,

namespace=NAMESPACE, pvc_name=PVC_NAME, ...

this makes the function more versatile, and we don't litter the code with these allcaps identifiers

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

mbuechse · 2024-10-25T11:30:14Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

+        if isinstance(exc_value, SCSTestException):
+            self.return_message = exc_value.args[0]
+            self.return_code = exc_value.return_code
+            print(f"SCSTestException occurred with return_code: {self.return_code}")
+        else:
+            # No specific exception, handle normally
+            print(f"Exiting the context with return_code: {self.return_code}")


This seems really convoluted. The exceptions are being caught in the main function anyway. And why should the test environment carry the burden of storing the return code? That violates the single-responsibility principle (aka separation of concerns).

because in this case I assumed that these exceptions are only intercepted afterwards. i'll check this again.

the return code is kept there to feed it into the sonobuoy result, which is created in the exit, to make sure it gets created, after any error whatsoever. This is actually the reason why i created this context management to avoid having the same exception (SCSTestException) raised in 5 different places.

mbuechse · 2024-10-30T10:06:23Z

@berendt Hi Christian, can you provide us with a k8s cluster for testing this?

mbuechse · 2024-11-06T12:32:36Z

Tests/iaas/security-groups/default-security-group-rules.py

looks like a mistake, this file does not belong to this PR

mbuechse · 2024-11-06T12:32:59Z

Tests/iaas/key-manager/check-for-key-manager.py

this file seems to have been added in error as well

strange.. at checkout they should be reseted

i'll fix this asap

Signed-off-by: Katharina Trentau <[email protected]>

…_readwriteonce() Signed-off-by: Katharina Trentau <[email protected]>

Signed-off-by: Katharina Trentau <[email protected]>

Signed-off-by: Matthias Büchse <[email protected]>

mbuechse

Overall this is going into the right direction. I'm afraid a few problems are still left to tackle. I also altered a few things myself (see the commits).

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

mbuechse · 2024-11-13T14:29:43Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py


+def check_csi_provider(k8s_core_api, allowed_csi_prov=ALLOWED_CSI_PROV):


What is this good for, and how was the list ALLOWED_CSI_PROV determined? I can't see any mention of this in the standard nor the testing notes. A comment in the code is required, but also some explanation in the testing notes.

it was a suggestion by @martinmo to provide a list of allowed csi-providers, that work accordingly to the standard

Okay, but this doesn't address any of my points.

mbuechse · 2024-11-13T14:30:39Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

+                    return_code=33,
+                )
+    if not csi_list:
+      logger.info("CSI-Provider: No CSI Provider found.")


Why is this not an error? (I really don't know. Maybe a comment would help)
Also please note that the indentation is wrong here.

its not an error, in case a kind cluster is used (where no csi privider is used)

it should simply filter out "wrong" csi providers

I'm not sure if Kind is supposed to pass the test. Why have a special case for Kind? But this whole thing should AT MOST produce a warning anyway for the standard makes not mention of it whatsoever.

mbuechse · 2024-11-13T14:34:03Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

+            logger.error(f"Error preparing Environment: {e}")
+            return False
+
+    def clean(self):


I think we can get rid of (big parts of) prepare as well as the variable cleanup by making this method ignore any error that says "resource not found". To me, this would be very much preferable. (It would in principle also reduce the risk of race conditions.)

this is a good idea

well, i can catch a "404 resource not found" now and then skip the clean() execution, but if only one of both (pvc,pod) is missing the other one does not get deleted. so i am thinking, that i should ratehr catch that in the clean() function itself. what do you think?

My proposal was to call delete_namespace_pod and delete_namespaced_persistent_volume_claim unconditionally, and to swallow any exception that indicates that deletion was unsuccessful because the resource didn't exist in the first place.

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

mbuechse · 2024-11-13T14:34:55Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

+            env.return_code = 1
+            logger.debug("check_default_storageclass() failed")
+            return env.return_code


Suggested change

env.return_code = 1

logger.debug("check_default_storageclass() failed")

return env.return_code

logger.debug("check_default_storageclass() failed")

return 1

mbuechse · 2024-11-13T14:35:41Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

+                env.return_code = 3
+                env.return_message = "(409) conflicting resources"
+                return


I think the return is in error here.

mbuechse · 2024-11-13T14:38:33Z

Tests/kaas/k8s-default-storage-class/k8s-default-storage-class-check.py

+
+    def __exit__(self, exc_type, exc_value, traceback):
+        if self.cleanup:
+          self.clean()


(as reported by flake8: indention is wrong here as well)

Signed-off-by: Katharina Trentau <[email protected]>

mbuechse · 2024-11-14T19:12:52Z

Tests/iaas/security-groups/default-security-group-rules.py

This file seems to be here in error.

fraugabel requested review from mbuechse, martinmo and tonifinger September 9, 2024 11:27

fraugabel force-pushed the issue/662_default_storage_class branch from be2c002 to 7e9061a Compare September 9, 2024 11:33

martinmo reviewed Sep 13, 2024

View reviewed changes

mbuechse mentioned this pull request Oct 25, 2024

[BUG] Conformance check for scs-0211-v1 (and v2) is broken #662

Open

mbuechse linked an issue Oct 25, 2024 that may be closed by this pull request

[BUG] Conformance check for scs-0211-v1 (and v2) is broken #662

Open

mbuechse reviewed Oct 25, 2024

View reviewed changes

mbuechse reviewed Nov 6, 2024

View reviewed changes

fraugabel added 19 commits November 11, 2024 14:02

compiled new requirements.txt

1c4ca85

Signed-off-by: Katharina Trentau <[email protected]>

added makdirs check in helper.py

8880a15

Signed-off-by: Katharina Trentau <[email protected]>

deleted gen_sonsobuoy

a4e3603

Signed-off-by: Katharina Trentau <[email protected]>

re-compiled requirements

dcec54e

Signed-off-by: Katharina Trentau <[email protected]>

re-added gen_sonobuoy_result_file

fad1ed6

Signed-off-by: Katharina Trentau <[email protected]>

re-added gen_sonobuoy_result_file

b92fade

Signed-off-by: Katharina Trentau <[email protected]>

added cleanup() as subfunction to check_default_persistentvolumeclaim…

545d0ed

…_readwriteonce() Signed-off-by: Katharina Trentau <[email protected]>

added cleanup() as subfunction to check_default_persistentvolumeclaim…

9956bd1

…_readwriteonce() Signed-off-by: Katharina Trentau <[email protected]>

rebased

43e1282

Signed-off-by: Katharina Trentau <[email protected]>

resolved sonobuoy filename issue

218883f

Signed-off-by: Katharina Trentau <[email protected]>

Default Retries adjustable on module level

1743399

Signed-off-by: Katharina Trentau <[email protected]>

cleanup on module level and cleanup on ressource conflict

f717aa8

Signed-off-by: Katharina Trentau <[email protected]>

recompiled requirements.in after deleting fabric

2fd1fc7

Signed-off-by: Katharina Trentau <[email protected]>

added debug option

95c74c6

Signed-off-by: Katharina Trentau <[email protected]>

added debug option

12d3c15

Signed-off-by: Katharina Trentau <[email protected]>

creating context manager

d00f1ba

Signed-off-by: Katharina Trentau <[email protected]>

context manager cleaning up

8afc712

Signed-off-by: Katharina Trentau <[email protected]>

cleaning up with error handling

13c7622

Signed-off-by: Katharina Trentau <[email protected]>

break apart monolyte

0211b18

Signed-off-by: Katharina Trentau <[email protected]>

fraugabel added 14 commits November 11, 2024 14:02

SCSTestExceptions must be raised inside with block

0ff381a

Signed-off-by: Katharina Trentau <[email protected]>

SCSTestException correctly caught into yaml

b44b0ad

Signed-off-by: Katharina Trentau <[email protected]>

fixed modul sorting and doc string def prepare

6e079ed

Signed-off-by: Katharina Trentau <[email protected]>

checked convoluted exit

0ba6f9a

Signed-off-by: Katharina Trentau <[email protected]>

checked convoluted exit

3706237

Signed-off-by: Katharina Trentau <[email protected]>

checked convoluted exit

0465356

Signed-off-by: Katharina Trentau <[email protected]>

for flake

ad39c6d

Signed-off-by: Katharina Trentau <[email protected]>

csi provider check

31f9e68

Signed-off-by: Katharina Trentau <[email protected]>

csi provider check

0f16b17

Signed-off-by: Katharina Trentau <[email protected]>

ReadWriteOnce and PVC Bound tested

4f1a0f1

Signed-off-by: Katharina Trentau <[email protected]>

ReadWriteOnce and PVC Bound tested

7696250

Signed-off-by: Katharina Trentau <[email protected]>

default values

ea6c576

Signed-off-by: Katharina Trentau <[email protected]>

refracturing Exception chain

92ea10e

Signed-off-by: Katharina Trentau <[email protected]>

removed files that are not supposed to be in the PR

f9436e5

Signed-off-by: Katharina Trentau <[email protected]>

fraugabel force-pushed the issue/662_default_storage_class branch from f3fcb43 to f9436e5 Compare November 11, 2024 13:13

tonifinger mentioned this pull request Nov 11, 2024

Fix result file #818

Closed

fraugabel added 2 commits November 11, 2024 16:32

fixed key error in getting default storage class

78872c5

Signed-off-by: Katharina Trentau <[email protected]>

successfully tested csi-provider function

5ff4bda

Signed-off-by: Katharina Trentau <[email protected]>

fraugabel requested review from mbuechse and martinmo November 11, 2024 15:52

mbuechse added 3 commits November 13, 2024 15:03

Make executable, fix error message

20ffdab

Signed-off-by: Matthias Büchse <[email protected]>

simplify check_default_storageclass

fe7e58a

Signed-off-by: Matthias Büchse <[email protected]>

typos in create_pvc_pod

50643b2

Signed-off-by: Matthias Büchse <[email protected]>

mbuechse reviewed Nov 13, 2024

View reviewed changes

fraugabel added 4 commits November 13, 2024 16:25

commenting

389e87a

Signed-off-by: Katharina Trentau <[email protected]>

raising 404

c9a8b23

Signed-off-by: Katharina Trentau <[email protected]>

raising 404

2d3e749

Signed-off-by: Katharina Trentau <[email protected]>

raising 404

0dea150

Signed-off-by: Katharina Trentau <[email protected]>

mbuechse reviewed Nov 14, 2024

View reviewed changes

Tests/iaas/security-groups/default-security-group-rules.py

Copy link

Contributor

mbuechse Nov 14, 2024

Choose a reason for hiding this comment

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

This file seems to be here in error.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/662 default storage class #745

Issue/662 default storage class #745

fraugabel commented Sep 9, 2024

martinmo left a comment

martinmo Sep 13, 2024

martinmo Sep 13, 2024

fraugabel Oct 2, 2024

mbuechse left a comment

mbuechse Oct 25, 2024

mbuechse Oct 25, 2024

fraugabel Oct 27, 2024

fraugabel Oct 28, 2024

mbuechse commented Oct 30, 2024

mbuechse Nov 6, 2024

fraugabel Nov 6, 2024

mbuechse Nov 6, 2024

fraugabel Nov 6, 2024

fraugabel Nov 6, 2024

mbuechse left a comment

mbuechse Nov 13, 2024

fraugabel Nov 13, 2024

mbuechse Nov 13, 2024

mbuechse Nov 13, 2024

fraugabel Nov 13, 2024 •

edited

Loading

fraugabel Nov 13, 2024

mbuechse Nov 13, 2024

mbuechse Nov 13, 2024

fraugabel Nov 13, 2024

fraugabel Nov 14, 2024

mbuechse Nov 14, 2024

mbuechse Nov 13, 2024

mbuechse Nov 13, 2024

mbuechse Nov 13, 2024

mbuechse Nov 14, 2024


		def check_csi_provider(k8s_core_api, allowed_csi_prov=ALLOWED_CSI_PROV):

Issue/662 default storage class #745

Are you sure you want to change the base?

Issue/662 default storage class #745

Conversation

fraugabel commented Sep 9, 2024

martinmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbuechse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbuechse commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbuechse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraugabel Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fraugabel Nov 13, 2024 •

edited

Loading