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

Proposal: Simplify the Jsonnet renderer entry point. #38

Closed
bwplotka opened this issue Jan 25, 2021 · 6 comments
Closed

Proposal: Simplify the Jsonnet renderer entry point. #38

bwplotka opened this issue Jan 25, 2021 · 6 comments

Comments

@bwplotka
Copy link

TL;DR: It's not trivial to understand how jsonnet renderer invokes jsonnet templates: Might be blocking adoption. Also indirection might leak Locutus specifics.

Problem Statement

  1. The way jsonnet renderer passes the input involves indirection which is
    • potentially surprising (it was for me)
    • leaks Locutus existence
    • makes it impossible to run those templates without Locutus

Context: jsonnet code has to be adjusted correctly in a way that makes it impossible to run without locutus. That's not a strong use case in itself, just confusing to me. In some way I expected Locutus renderer to execute my jsonnet files without my jsonnet files to even know about Locutus existence. If you look at our operator example the fact that this weird (maybe to me) generic-operator/config has to be added was a super surprising and significant learning step. And the main problem is that it leaks Locutus existence. Wonder if we can improve this 🤗

  1. The way jsonnet expects output is ambiguous and quite complex for basic use cases.

Context: This one might be a documentation issue, as there are too many options in my eyes. Or maybe it would just be more flexible on parsing side. As far as I get it it's something like:

{ 
   objects: // map of Kubernetes objects (or groups of Kubernetes objects, or groups of groups of k8s objects etc).
   rollout: // some specific locutus CR with specific API Version, name which unused (?) with groups which is a list of steps groups. Each steps group can contain list of  {object, action} where object is the specific first-level key of the very above object map.
}

(I had to reverse engineer this based on https://github.com/observatorium/operator/blob/master/jsonnet/main.jsonnet and locutus examples so I might be terribly wrong).

Even explaining this quickly here took me a while. I think we could improve this a little. Do we need to pass name and APIVersion? Do we need to pass both objects and link it in steps if we link only once? And what if I want just simple deploy (which should be 99% of use cases when you want to use e.g OLM and operator to deploy stateless applications).

Proposal

  1. Remove the requirement of putting placeholder: https://github.com/observatorium/operator/blob/master/jsonnet/obs-operator.jsonnet#L1
  2. Define hooking jsonnet templates into locutus renderer as follows:
  • entrypoint is a file function (file with top level function, or something like this) that takes a single argument (values JSON) and prints map with:
    • objects (I guess similar to the existing structure if we want flexibility in groups)
    • optionaly: rollout (I guess existing rollout structure)

I literally did that by wrapping locutus: https://github.com/observatorium/rndr/blob/main/pkg/rndr/jsonnet/renderer.go#L30, so I could use locutus in a simple "kube, apply" way as well. I think this is how we want to use locutus for production observatorium rendering with optional operator literally - move stateful operations to specific (maybe locutus based) operators, but the top level deployments being deployed in simple kube apply form OR optional operator kube apply loop - nothing rollout specific.

I know I did not change the output much, but I have not found a better way (:

What do you think? This is one reason why rndr was created so maybe we can improve Locutus on this front together 🤗

cc @paulfantom @s-urbaniak @kakkoyun for awareness.

@brancz
Copy link
Owner

brancz commented Jan 27, 2021

I think these are largely documentation issues, which is unsurprising, since I haven't touched any of the documentation since I put together the proof of concept.

  1. The examples here don't do a great job at this, but the way I have written jsonnet code with this is to have functions that any configuration gets passed to, and it just happens that in the main function it's the generic import. Production main.jsonnet that I'm using:
local cfgBuilder = import 'build_config.libsonnet';
local config = import 'generic-operator/config';

cfgBuilder.Build(import 'generic-operator/config')

The build_config.libsonnet code is entirely agnostic to the configuration passed to it, and is separately tested with unit tests.

I'm open to suggestions, but I don't know of any other way to inject dynamic content into a jsonnet VM. If you have any, I'm all ears.

  1. I think this is a documentation issue. objects is a flat object where every key must correspond to a json object, which each must be a full Kubernetes API object. The object keys can be organized however a user chooses, it's akin to object storage keys. The file renderer for example maps each full filepath in a directory structure as their key.

I'm happy with having the render only mode output files to disk with their key representing their filepath if that helps and have rollout be skipped if null/undefined/empty.

@bwplotka
Copy link
Author

bwplotka commented Jan 27, 2021

Thanks!

and is separately tested with unit tests.

There unit tests for this language? 😱 (this? https://github.com/yugui/jsonnetunit)

  1. I'm open to suggestions, but I don't know of any other way to inject dynamic content into a jsonnet VM. If you have any, I'm all ears.

I did here: https://github.com/observatorium/rndr/blob/main/pkg/rndr/jsonnet/renderer.go#L81
Probably we could contribute to jsonnet VM for a less hacky approach though.

The only question is - do we feel like expecting object with a certain name (e.g values) to be passed is a better approach or no. Now when I think about it and I know how it works, maybe it does not matter that much.

  1. Yes, docs and some defaults like this would be amazing. Let's contribute this then 🤗

@bwplotka bwplotka reopened this Jan 27, 2021
@bwplotka
Copy link
Author

Ups, accidential enter

@brancz
Copy link
Owner

brancz commented Jan 28, 2021

There unit tests for this language?

See "Assertions and Debugging": https://jsonnet.org/ref/stdlib.html


Regarding 1) I think we would be limiting ourselves by passing a single object. We actually already have examples of other dynamic imports where content is dynamically retrieved at import time, like here: e608a48

Last time I spoke to the Dave (creator of jsonnet) about this it was not really something they wanted, as it effectively removes the guarantee of jsonnet being hermetic, and I understand and respect that. For us it's effectively the same though as saving that content to a file and then executing the jsonnet VM.

@bwplotka
Copy link
Author

Agree.

@bwplotka
Copy link
Author

Let's close this and create cleaner issue for 2.

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

No branches or pull requests

2 participants