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 workload config object to the desired state #267

Closed
10 tasks done
windsource opened this issue May 6, 2024 · 16 comments
Closed
10 tasks done

Add workload config object to the desired state #267

windsource opened this issue May 6, 2024 · 16 comments
Assignees
Labels
enhancement New feature or request. Issue will appear in the change log "Features"
Milestone

Comments

@windsource
Copy link
Contributor

windsource commented May 6, 2024

Description

Currently the only way to configure workloads is to add configuration parameters to the runtime. This enhancement shall allow to decouple configuration from workload such that the same workload can be used with different configurations. For that a new object config shall be created that also resides in Ankaios manifests parallel to workload.

A config shall contain key-value pairs that can be used as environment variables, command-line arguments or as read-only configuration files to be provide to the workload.

To limit resource usage (probably the config map will be part of the complete state and thus consume RAM) we should limit the size of a config to a maximum value (e.g. 1MB).

For the values, text and binary (base-64 encoded) data shall be allowed.

When providing a config value as file then owner and mode shall also be configurable.

Maybe we reuse the same format as the Kubernetes ConfigMap, for example:

configs:
  myconfig:
    binaryData:
      mykey1: <base64 encoded data>
    data:
      mykey2: bla
      mykey3: |
        foo=bar
        name=alice

It should be possible that one workload uses several configs. It should also be possible that all key/value pairs of one config are expanded to environment variables.

Goals

Separate config and workload such that the same workload can be used with different configurations. That allows also updating the config separately from the workload.

Final result

Summary

The desiredState now supports the configs field for defining configuration items that can be assigned to workloads. The workload configuration has a configs field to assign configs of the desriedState.configs by defining an config name alias as key and the key of the config item as value. For rendering workload fields with configuration items, the handlebars-rs Rust crate was introduced. It supports a sufficient and not overwhelming feature set for the config rendering use case (variables, loops, conditionals).

The following workload fields are supporting templated strings:

  • agent
  • runtimeConfig

Furthermore, for config aliases and config keys they are pointing to and also the config keys in the desriedState.configs field a character check was added. Supported are only normal characters, '_' and '-' (same like for workloads).

The Ankaios server renders the state at each update of the state (startup, subsequent updates) and only if workloads having configs assigned. The Ankaios server stores the rendered workloads separated from the CompleteState to avoid excessive on-the-fly rendering when Ankaios components like Ankaios agents, etc connects and to be able to return the unrendered state when requesting the CompleteState.

Tasks

  • Add configuration object to DesiredState
  • Use template engine when starting a workload
  • Update workload if an update of the configuration object changes the workload
  • Adapt and add utests for server configs logic and detailed update logic of configs
  • Adapt ank apply command to support workload configs
  • Add utests for ank apply changed logic and fix the utest utest_generate_state_obj_and_filter_masks_from_manifests_no_workload_provided (the prod code does now an exit(0))
  • Add stests for configs
  • Enhance the check for config item keys to allow only [a-zA-Z0-9-_] (same like workloads), requirements + utests
  • Implement shell completion for configs #391
  • Add a new subcommands ank delete config and ank get configs #392
@windsource windsource added the enhancement New feature or request. Issue will appear in the change log "Features" label May 6, 2024
@krucod3 krucod3 added this to the v0.5 milestone May 17, 2024
@krucod3 krucod3 changed the title Separate config from workloads Add workload config objects to the desired state Jun 26, 2024
@krucod3 krucod3 changed the title Add workload config objects to the desired state Add workload config object to the desired state Jun 26, 2024
@krucod3
Copy link
Contributor

krucod3 commented Aug 6, 2024

Basic example:

desiredState:
  workloads:
    workload1:
      runtime: podman
...
      runtimeConfig: |
        image: docker.io/nginx:latest
        commandOptions: ["-p", "{{nginxPort}}:80"]

configs:
  myconfig:
    binaryData:
      mykey1: <base64 encoded data>
    data:
      nginxPort: 8081
      mykey3: |
        foo=bar
        name=alice

