Refactor: Remote Juju and complex caching

johnlettman commented 2 months ago

Deep object filtering, regex, strong types, remote and local Juju all implemented -- reworking the outputs to support output formatting.

johnlettman commented 2 months ago

Object sub-selection: e.g., get the hostname of units matching the expression name=controller/0:

Getting pretty wild. The parsing of certain fields was inspired by @peterctl here.

LCVcode commented 2 months ago

Wow big WIP commit here. Pretty excited to see what you are working on.

I am not sure I fully grok the reasoning behind strong typing stuff. It looks like implementing that may have changed the format of jockey queries. Is that correct? I am sure there is some win with strong typing that I am not immediately seeing.

LCVcode commented 2 months ago

Will #36 be closed by this PR?

johnlettman commented 2 months ago

One of the ways of dealing with no authentication to the SSH server:

johnlettman commented 2 months ago

Wow big WIP commit here. Pretty excited to see what you are working on.

It's pretty exciting stuff. I'll be sure to push current shortly so you can play around with it. It's really rough, so forewarning.

I am not sure I fully grok the reasoning behind strong typing stuff.

The solid typing for the Juju status JSON is helpful from a development perspective for traversing the types. It's not super important, but it can be used to validate whether we received what we'd expect for status JSON.

It looks like implementing that may have changed the format of jockey queries. Is that correct?

The strong typing did not change the queries; the dot-notation traversal of the objects did. By change here, I only mean "extend." Ultimately, the command-line interface could be used in the same way as before with the same outputs.

Queries look identical to the prior functionality: juju-jockey units name~controller (units where name contains "controller")

controller/0

But, we can go even further beyond: juju-jockey units host.machine-status.current~run (units where their host machine-status current contains "run")

controller/0
juju-dashboard/0

