Make LocalNode and RemoteNode extendable classes (yml-wise) #1280

glima · 2021-04-14T22:18:12Z

Make LocalNode and RemoteNode extendable classes (yml-wise).

We do so by making use of BaseClassWithRunbookMixin, on the actual
instance classes, and ExtendableSchemaMixin, on the respective schema
classes. This makes it possible to have custom, type-defined sections
on general (SUT) nodes coming from the runbook, together with Platform
and Notifier.

This will come in handy for LISA users who have node logic/fields that
go beyond what is originally defined to work for (e.g.) Azure.

Example of power/usage:

By declaring this, on code:

@dataclass_json()
@dataclass
class MyNodeSchema(schema.RemoteNode):
    type: str = field(
        default=MYNAME,
        metadata=schema.metadata(
            required=True,
            validate=validate.OneOf([MYNAME]),
        ),
    )

    my_example_extra_field: Optional[str] = field(default=None)

class MyNode(node.RemoteNode):
    def __init__(
        self,
        index: int,
        runbook: MyNodeSchema,
        logger_name: str,
        base_log_path: Optional[Path] = None,
        name: str = "",
    ) -> None:
        super().__init__(index, runbook,
                         logger_name=logger_name,
                         base_log_path=base_log_path,
                         name=name)
        self.my_example_extra_field = runbook.my_example_extra_field
        assert self.my_example_extra_field, \
            f"my_example_extra_field field of {MYNAME}-typed " \
            "nodes cannot be empty "

    @classmethod
    def type_name(cls) -> str:
        return MYNAME

    @classmethod
    def type_schema(cls) -> Type[schema.TypedSchema]:
        return MyNodeSchema

one is now able to do this, yml-wise:

environment:
  warn_as_error: true
  environments:
    - nodes:
      - type: MYNAME
        public_address: ...
        public_port: ...
        username: ...
        password: ...
->      my_example_extra_field: ...

Of course, custom logic for only that type of node will be at your
fingertips, just by extending/overriding your node class. Of course
this is advanced usage and not meant for the average user.

LiliDeng · 2021-04-15T03:23:37Z

docs/run.md

+    ```
+
+	on a Linux shell.
+


from https://github.com/glima/lisa/blob/main/docs/run.md, I feel the display a little bit strange.

Can you point out what is exacly strange, please?

squirrelsc · 2021-04-15T07:05:57Z

docs/run.md

    ```

+	on Windows' cmd.exe or


If the user follows the instruction, he will create an alias. So that, it doesn't need to run with lisa.sh. It doesn't need to increase document due to the minor difference.

It's a tiny increase, come on. And having someone adding an alias just for you is very invasive.

Can you open an issue to me for this document change? The run.cmd is very important to new users. It tries to provide an early success as simple as possible. The longer document spends time of every reader.
The people works on both Windows and Linux may try to compare the content to understand difference. When we modify the command line, we need to update two place.

I agree things should be simple, and encapsulating a complex poetry env call into those scripts is good enough, isn't it? It's just a matter of destiny that cmd.exe will lookup $PWD for path lookup (and use extensions to derive meaning), that's not a thing in Linux. Having two driver files is perfectly fine (lots of projects have that), whereas having the user install an alias, which only makes sense inside a certain dir, is very orthodox. My 2 cents. I can open an issue, what would the wording be about, specifically?

Add an issue #1287 . Can you drop this commit?

Alright, I'll revisit that later, no biggie

squirrelsc · 2021-04-19T02:10:54Z

