-
Notifications
You must be signed in to change notification settings - Fork 28
Create conformance test for scs-0214-v1 #486
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
Conversation
mbuechse
left a comment
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.
A few things that I think should be addressed. If you are time-constrained, I offer to assist.
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
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
Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py
Outdated
Show resolved
Hide resolved
| import yaml | ||
|
|
||
|
|
||
| logging_config = { |
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 seems kinda redundant given that the same info is in the yaml 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 is there if a yaml file isn't provided.
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.
I know. Then it should just be provided (and the fallback can be way more minimal)
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.
Its already more minimal than the config, but maybe it can be minimized more(?) Not sure though.
I fixed some small artifacts I found in the config.
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.
Why should the user be able to configure the logging in the first place? I would actually remove the -c/--config option and the yaml file completely.
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.
Let's come back to this point later. It's not mission critical.
Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py
Outdated
Show resolved
Hide resolved
|
Added a fixup containing the mentioned changes. I hope I didn't forget anything @mbuechse Hint: I couldn't test this right now, I got some problems with my K8s clusters. |
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
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
left a comment
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.
Good work, thanks Hannes. But for me, some of the code was hard to understand/follow and I left some suggestions for improvement.
Additionally, with manual testing I could only verify that the following cases have the expected conformance test outcome:
- test with a minikube which is too small ("ERROR: Node is not labelled as a control plane node" and exit code
2) - test with a k8s cluster with only one region ("WARNING: There seems to be no distribution across multiple regions or labels aren't set correctly across nodes." and exit code
0, i.e., conforming)
It would be nice if you could provide some test examples. If you reorganize the code in a way that the labels-by-node list can come from another source and not only from actually calling get_k8s_cluster_labelled_nodes(…) this would be easier to review.
| import yaml | ||
|
|
||
|
|
||
| logging_config = { |
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.
Why should the user be able to configure the logging in the first place? I would actually remove the -c/--config option and the yaml file completely.
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
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
Tests/kaas/k8s-node-distribution/k8s-node-distribution-check.py
Outdated
Show resolved
Hide resolved
I forgot to mention that, to me, the testing logic seems to be ok. Still, it would be good to have an easy way to verify the different scenarios, without having access to different kinds of clusters. |
I agree with you on this, we can probably look into this later on. (Sadly not until tomorrow). I also fixed/updated/changed most of the things you mentioned in the other comments, so you can recheck tomorrow =) Edit: Also in response to #486 (comment), in a previous test, we wanted to provide a possibility to configure the logging however it is necessary. You could now use the same config from that test here. |
martinmo
left a comment
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.
LGTM & we agreed via chat to add tests for the tests later.
This provides a basic node distribution test only operating based on the labels of the kubernetes cluster provided. A test will come back negative, if no distribution according to the labels can be detected. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
The test section of the standard also needs to be updated, even if the standard is already stabilized. Signed-off-by: Hannes Baum <hannes.baum@cloudandheat.com>
4f85444 to
f6e34d9
Compare
As discussed in #481, we need to create a conformance test for SCS-0214-v1. This PR does that and also includes a separate commit containing the necessary changes for the
Conformance Testssection of the standard. If we don't want to include this, the commit will be split up into a separate PR.Resolves SovereignCloudStack/issues#477