@windsource
Copy link
Contributor Author

For inserting the config data into the workload's runtimeConfig, a template engine could be used. For Rust there is a list of available template engines.
Some criteria for selecting a template engine might be:

  • Supporting loops
  • Low resource usage
  • Widely used
  • Maintenance in place

Some candidates:

Name Comment
Tera Template engine inspired by Jinja2 and the Django template language
Askama It generates Rust code from the template. As for Ankaios the template will only be provided on runtime, that does not seem to fit.
Handlebars Handlebars is largely compatible with Mustache templates. Very popular

When using a template engine in the runtimeConfig, then environment variables and command-line arguments can easily be created using config objects.
A tricky thing seems to be the creation of read-only config files. The runtimeConfig could be used to mount an existing file but that file probably needs to be created before that in the host file system based on the config object.

@krucod3
Copy link
Contributor

krucod3 commented Sep 3, 2024

The following yaml show how the configs could look like:

desiredState:
  workload:
    dashboard:
      ...
      configs:
        c1: dashboard_configs
        ports: ports
      files:
        - mount_point: "/some/path/to/file.json"
          data: {{c1.json_data_file_content}}
        - mount_point: {{/some/path/to/bin_data}}
          binary_data: {{c1.some_data}}
      runtimeConfig: |
        image: fancy_dashboard:latest
        commandOptions: [ "-p", "{{ports.access_port}}:80"]
        commandArgs: [ "echo", "{{c1.echo_value}}"]
  configs:
    dashboard_configs:
      echo_value: "Hello Ankaios"
      json_data_file_content: |
        {
          some: {
            random: string
            object: 5
          }
        }
      some_data: dGhpcyBpcyBhIGJpbmFyeSBmaWxl
    ports:
      access_port: 8081

@windsource suggested the aliasing in the configs attribute of a workload to make it more obvious which config items are used inside a workload configuration.
The configs attribute containing the config data must be part of the Ankaios manifest (located under desiredState).
@christoph-hamm suggested the files attribute making it easier to mount (or link in case of a native workload) files needed by workloads.

@christoph-hamm christoph-hamm self-assigned this Sep 5, 2024
christoph-hamm added a commit that referenced this issue Sep 9, 2024
christoph-hamm added a commit that referenced this issue Sep 12, 2024
Before this fix it was not possible to run the tests from the common
folder. This also prohibited running individual test from Visual
Studio Code (e.g. for debugging).

Issue-Id: #267
christoph-hamm added a commit that referenced this issue Sep 12, 2024
christoph-hamm added a commit that referenced this issue Sep 13, 2024
christoph-hamm added a commit that referenced this issue Sep 17, 2024
christoph-hamm added a commit that referenced this issue Sep 18, 2024
christoph-hamm added a commit that referenced this issue Sep 18, 2024
christoph-hamm added a commit that referenced this issue Sep 26, 2024
christoph-hamm added a commit that referenced this issue Sep 26, 2024
christoph-hamm added a commit that referenced this issue Sep 26, 2024
@inf17101
Copy link
Contributor

inf17101 commented Sep 27, 2024

The following yaml show how the configs could look like:

desiredState:
  workload:
    dashboard:
      ...
      configs:
        c1: dashboard_configs
        ports: ports
      files:
        - mount_point: "/some/path/to/file.json"
          data: {{c1.json_data_file_content}}
        - mount_point: {{/some/path/to/bin_data}}
          binary_data: {{c1.some_data}}
      runtimeConfig: |
        image: fancy_dashboard:latest
        commandOptions: [ "-p", "{{ports.access_port}}:80"]
        commandArgs: [ "echo", "{{c1.echo_value}}"]
  configs:
    dashboard_configs:
      echo_value: "Hello Ankaios"
      json_data_file_content: |
        {
          some: {
            random: string
            object: 5
          }
        }
      some_data: dGhpcyBpcyBhIGJpbmFyeSBmaWxl
    ports:
      access_port: 8081

