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

Define a Kapitan target per component #223

Closed
simu opened this issue Oct 20, 2020 · 8 comments
Closed

Define a Kapitan target per component #223

simu opened this issue Oct 20, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request RFC Request for Comments

Comments

@simu
Copy link
Member

simu commented Oct 20, 2020

Context

We've found some instances (e.g. #156) where it would be beneficial to have a Kapitan target per component. For #156, having specific targets per component would allow us to reduce the amount of information that must be provided for the postprocessing filter definition (e.g. the component defining the filter would be clear based on the target).

There are some challenges in the implementation, if we want to make this change in a backwards-compatible manner. The current architecture relies on the single target Kapitan inventory for a number of features, the big ones being the dynamic hierarchy and component discovery. Additionally, some approaches for implementing this change would require refactoring all component classes.

Below we outline an approach which should be backwards compatible for the dynamic hierarchy and component discovery implementation, while not requiring any refactoring in the existing components.

Proposal

The best approach which doesn't require component refactoring and allows us to reuse the current implementation for the dynamic hierarchy and component discovery is to reorganize the Kapitan inventory slightly.

The new inventory structure will be constructed with empty files in classes/components/ for each component. This way, the existing class includes of form components.<component-name> do not pull in the component class which has the parameters.kapitan key anymore. The actual component classes will be made available in classes/_components (exact naming TBD).

With this inventory structure, each target can have the dynamic hierarchy class include of global.commodore without defining compilation instructions for each component. Each target still needs to include all component default classes, to ensure all inventory references can be resolved. Additionally each target will need to include class _components.<component-name> to pull in the Kapitan compile instructions for the targeted component.

A target will have the following form:

classes:
  - [ ... all component defaults for the cluster ... ]
  - global.commodore
  - _components.<comonent-name>

parameters:
  kapitan:
    vars:
      target: <component-name>

Alternatives

An alternative approach which requires refactoring of all components would be to change components to define their Kapitan configuration in parameters.<component-name>.kapitan instead of parameters.kapitan. This allows us to continue using the component classes in the hierarchy exactly the same way as previously.

Commodore could then generate individual targets per component where parameters.kapitan is extended as follows

parameters:
  kapitan: ${<component_name>:kapitan}

Each component is then responsible that their parameters.<component-name>.kapitan entry has vars.target set to <component-name>.

@simu simu added the enhancement New feature or request label Oct 20, 2020
@simu simu added this to To do in Project Syn Focus Weeks Oct 2020 via automation Oct 20, 2020
@simu simu self-assigned this Oct 20, 2020
@simu simu moved this from To do to In progress in Project Syn Focus Weeks Oct 2020 Oct 20, 2020
@simu simu added the RFC Request for Comments label Oct 20, 2020
@srueg
Copy link
Contributor

srueg commented Oct 20, 2020

Reclass also has the concept of aplications (basically a list of strings?). Maybe we can use this to specify the components to include in the hierarchy? (To prevent creating these empty class files).
This would require a refactoring of the whole inventory and moving the components.$component_name includes to the applications: array.

This could also allow the removal of components further "down" the hierarchy by using the ~ prefix in the applications array (xref #71 ).

@simu
Copy link
Member Author

simu commented Oct 20, 2020

Thanks for the hint about applications, I'll have a look at those as well. Looks like that approach might work for component includes in the hierarchy.

@srueg
Copy link
Contributor

srueg commented Oct 20, 2020

I'm not sure if there's anything special about the applications array. We could also call it components for example:

classes:
- cloud.cloudscale

components:
- csi-cloudscale

parameters:
   csi_cloudscale:
    some: value

@srueg
Copy link
Contributor

srueg commented Oct 20, 2020

Basically the idea boils down to having a separate data structure to define components which should be included (instead of using classes: directly) and having Commodore generate the appropriate class includes for each component.

@simu
Copy link
Member Author

simu commented Oct 20, 2020

I'm not sure if there's anything special about the applications array. We could also call it components for example:

Turns out it needs to be called applications for the reclass magic to work, cf. https://github.com/kapicorp/reclass/blob/1869c478624b37d3479d6c4ea4769dc9ec41fadd/reclass/core.py#L253-L284

edit: hm, no maybe not, having a closer look.

@srueg
Copy link
Contributor

srueg commented Oct 20, 2020

I also did a quick test with components but this (obviously in hindsight) doesn't work since only things in classes, applications and parameters are merged together (the rest is dropped).

@simu
Copy link
Member Author

simu commented Oct 20, 2020

There's also some interesting (buggy?) behavior with regard to reclass's documented feature of prefixing entries in applications with a ~: due to the way the recursive processing of the applications field is managed, dropping entries doesn't always work.

For example, with the following snippets, the final value of applications for targets/test.yml is ["argocd", "resource-locker", "steward", "storageclass"].

# targets/test.yml
classes:
  - global.kubernetes
  - tenant.cluster
# global/kubernetes.yml
applications:
- argocd
- resource-locker
- steward
- storageclass
# tenant/cluster.yml
applications:
- ~storageclass

I've spent a good amount of time investigating this issue in reclass, and have found the cause in https://github.com/projectsyn/reclass/blob/2585bfc620ddd8c2cd7d159c4eb6d117a526a819/reclass/core.py#L148-L159, where the calls to _recurse_entity while processing the entries in classes are passed merge_base=None, which has the effect that the applications field is processed in isolation for each class included in the same file (class or target). Since entries prefixed with ~ in applications are not preserved across calls, but immediately resolved during the calls to merge_base.merge(), we get the behavior outlined above.

The fix appears to be to change the linked snippet to the version present in https://github.com/projectsyn/reclass/tree/fix/application-removal. Note that this change needs kapicorp/reclass#4 to be merged to work correctly.

simu added a commit to projectsyn/component-argocd that referenced this issue Oct 22, 2020
With projectsyn/commodore#223 we're moving to
using the reclass `applications` array to define which components are to
be included, instead of directly including component classes in the
hierarchy.

Therefore the component needs to check in `inv.applications` instead of
`inv.classes` whether synsights-metrics is enabled to decide whether to
deploy the ArgoCD monitoring.
simu added a commit to projectsyn/getting-started-commodore-defaults that referenced this issue Oct 22, 2020
@simu
Copy link
Member Author

simu commented Oct 27, 2020

Implemented in #227.

@simu simu closed this as completed Oct 27, 2020
Project Syn Focus Weeks Oct 2020 automation moved this from In progress to Done Oct 27, 2020
@simu simu mentioned this issue Dec 14, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request RFC Request for Comments
Projects
No open projects
Development

No branches or pull requests

2 participants