Skip to content
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

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

Closed
wants to merge 3 commits into from

Conversation

glima
Copy link
Contributor

@glima glima commented Apr 14, 2021

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.

@glima glima requested a review from squirrelsc as a code owner April 14, 2021 22:18
@glima glima force-pushed the main branch 2 times, most recently from fda8a47 to 2a254c0 Compare April 14, 2021 22:27
@LiliDeng LiliDeng added the 🆕 LISAv3 Incubation work for the next version of LISA label Apr 15, 2021
docs/run.md Outdated
```

on a Linux shell.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
Can you point out what is exacly strange, please?

docs/run.md Outdated
```

on Windows' cmd.exe or
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll revisit that later, no biggie

@glima glima force-pushed the main branch 2 times, most recently from 318e26c to ee715fb Compare April 16, 2021 23:50
docs/run.md Outdated

```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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

docs/run.md Outdated
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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

@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 glima force-pushed the main branch 2 times, most recently from 188723a to f0bfdf8 Compare April 20, 2021 23:23
@glima
Copy link
Contributor Author

glima commented Apr 21, 2021

Friendly ping :)

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@glima glima Apr 23, 2021

Choose a reason for hiding this comment

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

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.

@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :/

nodes_final.append(new_node)
self.nodes = nodes_final

def _get_subclasses(self, t: Type[Node]) -> Iterable[Type[Node]]:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I will make a prototype of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems unrelated to my patch

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

glima commented Apr 28, 2021

Closing in favor of sibling PR that got in.

@glima glima closed this Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 LISAv3 Incubation work for the next version of LISA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants