-
Notifications
You must be signed in to change notification settings - Fork 12
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
ovn-tester: Add CMS plugin support. #173
Conversation
97d8699
to
ddf5ad4
Compare
] | ||
return worker_nodes | ||
|
||
def prepare_test(self, central_node, worker_nodes, cluster_cfg, brex_cfg): |
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.
Would it make sense to move that function to ovn-tester/ovn_tester.py
? It looks rather generic to me
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.
It does indeed look like one of the generic ones, I think my thinking at the time was that Cluster
potentially is CMS specific. I'll consider moving it back. Thanks @felixhuettner !
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.
In #175 this does become CMS specific:
c964fb2#diff-b2472ea71d24c06480c4a2d181759380d75d19cd409936937d72927de0fe80c5R146
ClusterBringupCfg = namedtuple("ClusterBringupCfg", ["n_pods_per_node"]) | ||
|
||
|
||
class ovn_kubernetes(object): |
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.
Class name should use PascalCase, i.e. OVNKubernetes
, and the (object)
is also not necessary
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.
The reason for the all lower case is that the class name will be computed and the class loaded at runtime ref: https://github.com/ovn-org/ovn-heater/pull/173/files#diff-fd90db2a7b9d7962bf6c9f4bc48ab0664be99e8b03d476e049c6513452e96411R175
We can of course teach that code to convert the end user supplied string to PascalCase.
Thanks for the tip on object
, old habits hard to eradicate ;)
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 for the tip on
object
, old habits hard to eradicate ;)
OTOH, the other class definition in the project do appear to inherit from (object)
so we might want to keep it for consistency.
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.
The reason for the all lower case is that the class name will be computed and the class loaded at runtime ref
I had some thoughts about this too because I'm always wary of dynamic imports. Since there are limited amount of CMS implementation classes, we could create Dict[str, CMSClass]
mapping between strings from test definitions and implementation classes. I think it'd be bit more deterministic.
There's only one other object in the cms module, ClusterBringupCfg
named tuple, but that could be moved to the implementation class as well, as a class-level attribute.
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.
Maybe I'm misunderstanding this but we dynamically import the test classes, e.g., cms.ovn_kubernetes.tests.densty_light
. There can be quite a few different tests per CMS type. How would a static mapping work for those?
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 for the tip on
object
, old habits hard to eradicate ;)
It's actually my fault, I added all those (that shows how often I write python code :D), sorry. I'll post a cleanup PR soon.
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.
yeah, dynamic test loading would probably have to stay as there are many tests. My suggestion was only aimed at mapping between CMS definition in test scenario
cms:
name: openstack
# or
cms:
name: ovn-kubernetes
and their implementation class.
It's not crucial, but I think it'd be bit easier to read and more explicit. Currently proposed load_cms
function looks like this:
def load_cms(cms_args):
cms_name = cms_args["name"]
mod = importlib.import_module(f"cms.{cms_name}")
class_name = cms_name.replace("-", "_")
cls = getattr(mod, class_name)
return cls(cms_args)
And my suggestion would look something like this
CMS_MAPPING = {
"ovn-kubernetes": cms.ovn_kubernetes.OVNKubernetes,
"openstack": cms.openstack.Openstack,
}
def load_cms(cms_args):
cms_name = cms_args["name"]
cms_class = CMS_MAPPING.get(cms_name)
if cms_class is None:
raise RuntimeError()
return cms_class(cms_args)
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.
Sure, sounds OK to me.
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.
Ait, let's make it pretty. I'll most likely rely on an attribute in the imported module to tell us the class name to use rather than maintaining a map in the generic code though, but it should serve the same purpose.
n_pods_per_node=bringup_args.get("n_pods_per_node", 10) | ||
) | ||
|
||
def create_nodes(self, cluster_config, workers): |
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.
nitpick: This method (and prepare_test
as well) does not reference self
. It could be decorated as @staticmethod
and drop self
from arguments.
We expect Python3 to be used and in Python3 all classes inherit from object. Reported-at: ovn-org#173 (comment) Signed-off-by: Dumitru Ceara <[email protected]>
ovn-tester/ovn_tester.py
Outdated
@@ -19,34 +19,34 @@ | |||
|
|||
|
|||
GlobalCfg = namedtuple( | |||
'GlobalCfg', ['log_cmds', 'cleanup', 'run_ipv4', 'run_ipv6'] | |||
"GlobalCfg", ["log_cmds", "cleanup", "run_ipv4", "run_ipv6"] |
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 was kind of on purpose and to avoid large diffs, we enforced black formatting but without string quotes or prefixes normalization:
9c3e007
However, if I apply string quotes normalization to the whole repo I get:
$ git diff --stat
ovn-fake-multinode-utils/generate-hosts.py | 46 ++++++++++++-------------
ovn-fake-multinode-utils/get-config-value.py | 4 +--
ovn-fake-multinode-utils/process-monitor.py | 20 +++++------
ovn-fake-multinode-utils/translate_yaml.py | 20 +++++------
ovn-tester/ovn_context.py | 12 +++----
ovn-tester/ovn_ext_cmd.py | 22 ++++++------
ovn-tester/ovn_load_balancer.py | 24 ++++++-------
ovn-tester/ovn_sandbox.py | 30 ++++++++--------
ovn-tester/ovn_stats.py | 42 +++++++++++-----------
ovn-tester/ovn_tester.py | 110 +++++++++++++++++++++++++++++-----------------------------
ovn-tester/ovn_utils.py | 130 ++++++++++++++++++++++++++++++++++----------------------------------
ovn-tester/ovn_workload.py | 352 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------------------------
ovn-tester/tests/cluster_density.py | 16 ++++-----
ovn-tester/tests/density_heavy.py | 28 +++++++--------
ovn-tester/tests/density_light.py | 16 ++++-----
ovn-tester/tests/netpol.py | 14 ++++----
ovn-tester/tests/netpol_cross_ns.py | 16 ++++-----
ovn-tester/tests/netpol_large.py | 2 +-
ovn-tester/tests/netpol_multitenant.py | 34 +++++++++---------
ovn-tester/tests/netpol_small.py | 2 +-
ovn-tester/tests/service_route.py | 26 +++++++-------
utils/latency.py | 34 +++++++++---------
utils/process-stats.py | 42 +++++++++++-----------
23 files changed, 521 insertions(+), 521 deletions(-)
It doesn't seem that extreme, so maybe it's acceptable to just do this all in a single commit now that we're refactoring most of the tester.
In any case, we should at least enforce this for new changes so please also update the black
invocation in the GitHub action worflow definition and remove the -S
.
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.
Just adding my opinion on this, I don't think it's a good idea to do this change. It bloats git history a bit and doesn't provide any value IMO. From the PEP 8 perspective we are fine with both:
In Python, single-quoted strings and double-quoted strings are the same. This PEP does not make a
recommendation for this. Pick a rule and stick to it. When a string contains single or double quote characters,
however, use the other one to avoid backslashes in the string. It improves readability.
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.
OTOH, we're changing a lot of the code base so if we want to make such a change, now is the moment IMO.
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.
The formatting was applied to make the CI happy for file changed, and for some reason I picked up on the project using black -l 79
but missed the -S
when applying it.
I'm fine with redoing those commits with -S
applied and my personal style is to use single quotes for strings as well (''
).
To avoid this in the future, what do you think about adding a top level pyproject.toml
file with black configuration?
For example
[tool.black]
line-length = 79
skip-string-normalization = 1
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.
Perfect, +1 for a pyproject.toml
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.
Overall the change makes sense. I did leave some small comments about points that need some more discussion IMO before we continue.
Thanks!
To ensure contributors make use of the projects formatting preferences we create a top level `pyproject.toml` file with configuration for the black Python code formatter. Signed-off-by: Frode Nordahl <[email protected]>
The explicit reference to type in calls to super() are no longer necessary as of Python 3 [0]. 0: https://docs.python.org/3/library/functions.html#super Signed-off-by: Frode Nordahl <[email protected]>
5fbebb2
to
32c47ae
Compare
Move CMS specific test bringup code out of `ovn_tester` module. Introduce `cms_name` global config. Load cms specific module and class on the back of name from configuration. Convert the `base_cluster_bringup` step to a regular test step. Signed-off-by: Frode Nordahl <[email protected]>
The test cases in `ovn-tester/tests` are all CMS specific. Now that we have CMS plugins, relocate the test code along with them. Signed-off-by: Frode Nordahl <[email protected]>
32c47ae
to
bc24b19
Compare
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 iteration LGTM. Thanks Frode.
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 a lot @fnordahl @felixhuettner @mkalcok, looks good to me!
We expect Python3 to be used and in Python3 all classes inherit from object. Reported-at: ovn-org#173 (comment) Signed-off-by: Dumitru Ceara <[email protected]>
Please review/merge commit by commit.
The test cases in ovn-heater are currently specific to the
ovn-kubernetes
CMS.To support an ongoing effort to add test cases for the OpenStack CMS we need to make some architectural changes that includes moving CMS specific code into a plugin and allowing for automatic loading of new plugins.