kapitan icon indicating copy to clipboard operation
kapitan copied to clipboard

Add opt-in support for `reclass-rs`

Open simu opened this issue 2 years ago • 4 comments

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 as an optional dependency
  • Add new command line flag --use-reclass-rs for kapitan inventory, kapitan compile, kapitan searchvar and kapitan refs
  • Extend inventory and remoteinventory test cases to be executed with reclass-rs if the package is installed

simu avatar Sep 20 '23 15:09 simu

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.

simu avatar Sep 22 '23 12:09 simu

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

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

MatteoVoges avatar Jan 26 '24 19:01 MatteoVoges

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.

simu avatar Jan 27 '24 12:01 simu

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!

MatteoVoges avatar Jan 27 '24 13:01 MatteoVoges