We have filtered deep into the units, traversing to their machines. But, this is not even the final power level: juju-jockey units.host.hostname host.machine-status.current~run (now, let's get their hostnames)

juju-0df1b5-0
juju-0df1b5-1

We can now traverse everywhere.

I am sure there is some win with strong typing that I am not immediately seeing.

It will get weirder when I start parsing dates and such.

johnlettman commented 2 months ago

Also, when we start using mypy it will be unhappy about us spamming Any everywhere for Juju status. Since we can, it's just nice to have it consistent.

johnlettman commented 2 months ago

Lastly, #36 is closed by this PR.

ls -l ~/.cache/jockey/

total 10
-rw-rw-r-- 1 jlettman jlettman 3184 Aug 25 00:39 localhost_localhost-localhost_controller_status.json
-rw-rw-r-- 1 jlettman jlettman  324 Aug 25 00:47 localhost_localhost-localhost_test-model_status.json

This keeps them all separate (including remote Juju). I test for several variations of local Juju this way:

    @staticmethod
    def is_localhost_address(address: Optional[str]) -> bool:
        return (
            not address
            or address in ("local", "localhost", "localhost.localdomain", "127.0.0.1", "::1", "loopback")
            or ip_address(address).is_loopback
        )

The Cloud class normalizes these variations to localhost:

    def __str__(self) -> str:
        return "localhost" if self.localhost else self.host

And, that gets used as the cloud name for the cache:

        return self.cache.entry_or(
            str(self),
            controller,
            model,
            "status",
            lambda: self.run_juju_json(f"status --format=json --model {shell_quote(model_reference)}"),
        )

johnlettman commented 2 months ago

(sorry for the monster PR again, got carried away a bit)

LCVcode commented 2 months ago

The implications of your dot-notation are ridiculous. I appreciate that you implemented this in a way that preserved the existing query syntax.

Big +1 from me.

Regarding mypy, there are times when Any is exactly the tool for the job. I believe that --disable-error-code will allow us to suppress the results appropriately.

johnlettman commented 2 months ago

The implications of your dot-notation are ridiculous. I appreciate that you implemented this in a way that preserved the existing query syntax.

Big +1 from me.

Awesome! I aimed not to disturb anything existing. Though the temptation was there to add sub-commands, it would ruin the query language as it was.

As another added note:

juju-jockey units.

The period there dumps the entire JSON instead of the names (as the original syntax does).

Regarding mypy, there are times when Any is exactly the tool for the job. I believe that --disable-error-code will allow us to suppress the results appropriately.

You do have a point there. I haven't investigated too deeply. I think the types are purely development experience, and this PR could be made without that.

Furthermore, I will keep this as do not merge until I have this stuff thoroughly tested with unit tests. I don't want this to be a considerable change without validation. However, it's definitely at a place where it can be played.

johnlettman commented 2 months ago

@LCVcode for traversing "virtual relationships" that do not exist in the actual status (e.g., unit -> machine), I am considering using an indicator like $ to mark it separately.

This arises from a problem where unit already has a "machine" field filled with the Juju machine ID number. At the moment, the actual resolution of the machine object associated with the unit happens on host, but this feels unnatural. $machine populating the actual machine object might be nicer.

What is your opinion?

Current

```python { 'name': 'juju-dashboard/0', 'host': { 'juju-status': {'current': 'started', 'since': '19 Aug 2024 13:39:53-04:00', 'version': '3.3.1'}, 'hostname': 'juju-0df1b5-1', 'dns-name': '10.0.8.18', 'ip-addresses': ['10.0.8.18'], 'instance-id': 'juju-0df1b5-1', 'machine-status': {'current': 'running', 'message': 'Running', 'since': '19 Aug 2024 13:37:39-04:00'}, 'modification-status': {'current': 'applied', 'since': '23 Aug 2024 14:49:52-04:00'}, 'base': {'name': 'ubuntu', 'channel': '22.04'}, 'network-interfaces': { 'eth0': {'ip-addresses': ['10.0.8.18'], 'mac-address': '00:16:3e:b5:38:3c', 'gateway': '10.0.8.1', 'space': 'alpha', 'is-up': True} }, 'constraints': 'arch=amd64', 'hardware': 'arch=amd64 cores=0 mem=0M virt-type=container' }, 'workload-status': {'current': 'active', 'since': '19 Aug 2024 13:40:17-04:00'}, 'juju-status': {'current': 'idle', 'since': '23 Aug 2024 14:49:56-04:00', 'version': '3.3.1'}, 'leader': True, 'machine': '1', 'open-ports': ['8080/tcp'], 'public-address': '10.0.8.18' } ```

New

```python { 'name': 'juju-dashboard/0', '$machine': { 'juju-status': {'current': 'started', 'since': '19 Aug 2024 13:39:53-04:00', 'version': '3.3.1'}, 'hostname': 'juju-0df1b5-1', 'dns-name': '10.0.8.18', 'ip-addresses': ['10.0.8.18'], 'instance-id': 'juju-0df1b5-1', 'machine-status': {'current': 'running', 'message': 'Running', 'since': '19 Aug 2024 13:37:39-04:00'}, 'modification-status': {'current': 'applied', 'since': '23 Aug 2024 14:49:52-04:00'}, 'base': {'name': 'ubuntu', 'channel': '22.04'}, 'network-interfaces': { 'eth0': {'ip-addresses': ['10.0.8.18'], 'mac-address': '00:16:3e:b5:38:3c', 'gateway': '10.0.8.1', 'space': 'alpha', 'is-up': True} }, 'constraints': 'arch=amd64', 'hardware': 'arch=amd64 cores=0 mem=0M virt-type=container' }, 'workload-status': {'current': 'active', 'since': '19 Aug 2024 13:40:17-04:00'}, 'juju-status': {'current': 'idle', 'since': '23 Aug 2024 14:49:56-04:00', 'version': '3.3.1'}, 'leader': True, 'machine': '1', 'open-ports': ['8080/tcp'], 'public-address': '10.0.8.18' } ```

LCVcode commented 2 months ago

I need to think about this a bit. Would this change play into your dot-notation queries? If so, I would like to see relevant examples with and without this change.

At what level would this change be made? When the juju status gets generated and cached? We may want to double check how this works with subordinate charms. I cannot recall the json unit hierarchy offhand, but there are some bizarre inconsistencies between principal and subordinate units.

LCVcode commented 2 months ago

Either way, I think that this change belongs in a different PR. We should wrap this one up and capture this idea in a new issue.

LCVcode commented 2 months ago

Any estimate on when this PR might get wrapped up?

johnlettman commented 2 months ago

Either way, I think that this change belongs in a different PR. We should wrap this one up and capture this idea in a new issue.

Agreed.

Any estimate on when this PR might get wrapped up?

This weekend, most likely; sorry for the delay. :)

LCVcode commented 1 month ago

This weekend, most likely; sorry for the delay. :)

Yes! 🙏🏻 I am very excited to get my hands on some of your changes. I have some time next week to commit to this.

johnlettman commented 1 month ago

@LCVcode ready for initial review.

We now have CLI tests. Given the scope of the changes, it was appropriate to skeleton out the basis for testing a user's CLI interactions. At the moment, there are basic ones.

For testing, it may be helpful to have codecov.io running to gauge coverage, as there are quite a few steps on the developer side to get an idea of the overall coverage.

As mentioned, the prior CLI interactions will work as they did before. Dumping the entire object can be done by leaving a . after the object (e.g., juju-jockey unit.). You can traverse the object further (e.g., juju-jockey unit.juju-status).

We have more filters now:

(not) equals
(not) regex
greater than (or equals)
less than (or equals)
(not) contains

There is automatic type parsing for, e.g., booleans:

juju-jockey unit.juju-status leader=y -f tests/samples/k8s-core-juju-status.json

``` {'current': 'idle', 'since': '23 Mar 2024 13:29:59-06:00', 'version': '3.1.5'} {'current': 'idle', 'since': '23 Mar 2024 13:30:36-06:00', 'version': '3.1.5'} {'current': 'idle', 'since': '23 Mar 2024 13:31:34-06:00', 'version': '3.1.5'} {'current': 'idle', 'since': '23 Mar 2024 13:15:52-06:00', 'version': '3.1.5'} {'current': 'idle', 'since': '23 Mar 2024 13:12:05-06:00', 'version': '3.1.5'} {'current': 'idle', 'since': '23 Mar 2024 13:12:02-06:00', 'version': '3.1.5'} ```

