kapitan
kapitan copied to clipboard
Add opt-in support for `reclass-rs`
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-rsas an optional dependency - Add new command line flag
--use-reclass-rsforkapitan inventory,kapitan compile,kapitan searchvarandkapitan refs - Extend inventory and remoteinventory test cases to be executed with reclass-rs if the package is installed
Looks good! Just some things:
- As I'm currently working on a pluggable inventory system, I would offer, to migrate your
reclass_rsinto the new system. It seems really easy with just the call toReclass(...). 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?
- 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
- When do you think missing features like
compose_node_namewill 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.
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.pyinkapitan/inventory - create new class with method
render_targets() - create cli arg in the
inventory_backend-parser - select the inventory in
resources-->get_inventory()
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_rscompatible 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.
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!