@windsource suggested the aliasing in the configs attribute of a workload to make it more obvious which config items are used inside a workload configuration. The configs attribute containing the config data must be part of the Ankaios manifest (located under desiredState). @christoph-hamm suggested the files attribute making it easier to mount (or link in case of a native workload) files needed by workloads.

I am reviewing the PR #379 adding the necessary data types. Afterwards I will continue on the implementation.
However, When looking into the code, I have found out, that the current data types for an ConfigItem is not covering numbers and there is an parsing error when using the example from the last proto type above:

...
configs:
...
  ports:
    access_ports: 8081

Parsing error: [2024-09-27T12:51:56Z ERROR] Parsing start config failed with error: 'configs: data did not match any variant of untagged enum ConfigItem at line 14 column 3'

To make the server accept the config every literal as value must be a string so currently I need "8081" as string to get it work.

Shall we also allow numbers? Otherwise I have to explicitly write the port in quotes.

@christoph-hamm
Copy link
Contributor

I am strictly against allowing numbers in the ConfigItem.

We plan to only use these configs for string templates. With config strings this is easy. The placeholder in the template is replaced by the value of the config.

For numbers the config is first parsed into some internal representation and for replacing the template placeholder this internal representation is then converted to a string. This string might differ from the string provided in the yaml config/manifest. This unnecessarily introduces potential errors and leaks implementations details.

@inf17101
Copy link
Contributor

inf17101 commented Sep 30, 2024

After discussing the config reference complexity with @windsource and @krucod3, we decided to allow only one level of the configs field when referencing configurations in workload objects (like in the current prototype above, e.g.: c1: dashboard_configs, ...)

In addition, we decided to treat every input of replacement as string since the implementation is easier in the first step without considering too many corner cases for replacing with numbers, floats and so on. So we update the prototype to use quotes for the port number.

New prototype:

desiredState:
  workloads:
    dashboard:
      ...
      configs:
        c1: dashboard_configs
        ports: ports
      files:
        - mountPoint: "/some/path/to/file.json"
          data: "{{c1.json_data_file_content}}"
        - mountPoint: "/some/path/to/bin_data"
          binaryData: "{{c1.some_data}}"
      runtimeConfig: |
        image: fancy_dashboard:latest
        commandOptions: [ "-p", "{{ports.access_port}}:80"]
        commandArgs: [ "echo", "{{c1.echo_value}}"]
  configs:
    dashboard_configs:
      echo_value: "Hello Ankaios"
      json_data_file_content: |
        {
          some: {
            random: string
            object: "5"
          }
        }
      some_data: dGhpcyBpcyBhIGJpbmFyeSBmaWxl
    ports:
      access_port: "8081"

christoph-hamm added a commit that referenced this issue Sep 30, 2024
Issue-Id: #267
Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>
christoph-hamm added a commit that referenced this issue Sep 30, 2024
christoph-hamm added a commit that referenced this issue Sep 30, 2024
christoph-hamm added a commit that referenced this issue Oct 1, 2024
christoph-hamm added a commit that referenced this issue Oct 1, 2024
inf17101 added a commit that referenced this issue Oct 1, 2024
* Add config to state object

Issue-Id: #267

* Fix problem with tests not building for common

Before this fix it was not possible to run the tests from the common
folder. This also prohibited running individual test from Visual
Studio Code (e.g. for debugging).

Issue-Id: #267

* Improve YAML serialization of ConfigItem

Issue-Id: #267

* Add internal config object

Issue-Id: #267

* Fix unit tests

Issue-Id: #267

* Fix clippy error about unequal enum sizes

Issue-Id: #267

* Remove unused import

Issue-Id: #267

* Interpret None fields as default instead of failing

Issue-Id: #267

* Add configs to filtered state used in the ank CLI

* Also update server state if non workload has been changed

* Add missing fields in tests

Issue-Id: #267

* Remove commented out code

Issue-Id: #267

* Fix year in copyright header of new files

Issue-Id: #267

* Apply suggestions from code review

Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>

* Remove commented out code

Issue-Id: #267
Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>

* Add missing use statement

Issue-Id: #267

* Allow missing workloads in UpdateStateRequeset

Issue-Id: #267

* Fix configs in sample objects

Issue-Id: #267

* Add missing config

Issue-Id: #267

* Fix filtering redundant mask

---------

Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>
Co-authored-by: Oliver Klapper <oliver.klapper@elektrobit.com>
@inf17101 inf17101 self-assigned this Oct 2, 2024
@inf17101
Copy link
Contributor

inf17101 commented Oct 2, 2024

I decided to go with handlebars as template engine, because it is actively maintained, has an easy to use interface which fits well to our objects we work with and it contains just the minimum of required features covering the use cases for rendering configs (loops + if). It is not overblown with many other template engine features and the amount of fetched in dependencies is kept to manageable level. If more features are required in the future it is extensible through the so called helpers. I have already committed some first design decision into the requirements of the server.

@inf17101
Copy link
Contributor

inf17101 commented Oct 2, 2024

Noting down some corner cases to be considered during implementation:

  • The state is updated to delete the whole configs field content. What shall be done with the workloads in this case?
  • The state is updated to include workloads with template strings for the config but there is no config provided in the configs field which might be added later. Not rendering if there is no config items available and put the workload to pending state or throwing an error that configs are missing?

@inf17101
Copy link
Contributor

inf17101 commented Oct 4, 2024

@HorjuRares: I have created a fork in my private account, so that we can work both on the task. I thought about tasks that you can take over:

  • add the same character check (as for workload names you already did) for the keys in the CompleteState's field configs and inside the workload.configs attribute where they are referenced (swdd + impl + utests). No length check of the an config key is required. Allowed characters: "[a-zA-Z0-9-_]" just take the same like for workload names.
  • Enhance the interfaces and add the files attribute to the StoredWorkloadSpec and to the WorkloadSpec:
...
  dashboard_workload:
    ....
    files:
      - mount_point: "/some/path/to/file.json"
        data: {{c1.json_data_file_content}}
      - mount_point: "/some/path/to/bin_data"
        binary_data: {{c1.some_data}}
   ....
  • Adding stests for the config implementation (start with 1 general case + 1 update scenario)

You can take one of the tasks above if you want. Personally, I would recommend that you start with the config key check, since you know that code path best. If it is done you can move on to one of the other suggested tasks.

I have invited you to my fork, branch: 267_workload_config_impl. You should be able to commit there.

@inf17101
Copy link
Contributor

inf17101 commented Oct 7, 2024

I throw away the original idea of giving the ServerState its own type wrapping the CompleteState, which stores the workload objects unexpanded and expanded, because otherwise too many other fields (such as AgentMap, WorkloadStateDB) are also duplicated.

What I am doing now is to keep other parts of the ServerState stable, that I store only the expanded workload states inside the ServerState. I keep the desiredState storing the unexpanded workloads like it is to not break a lot of functionality like filtering the CompleteState inside the server parts (affecting all from impls and so on) and to make it possible to retrieve the unexpanded state over control interface and cli.

The StoredWorkloadSpec is the unexpanded workload and the WorkloadSpec is the expanded workload sent to agents. The StoredWorkloadSpec inside the nested CompleteState object is untouched so that the CompleteState and its members are not affected (that is used by a lot of code and also in the server). In addition, the server state only stores the collection of WorkloadSpecs and thus, I have only to change the source variable where the workloads are taken from when sending them to the agents and so on.

Currently, I am implementing the update logic, which I decided to compare the expanded workloads. Then, I can cover cases like config items changed or workload template config string changed easily.

@inf17101
Copy link
Contributor

inf17101 commented Oct 8, 2024