LCVcode commented 1 month ago

There is so much going on here, it is going to take me a bit to grok it. You introduced many tools I have not seen before, which is not an issue, but I will have to close some of that gap after this PR is merged.

One small thing I wish I saw in the PR was module docstrings throughout, just to help punch through some of the initial ambiguity.

LCVcode commented 1 month ago

Can you please provide a list of the key changes in this PR? There are so many improvements that it is hard to follow the flow of each of them. I am specifically interested to understand how your dot-notation is implemented.

Also, should there be changes to the readme? I would really appreciate seeing some more example commands, if they are needed. Perhaps you already automated that away. I do not know.

LCVcode commented 1 month ago

John, you are a legend.

I added some nitpick comments as well as some clarifications. But seems excellent overall. Insane, actually.

johnlettman commented 1 month ago

One small thing I wish I saw in the PR was module docstrings throughout, just to help punch through some of the initial ambiguity.

I can do this, no problem! My one gripe was with the documentation system. It needs to include more functions I am used to and keeps generating scuffed outputs. I am considering pushing over to Sphinx with the Canonical theme. What do you think?

Can you please provide a list of the key changes in this PR?

Absolutely! I will create a quick change document so you can view every change. Given the breadth and scope here, it is only fair.

Also, should there be changes to the readme? I would really appreciate seeing some more example commands, if they are needed.

Oh definitely!

Perhaps you already automated that away. I do not know.

Not yet, sadly. It's a good idea -- maybe soon ;) different PR

I added some nitpick comments as well as some clarifications. But seems excellent overall. Insane, actually.

Will review, and thank you so much :) !

LCVcode commented 1 month ago

I can do this, no problem! My one gripe was with the documentation system. It needs to include more functions I am used to and keeps generating scuffed outputs. I am considering pushing over to Sphinx with the Canonical theme. What do you think?

+1 to that!

Will review, and thank you so much :) !

No, thank you.

johnlettman commented 1 month ago

I am specifically interested to understand how your dot-notation is implemented.

The magic happens in wrap_filter_action within filters.py.

Terms

action/FilterAction: Filtering function (a callable taking two variadic type inputs and returning boolean indicating whether the filter passes). When calling action, the value extracted from the item and the query are direct references.
item: This is the "collected" segment of the Juju status relevant to the requested Object. For example, this may be ONE machine, unit, or application.
field: This is a dot-notation reference to a sub-field within the item. For example, you could get "hostname" from within a Juju machine Object.
obj/Object: This is the user-requested Juju object upon which to filter. Again, this is a machine, unit, or application.
mode/FilterMode: A filtering mode, identified by operative tokens, associating a name and filtering action together.

Background

FilterAction functions look like this: https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L33-L35

They are declared on FilterMode: https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L90-L91

They use type abstractions: https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/abstractions.py#L172-L188

Using uses_typevar_params, we can determine whether all parameters are type abstractions, implying that we want to convert the string query to the type of the value we're comparing against in the filter: https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/abstractions.py#L191-L198

We use type_parser_for to grab the function to convert the types as required: https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L131-L134

Process

We will receive a call to parse_filter_expression with a string from the user. This string is one of the user-provided filter expressions (e.g., "hostname~juju-xyz-"). https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L161
We will traverse each FilterMode, test each token against the expression, and when there is a match, we will extract the field (left of token) and the query (right of the token). https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L165-L169
Now, with a match, we call wrap_filter_action with the identified mode and extracted field and query. https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L179
The wrap_filter_action function wraps around the action to create a filter function that operates only Object as a parameter. It integrates the user filter expression into a custom function. https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L140-L141
The dot-notation lookup is performed against the item using the user-supplied field string. https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L142-L146
Using reflection against the filter action base function parameters, we automatically determine whether a type-parser to match the user-supplied query type against the type of the value within item is necessary. We need this for operations like greater than, where we may want to parse the query (a string) into an integer to match that of an integer inside of the item. https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L148-L154
Now, we can use the base action. https://github.com/LCVcode/jockey/blob/53672b2507b860ae41266ecb225ab261554e57a1/src/jockey/filters.py#L156

johnlettman commented 1 month ago

WIP for Canonical-style Sphinx

LCVcode commented 1 month ago

Thank you for the explanation! I had figured most of that out, but there were a few key elements that were eluding me.

Let me know when you are ready for me to squash and merge this PR. Tests are passing, so we can proceed whenever.

By the way, we probably want to bump the version for this. Considering the size of this upgrade, I'd say we should go to 0.2.0.

johnlettman commented 1 month ago

Agree on version. I do want to do the rest of the code docs real quick before merge so it's nice and clean.

johnlettman commented 1 month ago

Okay, I think we're good @LCVcode. I double-checked CodeQL for any issues. Nothing reported.

Docs should be in the new format now. Make sure you enable "pages" support using actions. It should generate a site.

If you don't mind, I can follow up with PRs to expand on examples, etc. It's a good place to branch out into further development concerns.

LCVcode / jockey