docs/run.md


    ```bash
-    lisa -r ./microsoft/runbook/azure.yml -v subscription_id:<subscription id> -v "admin_private_key_file:<private key file>"
+    lisa -r ./microsoft/runbook/azure.yml -v subscription_id:<subscription id> -v "admin_private_key_file:<private key file>" -v "marketplace_image:Debian:debian-10:10:latest"


In this document, it makes sure the environment is correct with minimum arguments. I have a task to make private key as optional. After that, only subscription is required. That would be much easier to have a first try.
I understand in most scenarios, the image should be specified. So we can add a notes below the command to explain this parameter can specify a marketplace image.
Feel free to create a task to me. I will fix this document later.

2021-04-19 18:55:08.033 INFO LISA completed in 1.086 sec 2021-04-19 18:55:08.034 ERROR LISA cannot find variable 'marketplace_image', make sure its value filled in runbook, command line or environment variables. Traceback (most recent call last):

without the market_place arg. I'll open an issue for that, then

squirrelsc · 2021-04-19T02:44:07Z

docs/run.md

-    Use above `<subscription id>` and `<private key file>` to replace in below command. It may take several minutes to complete.
+    Use above `<subscription id>` and `<private key file>` to replace
+    in commands below. They may take several minutes to complete,
+    depending on the complexity of the passed runbooks. A Marketplace


the marketplace image is not a must have argument. It has the default value. If it's not. please file an issue to Lili.

The time doesn't depends on complexity of the passed runbook here. The below command spends 3 to 5 minutes or more, the time depends on Azure's status.

Alright, but we're not tying this patch to enter only after that, right? Whoever fixes that, fixes my docstrings as well, afterwards.

OK, this comment is for that specific runbook, fair (about the second line).

dsrivastavv · 2021-04-20T20:25:44Z

@squirrelsc @LiliDeng Can you please update the settings so that I am notified if there are any new PR. I think there is also a way to make a team (https://docs.github.com/en/organizations/organizing-members-into-teams/creating-a-team) which can be added for reviews.

glima · 2021-04-21T18:10:46Z

Friendly ping :)

squirrelsc · 2021-04-22T06:29:10Z

lisa/executable.py

@@ -20,6 +21,11 @@
 T = TypeVar("T")


+# circular dep if we use the helper in node.py, unfortunately
+def _is_node_remote(node: Node) -> bool:


How about define is_remote to a member of Node? It will not be cycle reference.

I'd rather not have a method on the parent about children

See my PR, it can be an abstract method, no type checking.

It's still ugly to infer anything about extensions on a base class, that breaks OO totally :/

is_remote is a feature on node like is_posfix. Some case cannot run locally, like smoke_test. It's high cost to reboot current machine, and resume tests.

I know, but it belongs in the module, not the class. OS is one class only, node is not anymore. The module knows there is more to a Node, the class should not have anything to do with it.

lisa/executable.py

squirrelsc · 2021-04-22T06:32:20Z

lisa/executable.py

@@ -490,20 +496,20 @@ def __getitem__(self, tool_type: Union[Type[T], CustomScriptBuilder, str]) -> T:
            if not tool.exists:
                tool_log.debug(f"'{tool.name}' not installed")
                if tool.can_install:
-                    tool_log.debug(f"{tool.name} is installing")
+                    tool_log.debug(f"{tool.name} is about to be installed")


I think installing is more useful. The logic between the log and installing, they are very closed. installing is a more closed state.

No hard feelings for this one and I'll drop. But a native speaker shout here might help settle :)

squirrelsc · 2021-04-22T06:36:49Z

lisa/util/shell.py

+                        )
+                    sleep(1)
+                    times += 1
+            except Exception as e:


Well, we can add that, but we cannot add address:port to each line of log. This line can be corelated by environment name like customized_0. It's a global unique environment name, search it can get all information.

This is a fatal failure, most of the time, so it won't be repetitive. How is this related to env. name, I fail to graps that. BTW, what's the reasoning behind that "customized" naming?

There may be better name than "customized", feel free to suggest name to differentiate environments between test case requirements and explicit defined in runbook.
For the log line, it's ok to print IP/port here. Some day, there may be other information is necessary, like user name, kernel version. We cannot print all of them on one line. So it needs to look for the log by customized_0. The key word corelated logs.

So customized_0 is for the env name? How about "unnamed_env_0", then? It's best to have some namespacing on the thing, so readers are not so lost at a first glance.

The key information is about where an environment is from. Both runbook defined and generated from test cases may be unnamed. So unnamed may not be enough. In most place, the name shows like env[customized_0], so the type is out of it. Other types like platform, node, notifier... They won't be a part of name.

squirrelsc · 2021-04-22T06:44:04Z

lisa/parameter_parser/runbook.py

-    # merge others
-    result.update(data_from_current)
+    # Merge parent and current. We want a *deep/recursive* merge.
+    _update_dict_nested(result, data_from_current)


It's by design here. To merge runbooks are very complex. Currently, it takes simplest way. Besides variable and parent paths, all other fields takes only highest level things. It's enough for current purpose. If you check current merge logic of variable and parent paths, there are a lot of tricky logic. For other parts, it's similar. Blinding deep merge will bring unexpected behavior. That's not easy to troubleshooting.
I thought to define a merge strategy like this, it's not implemented yet. But it will be hard to use.

Well yeah, append strategy could be a thing. But what you have now is doubly bad, because it's destructive and one-level only. Mine is only destructive for things that will clash (maybe only very far deep), but at least you get what you expect of merges, if used right. Current thing is not a merge, it's a very shallow merge. How cool is that that you can simply merge with a parent with deep state already taken care of? It's not too hard to ask for users to be wary of collisions when using this, as they should already have been notified (they can happen in the shallow case and it's awful too).

If there is special logic on some part like variable, parent, I'm happy to have it. Because the merging is complex, currently the runbook should be composited by root level children, not support other scenarios. My principle is to provide a simple/limited feature, but not easy to make mistaken. Let me know, if it blocks something. We can figure out a better solution.

But it is easy to make a mistake as it is. Write a root level entry on parent, same entry on current, you're toast: One of them gets ignored. Mine will only ignore up to equal leaves, if there are any.

For merging runbooks, either approach cannot avoid all mistakes as far as I know. The current way takes latest, top one effective, and it should be completed. It's not recommended to define same child in different level. As what we used in microsoft/runbook, each level of runbook defines different nodes. the tn.yml define which cases to run, the ready, local and azure define the platform parameters.

Alright, I'll can hold this to the future--I only have warn_as_error for now which I'd merge anyway. But this will re-surface again soon :/

lisa/schema.py

squirrelsc · 2021-04-22T17:30:19Z

lisa/schema.py

+                    nodes_final.append(new_node)
+        self.nodes = nodes_final
+
+    def _get_subclasses(self, t: Type[Node]) -> Iterable[Type[Node]]:


The initialization of environment and node can be move from the schema to their implementations. That helps the logic is more clear, and the node can be used standalone. Refer to Notifier.

I will make a prototype of this.

This seems unrelated to my patch

It produces duplicate code like this methods. Fixed in my PR.

We do so by making use of BaseClassWithRunbookMixin, on the actual instance classes, and ExtendableSchemaMixin, on the respective schema classes. This makes it possible to have custom, type-defined sections on general (SUT) nodes coming from the runbook, together with Platform and Notifier. This will come in handy for LISA users who have node logic/fields that go beyond what is originally defined to work for (e.g.) Azure. Example of power/usage: By declaring this, on code: @dataclass_json() @DataClass class MyNodeSchema(schema.RemoteNode): type: str = field( default=MYNAME, metadata=schema.metadata( required=True, validate=validate.OneOf([MYNAME]), ), ) my_example_extra_field: Optional[str] = field(default=None) class MyNode(node.RemoteNode): def __init__( self, index: int, runbook: MyNodeSchema, logger_name: str, base_log_path: Optional[Path] = None, name: str = "", ) -> None: super().__init__(index, runbook, logger_name=logger_name, base_log_path=base_log_path, name=name) self.my_example_extra_field = runbook.my_example_extra_field assert self.my_example_extra_field, \ f"my_example_extra_field field of {MYNAME}-typed " \ "nodes cannot be empty " @classmethod def type_name(cls) -> str: return MYNAME @classmethod def type_schema(cls) -> Type[schema.TypedSchema]: return MyNodeSchema one is able to do this, yml-wise: environment: warn_as_error: true environments: - nodes: - type: MYNAME public_address: ... public_port: ... username: ... password: ... -> my_example_extra_field: ... Of course, custom logic for only that type of node will be at your fingertips, just by extending/overriding your node class. Of course this is advanced usage and not meant for the average user. UTs were added to help enforce regression testing. ammend to nodes

… host The user deserves to know which host was unreachable, at least, not just the port: ```2021-04-19 23:54:54.453 INFO LISA.lisa 'customized_0' attached to test case 'ATest.a_test': [Errno -2] Name or service not known``` was not telling anything

glima · 2021-04-28T17:25:26Z

Closing in favor of sibling PR that got in.

glima requested a review from squirrelsc as a code owner April 14, 2021 22:18

glima force-pushed the main branch 2 times, most recently from fda8a47 to 2a254c0 Compare April 14, 2021 22:27

LiliDeng assigned glima Apr 15, 2021

LiliDeng added the 🆕 LISAv3 Incubation work for the next version of LISA label Apr 15, 2021

LiliDeng reviewed Apr 15, 2021

View reviewed changes

squirrelsc reviewed Apr 15, 2021

View reviewed changes

glima force-pushed the main branch 2 times, most recently from 318e26c to ee715fb Compare April 16, 2021 23:50

squirrelsc reviewed Apr 19, 2021

View reviewed changes

glima force-pushed the main branch 5 times, most recently from dd14ba4 to 7f9f32f Compare April 20, 2021 00:09

glima force-pushed the main branch 2 times, most recently from 188723a to f0bfdf8 Compare April 20, 2021 23:23

squirrelsc reviewed Apr 22, 2021

View reviewed changes

lisa/executable.py Show resolved Hide resolved

squirrelsc reviewed Apr 22, 2021

View reviewed changes

lisa/schema.py Show resolved Hide resolved

squirrelsc reviewed Apr 22, 2021

View reviewed changes

glima force-pushed the main branch from f0bfdf8 to 27927ba Compare April 22, 2021 20:35

glima added 2 commits April 23, 2021 11:04

poetry: update

b0bc822

glima force-pushed the main branch from 063c616 to d61780e Compare April 23, 2021 18:16

glima closed this Apr 28, 2021

Make LocalNode and RemoteNode extendable classes (yml-wise) #1280

Make LocalNode and RemoteNode extendable classes (yml-wise) #1280

Conversation

glima commented Apr 14, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsrivastavv commented Apr 20, 2021

glima commented Apr 21, 2021

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

glima Apr 23, 2021 • edited

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

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

glima commented Apr 28, 2021

glima commented Apr 14, 2021 •

edited

glima Apr 23, 2021 •

edited