After implementing the update logic for workloads with configs, I have tested the following cases:

  • update of runtimeConfig and some config item (e.g. access port) the workload uses + update mask desiredState.workloads.dashboard => update of workload because of changed runtimeConfig, but runtimeConfig rendered with previous port number, because update mask does not consider the changed config item
  • update from previous step above, but with different update mask desiredState => workload is updated and runtimeConfig is rendered with new config item (access port), because the update mask includes now also the changed config item
  • update from previous step, but with update mask pointing only to the changed config item (access port): desiredState.configs.ports.access_ports => update of the workload referencing the access port config item
  • Update of config item only without changed workloads
  • Deletion of workloads

In addition, for optimization, a workload is not rendered, when no configs are assigned to that workload (empty configs field in the workload configuration). In this case all fields of the workload are not rendered and treated like the previous workload configuration without configs.

@inf17101
Copy link
Contributor

inf17101 commented Oct 9, 2024

@HorjuRares: I have created a fork in my private account, so that we can work both on the task. I thought about tasks that you can take over:

  • add the same character check (as for workload names you already did) for the keys in the CompleteState's field configs and inside the workload.configs attribute where they are referenced (swdd + impl + utests). No length check of the an config key is required. Allowed characters: "[a-zA-Z0-9-_]" just take the same like for workload names.
  • Enhance the interfaces and add the files attribute to the StoredWorkloadSpec and to the WorkloadSpec:
...
  dashboard_workload:
    ....
    files:
      - mount_point: "/some/path/to/file.json"
        data: {{c1.json_data_file_content}}
      - mount_point: "/some/path/to/bin_data"
        binary_data: {{c1.some_data}}
   ....
  • Adding stests for the config implementation (start with 1 general case + 1 update scenario)

You can take one of the tasks above if you want. Personally, I would recommend that you start with the config key check, since you know that code path best. If it is done you can move on to one of the other suggested tasks.

I have invited you to my fork, branch: 267_workload_config_impl. You should be able to commit there.

@HorjuRares: I have already implemented task 2 about the files field for mounting files into a workload. I have pushed it to a separate branch, since the files change and logic will be developed in a separate PR.

@inf17101
Copy link
Contributor

@HorjuRares: I have created a fork in my private account, so that we can work both on the task. I thought about tasks that you can take over:

  • add the same character check (as for workload names you already did) for the keys in the CompleteState's field configs and inside the workload.configs attribute where they are referenced (swdd + impl + utests). No length check of the an config key is required. Allowed characters: "[a-zA-Z0-9-_]" just take the same like for workload names.
  • Enhance the interfaces and add the files attribute to the StoredWorkloadSpec and to the WorkloadSpec:
...
  dashboard_workload:
    ....
    files:
      - mount_point: "/some/path/to/file.json"
        data: {{c1.json_data_file_content}}
      - mount_point: "/some/path/to/bin_data"
        binary_data: {{c1.some_data}}
   ....
  • Adding stests for the config implementation (start with 1 general case + 1 update scenario)

You can take one of the tasks above if you want. Personally, I would recommend that you start with the config key check, since you know that code path best. If it is done you can move on to one of the other suggested tasks.
I have invited you to my fork, branch: 267_workload_config_impl. You should be able to commit there.

@HorjuRares: I have already implemented task 2 about the files field for mounting files into a workload. I have pushed it to a separate branch, since the files change and logic will be developed in a separate PR.

The files mounted to a workload task is now a sub issue and I have posted the branch with all up-to-date information inside the sub issue.

@inf17101
Copy link
Contributor

I had a look to the ank apply logic with @krucod3 and we came to the conclusion that ank apply behaves correctly for configs and updates. We improved the user output (logs) to the user. Still I need to fix the utest utest_generate_state_obj_and_filter_masks_from_manifests_no_workload_provided since we changed the production code to exit(0) instead of throwing an error.

@inf17101
Copy link
Contributor

inf17101 commented Oct 17, 2024

I have covered the following State changes according to the table:

Configs cases

Update state cases

User action Expected Ankaios action covered by
Update config item with update mask pointint to changed configs Affected workloads shall be updated utest, stest
Update config items not affecting workloads No workloads shall be updated, but configs utest
Update used config item and runtimeConfig, update mask skips updated config Workload shall be updated, but with old config item utest
Update used config item and runtimeConfig, update mask includes both changes Workload shall be updated with new config item utest
Update used config item and runtimeConfig, update mask skips workload Workload shall be updated but only the used config item utest
RuntimeConfig changes Affected workloads shall be updated utest, stest
Remove unused config item No workloads shall be updated, but configs utest
Remove used config item or all config items ConfigRenderError, state not updated at all utest
Remove workload referencing config item Workload shall be removed, but configs kept utest
Add workload using a config not referenced ConfigRenderError, state not updated at all utest
Remove all config references, and keep templated config Workload not rendered, updated state and retries utest
Remove all config references and templated strings inside runtime config, change runtimeConfig Workload and state shall be updated utest
Replace templated string through same config item value Workload shall not be updated, but state utest

Startup cases

User action Expected Ankaios action covered by
Start server with templated Ankaios manifest Workloads shall be created, state contains unrendered config stest
Start server with invalid templated Ankaios manifest No startup, state rejected stest

krucod3 added a commit that referenced this issue Oct 29, 2024
* Small draft code to try out handlebars

* Write design decision for handlebars

* Add first swdds and first draft implementation

* Fix reference and swdd tags

* Add swdd for config renderer

* Update rendered workloads only when state is updated

* Implement update logic of expanded workloads

* Rename workload variables

* Skip rendering on no config assignment and introduce error type

* Fix update of config item only

* Remove in-place allocation of ConfigRenderer

* Add comment

* Fix server state utests

* Add config renderer utests

* Add update of configs utest and swdd

* Add configs update utests for server state

* Rename update_state method

* Ignore configs for agent name handling in ank apply

* Fix linkage in utest

* Add completions for configs in object field mask

Issue-Id: #267

* Improve log output in ank cli

* Update only desiredState

* Refactor set_state function and update plantuml

* Write swdd for comparing rendered workloads

* Split state format rendering

* Fix wrong dependencies workload names in test data

* Extract to constant var

* Remove not needed references

* First basic system test

* Rename stest file

* Remove notes file

* Make utest more strict

* Add more stests and link them

* Add utests for config key and alias validation

* Add config key format stest

* Add stests for workload config refs and fix linkage

* Add utests for ank apply

* Remove duplicated utest

* Adapt working with complete state section

* Disable wait in utest

* Merge main into branch and fix utest

* Get rid of conversion in utest

* Adapt quickstart doc

* Adapt startup-configuration user doc

* Extend user tutorial with configs

* Add update and delete config user doc

* Add comment about unrendered get complete state

* Fix rust contol interface example

* Add utest linkage

* Add linkage to utest for config rendering

* Fix missing swdd prefix in linkage

* Wording in comment

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Rename api_format method

* Fix naming convention swdd rationale

* Change swdd for selecting workloads for agents

* Add newline to swdd rationale

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Rename utest

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Use mockall double for import

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Output error message

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Space in utest linkage

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Change comment in utest

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Fix comment in stest documentation

* Fix comments for precondition

* Remove config_key from workload config map and insert utest

* Throw specific error upon not existing config key

* Adapt doc agent field description

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Change linkage to sub chapter

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

* Remove plurar

* Rename update state file in stest

* Revert to normal yaml string

Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>

---------

Co-authored-by: Holger Dormann <holger.dormann@elektrobit.com>
Co-authored-by: Kaloyan <36224699+krucod3@users.noreply.github.com>
@inf17101
Copy link
Contributor

inf17101 commented Nov 12, 2024

PR reviewed and merged. Summary written in the issue description. Issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. Issue will appear in the change log "Features"
Projects
None yet
Development

No branches or pull requests

4 participants