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

Add reclass-rs inventory backend #1059

Merged
merged 10 commits into from
Mar 15, 2024
Merged

Conversation

simu
Copy link
Contributor

@simu simu commented Sep 20, 2023

As announced in the Kapitan Slack channel, we (@projectsyn / @vshn) have started a reimplementation of Reclass in Rust: https://github.com/projectsyn/reclass-rs

This PR adds opt-in support for reclass-rs in Kapitan. The change is modelled after the opt-in support for gojsonnet.

Proposed Changes

  • Add Python package reclass-rs 0.4.0 as an optional dependency. This version supports compose_node_name. If you're relying on compose-node-name splitting target names on dots in the file name, you'll need to set the compose-node-name-literal-dots compatibility flag. An example is shown in the reclass-rs tests
  • Implement ReclassRsInventory as new inventory backend. Users can use reclass-rs by specifying --inventory-backend=reclass-rs
  • Extend inventory and remoteinventory test cases to be executed with reclass-rs if the package is installed
  • Add test case for compose-node-name (same test as introduced in fix: implement compose_target_name #1138)
  • Don't ignore *reclass* in make test_coverage anymore
  • Add documentation for reclass-rs as the inventory backend
  • Install reclass-rs in container image

Copy link
Contributor

@MatteoVoges MatteoVoges left a comment

Choose a reason for hiding this comment

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

Looks good!
Just some things:

  1. As I'm currently working on a pluggable inventory system, I would offer, to migrate your reclass_rs into the new system. It seems really easy with just the call to Reclass(...). This would work that in the beginning an inventory backend gets selected and then we don't need all this flag passing.
  2. Did you made some measurements on how much the performance increases. Because then we can write a cool docs / blog entry about it.
  3. When do you think missing features like compose_node_name will be ready? Maybe it would make sense to wait with the merge for that.

@simu
Copy link
Contributor Author

simu commented Sep 22, 2023

Looks good! Just some things:

  1. As I'm currently working on a pluggable inventory system, I would offer, to migrate your reclass_rs into the new system. It seems really easy with just the call to Reclass(...). This would work that in the beginning an inventory backend gets selected and then we don't need all this flag passing.

Seems like a good idea, I'm not in a hurry to get this merged, see below as well. I'd be happy to refactor this onto the pluggable inventory system. What's your expected timeline for the pluggable inventory system?

  1. Did you made some measurements on how much the performance increases. Because then we can write a cool docs / blog entry about it.

I have some measurements on our inventories, performance of kapitan inventory is much better (the output of kapitan inventory is 22MB YAML, on a Ryzen 5 5600x with stock speeds):

reclass-rs (with a custom print showing the actual inventory rendering time):

$ time kapitan inventory --use-reclass-rs > inv.yaml
running reclass_rs
Inventory (reclass_rs) took 0:00:01.960478

real	0m20.835s
user	0m38.864s
sys	0m1.195s

Python reclass:

$ time kapitan inventory > inv.yaml
running reclass
Inventory (reclass) took 0:01:19.215465

real	1m37.643s
user	1m37.384s
sys	0m0.217s
  1. When do you think missing features like compose_node_name will be ready? Maybe it would make sense to wait with the merge for that.

It's unlikely that this will happen very quickly, since we don't use that feature. I'd assume that it will be at least 6 months (unless we get external contributions) until we reach feature parity with the Python reclass implementation.

@github-actions github-actions bot added the Stale label Nov 22, 2023
@MatteoVoges
Copy link
Contributor

Hey @simu , the inventory interface is finally ready. Please give it a review ( #1106 ) and check if I missed something in functionality 😅

After we merged this, these would be the steps to have reclass_rs compatible with the interface:

  • create new file reclass_rust_inventory.py in kapitan/inventory
  • create new class with method render_targets()
  • create cli arg in the inventory_backend-parser
  • select the inventory in resources --> get_inventory()

@github-actions github-actions bot removed the Stale label Jan 27, 2024
@simu
Copy link
Contributor Author

simu commented Jan 27, 2024

Hey @simu , the inventory interface is finally ready. Please give it a review ( #1106 ) and check if I missed something in functionality 😅

After we merged this, these would be the steps to have reclass_rs compatible with the interface:

* create new file `reclass_rust_inventory.py` in `kapitan/inventory`

* create new class with method `render_targets()`

* create cli arg in the `inventory_backend`-parser

* select the inventory in `resources` --> `get_inventory()`

Hey @MatteoVoges

I tried to implement support for reclass-rs on the linked PR, and have noticed some things:

First off, the new inventory backend flags don't appear to be ever used in get_inventory() from what I can see. Also, I'd probably switch the flags to have something like

    inventory_backend_parser = argparse.ArgumentParser(add_help=False)
    inventory_backend_parser.add_argument(
        "--inventory-backend",
        action="store",
        default=from_dot_kapitan("inventory_backend", "inventory", "reclass"),
        choices=["reclass", "reclass-rs"],
        help="Select the inventory backend to use (default=reclass)",
    )

To make sure cached.args["inventory-backend"] contains the selected backend I've added the following in main():

    cached.args["inventory-backend"] = args.inventory_backend

Then we can just do something like the following in get_inventory():

    inventory_backends: dict(str, Inventory) = {
        "reclass": ReclassInventory,
        "reclass-rs": ReclassRsInventory,
    }

    # select inventory backend
    backend_id = cached.args.get("inventory-backend")
    backend = inventory_backends.get(backend_id)
    inventory_backend: Inventory | None = None
    if backend != None:
        logger.debug(f"Using {backend_id} as inventory backend")
        inventory_backend = backend(inventory_path)
    else:
        logger.debug("No backend specified, falling back to reclass as inventory backend")
        inventory_backend = ReclassInventory(inventory_path)

Second, there seems to be a bug in ReclassInventory's get_targets() which mangles the included classes. Also note that the logic to reconstruct the list of classes is unnecessary, you can just do self.targets[target_name].classes = rendered_target["classes"]). I've gone ahead and have opened a PR to address this issue: #1126.

Let me know how you think we should proceed regarding the command line flags to select inventory backends.

Edit: See #1127 for a Draft PR which implements the proposed change for the command line flags.

@MatteoVoges
Copy link
Contributor

You're right. The selection via cli args is only on my omegaconf branch and would then have been included, but your fix / suggestion looks good, so I think I will adapt my changes.

The selection of the inventory with choices is a good alternative to having one flag per inventory backend. 👍

Thanks for your feedback. I will have a look at your pr's in a minute!

@simu simu force-pushed the feat/reclass-rs branch 2 times, most recently from 1edda0e to fb39689 Compare February 26, 2024 13:17
@simu simu changed the title Add opt-in support for reclass-rs Add reclass-rs inventory backend Feb 27, 2024
@simu simu force-pushed the feat/reclass-rs branch from fb39689 to e52d5ce Compare March 4, 2024 16:10
@simu simu marked this pull request as ready for review March 4, 2024 16:10
@MatteoVoges MatteoVoges self-requested a review March 4, 2024 16:19
@simu simu force-pushed the feat/reclass-rs branch 4 times, most recently from 4a19ed3 to 75d959e Compare March 5, 2024 11:24
simu and others added 10 commits March 7, 2024 16:56
…for `get_reclass_config()`

By introducing these parameters, we don't need to override as many
options in the returned reclass config. Additionally, this change
introduces a clean interface for getting Kapitan's expected Reclass
config for the reclass-rs integration.
This commit introduces a new optional parameter for
`get_reclass_config()` which allows callers to disable the normalisation
of the `nodes_uri` and `classes_uri` parameters.

This option is required for reclass-rs, which expects these parameters
to be relative to the inventory path, rather than relative to the
working directory.
We reuse `get_reclass_config()` in the `ReclassRsInventory` backend.
Additionally, we make the backend selectable through the
`--inventory-backend` CLI option.
This commit is based on the test case introduced in PR kapicorp#1138.

We move the compose-node-name test implementation to `tests/` so it's
stored in the same place as the other tests and make the test class
inheritable so that we can reuse the test for reclass-rs.

Additionally we extend the test to check that all targets found by
`search_targets()` are rendered by `render_targets()`. This is needed
since we don't use reclass(-rs)'s target discovery logic in
`search_targets()` but instead use a simplified version that's
implemented directly in Kapitan's inventory backend base class.

However, `render_targets()` then renders whatever targets reclass(-rs)'s
target discovery finds, so if there's a mismatch we'd want to be
informed.

Co-authored-by: Matteo Voges <matteo.voges@nexenio.com>
Co-authored-by: Simon Gerber <simon.gerber@vshn.ch>
We implement the timing simply by executing `datetime.now()` before and
after the call to the method which renders the reclass inventory.
…lass*`

This is not required anymore since we don't import reclass as a Python
submodule anymore.
@simu simu force-pushed the feat/reclass-rs branch from 85bd7bf to 7f01abc Compare March 7, 2024 15:56
@ademariag ademariag self-requested a review March 15, 2024 11:12
@ademariag ademariag merged commit 1ff90b9 into kapicorp:master Mar 15, 2024
8 checks passed
@simu simu deleted the feat/reclass-rs branch March 18, 2024 08:56
simu added a commit to projectsyn/kapitan that referenced this pull request Mar 18, 2024
…s-rs backend

We forgot to update kapicorp#1059 to also initialize the `applications` and
`exports` fields in the target dataclass in the reclass-rs backend. This
commit updates the reclass-rs backend to correctly initialize these
fields to match the fix in kapicorp#1142.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants