Updates to Node distribution test (#489) #558

cah-hbaum · 2024-04-10T08:48:26Z

added a new input that enables testing the test by providing yaml files containing label information for different nodes
added "test-example.yaml" containing an example for such a test file
removed the internal config, since the normal config file is already provided and read in as a default
added a pytest that enables easy testing of the check_nodes function (thanks to @martinmo)

cah-hbaum · 2024-04-10T08:49:25Z

Also, like its written in the issue itself, we could think about providing some more scenarios for this test, especially with the newly added testing possibilities via files.

martinmo

Good idea with the test data. I have some suggestions about this to make it easier to unit test, see the inline comments.

Apart from that: I think it makes more sense to remove external logging configuration and the --config flag. It is not used anywhere and the short form -c also conflicts with the short form of --os-cloud in the IaaS scripts.

The logging config can be simplified like it is done here:

standards/Tests/iaas/flavor-naming/cli.py

Lines 128 to 130 in 5e9dfd5

    
           if __name__ == '__main__': 
        
               logging.basicConfig(format='%(levelname)s: %(message)s', level=logging.INFO) 
        
               cli(obj=Config())

Also, when I run without any arguments, the script fails.

$ ./kaas/k8s-node-distribution/k8s-node-distribution-check.py
The config file under ./config.yaml couldn't be found.
Traceback (most recent call last):
  File "/home/martinmorgenstern/workspace/scs-standards/Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py", line 127, in initialize_config
    with open(config.config_path, "r") as f:
FileNotFoundError: [Errno 2] No such file or directory: './config.yaml'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/martinmorgenstern/workspace/scs-standards/Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py", line 131, in initialize_config
    exit(1)
  File "/usr/lib/python3.10/_sitebuiltins.py", line 26, in __call__
    raise SystemExit(code)
SystemExit: 1

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/martinmorgenstern/workspace/scs-standards/Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py", line 244, in <module>
    return_code = asyncio.run(main(sys.argv[1:]))
  File "/usr/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/home/martinmorgenstern/workspace/scs-standards/Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py", line 197, in main
    config = initialize_config(parse_arguments(argv))
  File "/home/martinmorgenstern/workspace/scs-standards/Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py", line 135, in initialize_config
    setup_logging(config.logging)
  File "/home/martinmorgenstern/workspace/scs-standards/Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py", line 112, in setup_logging
    logging.config.dictConfig(config_log)
  File "/usr/lib/python3.10/logging/config.py", line 811, in dictConfig
    dictConfigClass(config).configure()
  File "/usr/lib/python3.10/logging/config.py", line 375, in __init__
    self.config = ConvertingDict(config)
TypeError: 'NoneType' object is not iterable

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py

martinmo

The standard states that

At least one control plane instance MUST be run in each "failure zone"

I played around with the test-example.yaml you provided in ipython and created a scenario where all control plane nodes are in the same zone and region. In this case, the script would still exit with 0 and just print

WARNING: There seems to be no distribution across multiple regions or labels aren't set correctly across nodes.
WARNING: There seems to be no distribution across multiple zones or labels aren't set correctly across nodes.
INFO: The master nodes are distributed across 3 host-ids.
INFO: The worker nodes are distributed across 2 regions.

Shouldn't it produce an ERROR? Or do we assume the physical host is the failure zone?

Furthermore, I would like to see more scenarios like the one in the yaml file, and have them tested automatically in the CI against the expected check outcome, with pytest.

The config yaml template file can be removed.

cah-hbaum · 2024-04-17T13:40:17Z

The standard states that

At least one control plane instance MUST be run in each "failure zone"

I played around with the test-example.yaml you provided in ipython and created a scenario where all control plane nodes are in the same zone and region. In this case, the script would still exit with 0 and just print
WARNING: There seems to be no distribution across multiple regions or labels aren't set correctly across nodes.
WARNING: There seems to be no distribution across multiple zones or labels aren't set correctly across nodes.
INFO: The master nodes are distributed across 3 host-ids.
INFO: The worker nodes are distributed across 2 regions.
Shouldn't it produce an ERROR? Or do we assume the physical host is the failure zone?

The thing that was a bit problematic while writing the standard was that different CSPs probably have different ideas about their failure zone, especially having small CSPs in mind. So it could very well be possible that the physical hosts are dividing the cluster into failure zones.
Thats why the test doesn't throw an error here.

Personally I think thats perfectly reasonable, but if you have another opinion on that, we can discuss this further. (But that would probably also mean to change the standard again)

Furthermore, I would like to see more scenarios like the one in the yaml file, and have them tested automatically in the CI against the expected check outcome, with pytest.

I'm gonna add the other files I added with some additional examples.

The config yaml template file can be removed.
You're right.

martinmo · 2024-04-17T15:08:31Z

The thing that was a bit problematic while writing the standard was that different CSPs probably have different ideas about their failure zone, especially having small CSPs in mind. So it could very well be possible that the physical hosts are dividing the cluster into failure zones.
Thats why the test doesn't throw an error here.

Personally I think thats perfectly reasonable, but if you have another opinion on that, we can discuss this further. (But that would probably also mean to change the standard again)

Thanks for the clarification, I agree 👍

cah-hbaum · 2024-04-18T10:32:30Z

Added some more example files.

martinmo

Second review round:

As we discussed today in person, I've committed a pytest script that uses the scenarios in your YAML files and I added a few more (in my opinion) interesting scenarios.

One test currently fails (intentionally) because the conformance test issues a warning twice if the host-id label is missing where I think it should only do that once (that's one of the new scenarios I added). What do you think?

Furthermore, I propose to replace all occurences of master with control-plane (or just control), because this fits better with the current terminology in Kubernetes (master is outdated).

Small typo, which is not part of the diff: in line 149, it should read "the label … doesn't …" instead of "… don't …".

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py

cah-hbaum · 2024-04-25T10:59:41Z

One test currently fails (intentionally) because the conformance test issues a warning twice if the host-id label is missing
where I think it should only do that once (that's one of the new scenarios I added). What do you think?

Agree with that, that duplication shouldn't happen. I removed that in the coming commit.

I also did a general update to test test script in order to enforce the standard requirement for labels to be provided (MUST). And I made a small change to your test to reflect this.

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py

- added a new input that enables testing the test by providing yaml files containing label information for different nodes - added "test-example.yaml" containing an example for such a test file - removed the internal config, since the normal config file is already provided and read in as a default Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

@martinmo

Some dates fixing some problems mentioned by @martinmo. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

Adds additional test files and removes previously deprecated config (template) files. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>

@martinmo

Some fixes and updates in order to be compliant with the testdata. Thanks to @martinmo. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>

mbuechse

Just one minor PEP-8 thing.

mbuechse · 2024-05-03T08:58:26Z

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py

+# infrastructure parts to smaller ones. So we first look if nodes are distributed
+# across regions, then zones and then hosts. If one of these requirements is fulfilled,
+# we don't need to check anymore, since a distribution was already detected.
+labels = (


Suggested change

labels = (

LABELS = (

this is the way PEP-8 recommends it, I think
(of course, the rest of the script would have to be adjusted accordingly)

Changed it to LABELS

mbuechse · 2024-05-03T08:59:02Z

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py

+class LabelException(BaseException):
+    """Exception raised if a label isn't set"""
+
+
 class Config:


This class now seems a bit superfluous.

(But it can stay in case the options grow again)

I'm gonna keep that for now.

mbuechse

Ah sorry, I would like to request two changes:

rename the script under test so that it can be imported directly
put the test data into a single yaml file, load that once, and hand out the sections using a fixture

Small pep-8 changes. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

@mbuechse

@mbuechse had some change requests, that are tackled with this commit. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

cah-hbaum · 2024-05-06T06:30:38Z

Ah sorry, I would like to request two changes:

rename the script under test so that it can be imported directly

put the test data into a single yaml file, load that once, and hand out the sections using a fixture

c60f895 should have the requested changes.

mbuechse · 2024-05-23T08:58:29Z

@cah-hbaum Thanks! Sorry for the long delay on my end. I think you need to update https://github.com/SovereignCloudStack/standards/blob/main/Tests/scs-compatible-kaas.yaml as well, no? Apart from that, I would prefer if the yaml file were loaded only once, which should be possible with pytest, if you use fixtures, or if you just do it on the module level. But it's not a deal breaker.

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

cah-hbaum · 2024-05-23T11:44:07Z

@cah-hbaum Thanks! Sorry for the long delay on my end. I think you need to update https://github.com/SovereignCloudStack/standards/blob/main/Tests/scs-compatible-kaas.yaml as well, no? Apart from that, I would prefer if the yaml file were loaded only once, which should be possible with pytest, if you use fixtures, or if you just do it on the module level. But it's not a deal breaker.

I updated the scs-compatible-kaas.yaml and introduced fixtures (I hope thats how you meant it).

mbuechse

LGTM ;)

cah-hbaum self-assigned this Apr 10, 2024

cah-hbaum requested a review from martinmo April 10, 2024 08:49

martinmo requested changes Apr 12, 2024

View reviewed changes

cah-hbaum linked an issue Apr 17, 2024 that may be closed by this pull request

Small updates to conformance test for scs-0214-v1 #489

Open

3 tasks

martinmo requested changes Apr 17, 2024

View reviewed changes

cah-hbaum requested a review from martinmo April 24, 2024 10:57

martinmo requested changes Apr 24, 2024

View reviewed changes

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py Outdated Show resolved Hide resolved

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py Outdated Show resolved Hide resolved

martinmo reviewed Apr 25, 2024

View reviewed changes

Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py Outdated Show resolved Hide resolved

martinmo approved these changes Apr 25, 2024

View reviewed changes

cah-hbaum requested a review from mbuechse April 30, 2024 10:14

cah-hbaum and others added 8 commits April 30, 2024 12:14

Change for requested updates

b224f4c

Some dates fixing some problems mentioned by @martinmo. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

Add additional test files

f1fe9c9

Adds additional test files and removes previously deprecated config (template) files. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

Add pytest script and more scenarios

dab39e2

Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>

Replace masterN with control-N in test data

565b80f

Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>

Streamline testdata

b427da4

Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>

Small fixes to the compliant to standard

04693ed

Some fixes and updates in order to be compliant with the testdata. Thanks to @martinmo. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

Use yaml.safe_load(…) instead of yaml.load(…, yaml.SafeLoader)

976a078

Signed-off-by: Martin Morgenstern <martin.morgenstern@cloudandheat.com>

cah-hbaum force-pushed the 489-small-update-to-scs-0214-test branch from a2a432a to 976a078 Compare April 30, 2024 10:14

mbuechse approved these changes May 3, 2024

View reviewed changes

mbuechse requested changes May 3, 2024

View reviewed changes

cah-hbaum added 2 commits May 3, 2024 16:39

PEP-8 changes (#475)

4d7cc7d

Small pep-8 changes. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

Update test according to change requests (#489)

c60f895

@mbuechse had some change requests, that are tackled with this commit. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

cah-hbaum requested a review from mbuechse May 22, 2024 07:12

fixup! Update test according to change requests (#489)

7749715

Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>

mbuechse approved these changes May 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to Node distribution test (#489) #558

Updates to Node distribution test (#489) #558

cah-hbaum commented Apr 10, 2024 •

edited

cah-hbaum commented Apr 10, 2024

martinmo left a comment

martinmo left a comment

cah-hbaum commented Apr 17, 2024

martinmo commented Apr 17, 2024

cah-hbaum commented Apr 18, 2024

martinmo left a comment

cah-hbaum commented Apr 25, 2024 •

edited

mbuechse left a comment

mbuechse May 3, 2024

cah-hbaum May 3, 2024

mbuechse May 3, 2024

mbuechse May 3, 2024

cah-hbaum May 3, 2024

mbuechse left a comment

cah-hbaum commented May 6, 2024

mbuechse commented May 23, 2024

cah-hbaum commented May 23, 2024

mbuechse left a comment

	if __name__ == '__main__':
	logging.basicConfig(format='%(levelname)s: %(message)s', level=logging.INFO)
	cli(obj=Config())

Updates to Node distribution test (#489) #558

Are you sure you want to change the base?

Updates to Node distribution test (#489) #558

Conversation

cah-hbaum commented Apr 10, 2024 • edited

cah-hbaum commented Apr 10, 2024

martinmo left a comment

Choose a reason for hiding this comment

martinmo left a comment

Choose a reason for hiding this comment

cah-hbaum commented Apr 17, 2024

martinmo commented Apr 17, 2024

cah-hbaum commented Apr 18, 2024

martinmo left a comment

Choose a reason for hiding this comment

cah-hbaum commented Apr 25, 2024 • edited

mbuechse left a comment

Choose a reason for hiding this comment

mbuechse May 3, 2024

Choose a reason for hiding this comment

cah-hbaum May 3, 2024

Choose a reason for hiding this comment

mbuechse May 3, 2024

Choose a reason for hiding this comment

mbuechse May 3, 2024

Choose a reason for hiding this comment

cah-hbaum May 3, 2024

Choose a reason for hiding this comment

mbuechse left a comment

Choose a reason for hiding this comment

cah-hbaum commented May 6, 2024

mbuechse commented May 23, 2024

cah-hbaum commented May 23, 2024

mbuechse left a comment

Choose a reason for hiding this comment

cah-hbaum commented Apr 10, 2024 •

edited

cah-hbaum commented Apr 25, 2024 •

edited