helm: add helm chart for plugins installations #45

fmuyassarov · 2023-05-17T14:51:26Z

This commit adds a helm chart to install balloons and topology-aware NRI plugins.

To install the chart into local k8s cluster without current default settings, simply running below is enough:

# install topology aware plugin
helm install topology-aware deployment/helm/resource-management-policies/topology-aware/

# install balloons aware plugin
helm install balloons deployment/helm/resource-management-policies/balloons

For testing and just only generating the final YAML without sending it to k8s

#  generate  topology aware plugin deployment with  dry run
helm install topology-aware deployment/helm/resource-management-policies/topology-aware/ --dry-run > deployment.yaml

# generate balloons plugin deployment with dry run
helm install balloons deployment/helm/resource-management-policies/balloons --dry-run > deployment.yaml

The images currently the Helm chart defaulting to are:
ghcr.io/containers/nri-plugins/nri-resource-policy-topology-aware:unstable
ghcr.io/containers/nri-plugins/nri-resource-policy-balloons:unstable

fmuyassarov · 2023-05-17T14:53:55Z

ping @klihub @jukkar
This is ready but there is only one thing I wanted to clarify and that's why I opened the PR as draft.
The container image is tagged as devel in some manifests and the one that is published on the GitHub is unstable.
I'm not sure how we want to the images to be tagged during templating.

fmuyassarov · 2023-05-17T14:55:04Z

Another thing, would you like me to add

documentation/instructions how to install the plugins with kustomize
documentation/instructions how to install the plugins with helm
GitHub action based Helm linter

klihub · 2023-05-23T13:56:53Z

ping @klihub @jukkar This is ready but there is only one thing I wanted to clarify and that's why I opened the PR as draft. The container image is tagged as devel in some manifests and the one that is published on the GitHub is unstable. I'm not sure how we want to the images to be tagged during templating.

We'll need to fix that. It would be nice if the user could somehow configure/select (absolute helm n00b speaking here) whether to go with a stable/devel/unstable version.

klihub · 2023-05-23T14:22:49Z

Another thing, would you like me to add

documentation/instructions how to install the plugins with kustomize

This would be nice to have as a separate PR.

documentation/instructions how to install the plugins with helm

This could be part of this PR.

GitHub action based Helm linter

Can you elaborate what does that mean ?

fmuyassarov · 2023-05-24T09:02:26Z

GitHub action based Helm linter

Can you elaborate what does that mean ?

Helm client has built in linting command to catch possible issues with charts and there is a github action that basically runs the lint command on github pull request. So, I was wondering, if we would need/want something like that. I don't have strong opinion here, because on one hand it is good to lint and catch possible bugs as early as possible (before merging and then realizing it) but on the other hand I don't expect to see many PRs against helm charts in the future. So, it doesn't really make sense to enable the GitHub action for every PR. GitHub action can be limited though to be executed only when charts directory is modified. So, I'm not sure to be honest.

jukkar · 2023-06-06T11:27:10Z

We need to update the documentation and add information about Helm after the docs pr #6 is merged.

fmuyassarov · 2023-06-06T13:01:44Z

We need to update the documentation and add information about Helm after the docs pr #6 is merged.

I have half cooked up PR for kustomize way of installing too. Will send once your PR is merged.

deployment/helm/nri-plugins/topology_aware_values.yaml

fmuyassarov · 2023-06-12T08:17:48Z

ping @klihub @jukkar @marquiz for review.

marquiz

Thsnk @fmuyassarov. some quick comments below. Are you planning to submit documentation as a separate PR?

deployment/helm/nri-plugins/Chart.yaml

deployment/helm/nri-plugins/balloons_values.yaml

deployment/helm/nri-plugins/templates/clusterrole.yaml

deployment/helm/nri-plugins/templates/daemonset.yaml

deployment/helm/nri-plugins/templates/noderesourcetopology.yaml

deployment/helm/nri-plugins/templates/role.yaml

fmuyassarov · 2023-06-12T09:20:53Z

Thsnk @fmuyassarov. some quick comments below. Are you planning to submit documentation as a separate PR?

yes, but can add here as well.

marquiz · 2023-06-12T09:34:01Z

yes, but can add here as well.

With this initial work, I'm fine with whatever you (or others think is preferred)

fmuyassarov · 2023-06-12T13:21:10Z

@marquiz thanks for the reviews. Addressed comments.

fmuyassarov · 2023-06-13T10:11:22Z

yes, but can add here as well.

With this initial work, I'm fine with whatever you (or others think is preferred)

Added in #62

deployment/helm/resource-policy/templates/daemonset.yaml

deployment/helm/resource-policy/topology_aware_values.yaml

marquiz · 2023-06-14T08:20:20Z

@fmuyassarov as I commented here I think we should have separate charts for the different policies and manage those as subcharts. When releasing the chart(s) in a helm repo we cannot rely on people relying on some policy-specific values file on their local machine. We need to have a chart that can be deployed with simple helm install and I don't know how to achieve that unless having separate (sub-)charts for each policy. WDYT?

fmuyassarov · 2023-06-14T09:53:25Z

@fmuyassarov as I commented here I think we should have separate charts for the different policies and manage those as subcharts. When releasing the chart(s) in a helm repo we cannot rely on people relying on some policy-specific values file on their local machine. We need to have a chart that can be deployed with simple helm install and I don't know how to achieve that unless having separate (sub-)charts for each policy. WDYT?

Yes, one way is to separate the plugins as stand alone charts instead of seperate values file.

From the previous comment

Hmm, I think we should have a main "values.yaml" for the chart, e.g. how else are we going to release the chart?

I'm not sure I understand this. If a user installs based on the main values.yaml what is that user going to get as a result? I mean what plugin? I think it would be correct to actually have separate charts per plugin as you suggested above, although most of the things (templates) are going to be duplicated. But that will give is stand alone chart which has it is own lifecycle.

marquiz · 2023-06-14T13:09:31Z

I think it would be correct to actually have separate charts per plugin as you suggested above, although most of the things (templates) are going to be duplicated. But that will give is stand alone chart which has it is own lifecycle.

Exactly (my comment was probably a bit cryptic). What I tried to say is that the chart can basically have only one values.yaml. You cannot choose between two different values.yaml in a chart when doing an install.

With subcharts you shouldn't need to duplicate the templates, right? Have common values there plus all the templates. Then just override required stuff in the subchart. Wouldn't that work and eliminate most of the duplication?

fmuyassarov · 2023-06-19T14:16:14Z

@marquiz @klihub @jukkar

Unfortunately, I was unable to succeed or I chose not to proceed with the Library chart, and here's why:

It has already become quite complicated, and I can't imagine how challenging it will be to maintain in the future. For instance, overriding the data part in the ConfigMap of the shared library alone requires at least three steps for a single parameterization.

Define a named function which encapsulated the DaemonSet definition

{{- define "resource-management-library.configmap.tpl" -}}
apiVersion: v1
kind: ConfigMap
metadata:
  name: nri-resource-policy-config.default
  namespace: default
data: {}
{{- end -}}
{{- define "resource-management-library.configmap" -}}
{{- include "resource-management-library.util.merge" (append . "resource-management-library.configmap.tpl") -}}
{{- end -}}

Define another named function in another .tpl file to merge image value coming from the Balloons Helm chart into the one written in step (1)

{{- /*
resource-management-library.util.merge will merge two YAML templates and output the result.
This takes an array of three values:
- the top context
- the template name of the overrides (destination)
- the template name of the base (source)
*/}}
{{- define "resource-management-library.util.merge" -}}
{{- $top := first . -}}
{{- $overrides := fromYaml (include (index . 1) $top) | default (dict ) -}}
{{- $tpl := fromYaml (include (index . 2) $top) | default (dict ) -}}
{{- toYaml (merge $overrides $tpl) -}}
{{- end -}}

Have include entry (or in other words simply call the named function) from within the Helm chart

{{- include "resource-management-library.configmap" (list . "balloons.configmap") -}}
{{- define "balloons.configmap" -}}
data:
  myvalue: "Hello World"
{{- end -}}

As you can see, this involves simply overriding the configMap data portion from the Balloons Helm chart. We would need to do the same for all other fields that we want the user to be able to override, such as ports, protocols, image versions, and so on.

However, what's even more problematic is that I'm currently unsure how users can perform these overrides. In the example above, the Balloons Helm chart is hard-coding the configMap data as "Hello World" within the file, and I don't know how to enable users to easily provide custom data from the terminal during chart installation. Essentially, the values.yaml file becomes useless in this kind of setup. Perhaps there is a way, but I have been unable to find it.

As such, I don't want to complicate it further and instead have separate charts with their own templates, even if they result in duplication. Sorry for switching back and forth between different approaches. It's just that none of them seem to be perfect, or there is not an easy way to override common boilerplate as there is, for example, in Kustomize.

If you're interested in seeing what this means in practice, you can take a look at my branch helm-monster branch, where I have kept changes for overriding comnfigMap and also DaemonSet if I recall it right.

fmuyassarov · 2023-06-19T14:20:25Z

and sorry for delaying the release. I just really didn't want to have duplication ....

klihub

If you @fmuyassarov and @marquiz consider this good enough to start with, then I'm also fine with it. I don't know enough about helm charts to be able to judge myself.

fmuyassarov · 2023-06-20T11:18:42Z

If you @fmuyassarov and @marquiz consider this good enough to start with, then I'm also fine with it. I don't know enough about helm charts to be able to judge myself.

There are a couple of improvements I would like to see in Helm charts. Like those we discussed over the call today. But it doesn't have to be done here but instead as a follow up PR. So, I think this is fine to merge it, but I would like @marquiz to have his words,

klihub · 2023-06-20T14:03:09Z

If you @fmuyassarov and @marquiz consider this good enough to start with, then I'm also fine with it. I don't know enough about helm charts to be able to judge myself.

There are a couple of improvements I would like to see in Helm charts. Like those we discussed over the call today. But it doesn't have to be done here but instead as a follow up PR. So, I think this is fine to merge it, but I would like @marquiz to have his words,

@marquiz ?

jukkar

LGTM

marquiz

Some comments, mostly about naming

deployment/helm/resource-management-policies/balloons/Chart.yaml

deployment/helm/resource-management-policies/balloons/templates/_helpers.tpl

deployment/helm/resource-management-policies/balloons/templates/clusterrole.yaml

deployment/helm/resource-management-policies/topology-aware/values.yaml

fmuyassarov · 2023-06-20T20:48:58Z

@marquiz comments are addressed now.

deployment/helm/resource-management-policies/balloons/templates/clusterrole.yaml

deployment/helm/resource-management-policies/balloons/templates/clusterrolebinding.yaml

deployment/helm/resource-management-policies/balloons/templates/configmap.yaml

deployment/helm/resource-management-policies/topology-aware/values.yaml

deployment/helm/resource-management-policies/balloons/values.yaml

marquiz · 2023-06-21T06:41:03Z

deployment/helm/resource-management-policies/balloons/values.yaml

+
+metrics:
+  port: 8891
+  protocol: TCP


Is this something that is relevant for the user to change? Drop?

maybe not protocol and container port, but at least host port is relevant in case 8891 port on the host is utilized by something else already and the user wants to expose the metrics on the host

I removed those fields and kept only the hostport, because I think that might be desirable by the user to be configured

Why would we want hostport? Then that port is reserved and e.g. another plugin cannot be deployed. Containerport should be enough, right?

Or, actually 😏 The HostPort setting could be THE way to achieve pod anti-affinity, if the daemonset controller takes that into account. I.e. one resource policy reserves that port and another one using the same port cannot be scheduled on that node. I don't like it necessarily but it could work

Why would we want hostport? Then that port is reserved and e.g. another plugin cannot be deployed. Containerport should be enough, right?

each plugin can be exposed to a different hostPort. Why user might need is for exposing metrics fro example (Prometheus) to the host.

Or, actually smirk The HostPort setting could be THE way to achieve pod anti-affinity, if the daemonset controller takes that into account. I.e. one resource policy reserves that port and another one using the same port cannot be scheduled on that node. I don't like it necessarily but it could work

it might work, but when user has given some custom hostPort, then it will be missed.

each plugin can be exposed to a different hostPort. Why user might need is for exposing metrics fro example (Prometheus) to the host.

But isn't the usual way to have a K8s service to get the prometheus metrics? Not configure a hostport for each an every app you have? But let's leave it like this for now

deployment/helm/resource-management-policies/topology-aware/values.yaml

deployment/helm/resource-management-policies/balloons/values.yaml

fmuyassarov · 2023-06-21T07:36:57Z

@marquiz thanks for another round of review. PTAL once more. Thanks

fmuyassarov · 2023-06-21T08:06:56Z

@marquiz thanks for another round of review. PTAL once more. Thanks

also added docs update accordingly here: #74

jukkar · 2023-06-21T08:37:46Z

FYI, just noticed that upstream documentation generation is broken. I get this error

Warning, treated as error:
/docs/docs/resource-policy/installation.md:10:'myst' reference target not found: #Helm

Also #Manual ref is not found.

/docs/docs/resource-policy/installation.md:11:'myst' reference target not found: #Manual

and TODO

/docs/docs/resource-policy/installation.md:40:'myst' reference target not found: TODO

and "terminal"

/docs/docs/resource-policy/installation.md:48:Pygments lexer name 'terminal' is not known

I suggest using "console" instead which works for me.

fmuyassarov · 2023-06-21T09:38:59Z

FYI, just noticed that upstream documentation generation is broken. I get this error

thanks for the catch. Let's not do it here, because this PR is not about doc. I see that @marquiz has already submitted a patch in #75

deployment/helm/resource-management-policies/balloons/templates/configmap.yaml

This commit adds a helm chart to install balloons and topology-aware NRI plugins. Signed-off-by: Feruzjon Muyassarov <[email protected]>

marquiz

Thanks @fmuyassarov for working on this 👍 I didn't spot anything obvious anymore so LGTM from my side.

Has anyone actually verified (i.e. tried in practice that they work) these?

fmuyassarov · 2023-06-21T13:50:58Z

Thanks @fmuyassarov for working on this +1 I didn't spot anything obvious anymore so LGTM from my side.

Has anyone actually verified (i.e. tried in practice that they work) these?

I have not installed it but I tested generation of Manifests and since they are the same as we have in kustomize, or in e2e, I assume this should work too as long as other prerequisites are met.

jukkar · 2023-06-21T13:51:27Z

I could try this before we merge this, just a sec...

jukkar · 2023-06-21T14:19:11Z

Couple of notes:

Could we have pointer to the site where helm can be downloaded/installed. This would help the user to install helm if it is missing from the system
The daemonset name is now nri-resource-policy-topology-aware so the pod name will be like this nri-resource-policy-topology-aware-ws78f. The daemonset name was nri-resource-policyearlier which created pod name like nri-resource-policy-ws78f. This is perhaps not a big issue as we do not use helm to install the plugins in our e2e tests. Those tests expect pod name to be without the plugin type and will fail with this naming schema. As I said, perhaps this is not an issue, but just mentioning it.

jukkar · 2023-06-21T14:20:01Z

Please rebase to main and tweak installation.md to add the helm location (in a separate commit).

fmuyassarov · 2023-06-21T14:26:05Z

Couple of notes:

* Could we have pointer to the site where helm can be downloaded/installed. This would help the user to install helm if it is missing from the system

I have already added the documentation. We don't have a place to where we can upload the charts because we don't even have public images yet. Once we have images available and the initial release, then we can find a place for it in somewhere remotely from where it can be directly installed.

* The daemonset name is now `nri-resource-policy-topology-aware` so the pod name will be like this `nri-resource-policy-topology-aware-ws78f`. The daemonset name was `nri-resource-policy`earlier which created pod name like `nri-resource-policy-ws78f`. This is perhaps not a big issue as we do not use helm to install the plugins in our e2e tests. Those tests expect pod name to be without the plugin type and will fail with this naming schema. As I said, perhaps this is not an issue, but just mentioning it.

Yes, e2e changes will be on a follow up PR.

fmuyassarov · 2023-06-21T14:26:46Z

Please rebase to main and tweak installation.md to add the helm location (in a separate commit).

Please see my comment above.

fmuyassarov requested review from klihub and jukkar May 23, 2023 07:41

jukkar mentioned this pull request Jun 6, 2023

Resource policy documentation #6

Merged

fmuyassarov force-pushed the add-helm-support branch from 22deea8 to a5c0400 Compare June 12, 2023 08:04

fmuyassarov commented Jun 12, 2023

View reviewed changes

deployment/helm/nri-plugins/topology_aware_values.yaml Outdated Show resolved Hide resolved

fmuyassarov marked this pull request as ready for review June 12, 2023 08:17

marquiz reviewed Jun 12, 2023

View reviewed changes

fmuyassarov force-pushed the add-helm-support branch from a5c0400 to 7a9e784 Compare June 12, 2023 13:19

marquiz reviewed Jun 13, 2023

View reviewed changes

deployment/helm/resource-policy/templates/daemonset.yaml Outdated Show resolved Hide resolved

marquiz reviewed Jun 13, 2023

View reviewed changes

deployment/helm/resource-policy/topology_aware_values.yaml Outdated Show resolved Hide resolved

deployment/helm/resource-policy/topology_aware_values.yaml Outdated Show resolved Hide resolved

deployment/helm/resource-policy/topology_aware_values.yaml Outdated Show resolved Hide resolved

fmuyassarov force-pushed the add-helm-support branch 2 times, most recently from 62fc42d to 4ec26e0 Compare June 19, 2023 22:45

klihub approved these changes Jun 20, 2023

View reviewed changes

klihub requested a review from marquiz June 20, 2023 14:03

jukkar approved these changes Jun 20, 2023

View reviewed changes

marquiz reviewed Jun 20, 2023

View reviewed changes

fmuyassarov force-pushed the add-helm-support branch from 4ec26e0 to 7b01334 Compare June 20, 2023 20:47

marquiz reviewed Jun 21, 2023

View reviewed changes

fmuyassarov force-pushed the add-helm-support branch 2 times, most recently from 0c11c94 to 5016b94 Compare June 21, 2023 07:32

klihub requested a review from marquiz June 21, 2023 11:00

marquiz reviewed Jun 21, 2023

View reviewed changes

deployment/helm/resource-management-policies/balloons/templates/configmap.yaml Show resolved Hide resolved

helm: add helm chart for plugins installations

c272c24

This commit adds a helm chart to install balloons and topology-aware NRI plugins. Signed-off-by: Feruzjon Muyassarov <[email protected]>

fmuyassarov force-pushed the add-helm-support branch from 5016b94 to c272c24 Compare June 21, 2023 13:11

fmuyassarov requested a review from marquiz June 21, 2023 13:11

marquiz approved these changes Jun 21, 2023

View reviewed changes

jukkar merged commit 578fb25 into containers:main Jun 22, 2023

fmuyassarov deleted the add-helm-support branch June 22, 2023 08:20

helm: add helm chart for plugins installations #45

helm: add helm chart for plugins installations #45

Conversation

fmuyassarov commented May 17, 2023 • edited Loading

fmuyassarov commented May 17, 2023

fmuyassarov commented May 17, 2023

klihub commented May 23, 2023

klihub commented May 23, 2023

fmuyassarov commented May 24, 2023 • edited Loading

jukkar commented Jun 6, 2023

fmuyassarov commented Jun 6, 2023

fmuyassarov commented Jun 12, 2023

marquiz left a comment

Choose a reason for hiding this comment

fmuyassarov commented Jun 12, 2023

marquiz commented Jun 12, 2023

fmuyassarov commented Jun 12, 2023

fmuyassarov commented Jun 13, 2023

marquiz commented Jun 14, 2023

fmuyassarov commented Jun 14, 2023

marquiz commented Jun 14, 2023

fmuyassarov commented Jun 19, 2023 • edited Loading

fmuyassarov commented Jun 19, 2023

klihub left a comment

Choose a reason for hiding this comment

fmuyassarov commented Jun 20, 2023

klihub commented Jun 20, 2023

jukkar left a comment

Choose a reason for hiding this comment

marquiz left a comment

Choose a reason for hiding this comment

fmuyassarov commented Jun 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmuyassarov commented Jun 21, 2023

fmuyassarov commented Jun 21, 2023

jukkar commented Jun 21, 2023 • edited Loading

fmuyassarov commented Jun 21, 2023 • edited Loading

marquiz left a comment

Choose a reason for hiding this comment

fmuyassarov commented Jun 21, 2023

jukkar commented Jun 21, 2023

jukkar commented Jun 21, 2023

jukkar commented Jun 21, 2023

fmuyassarov commented Jun 21, 2023

fmuyassarov commented Jun 21, 2023

fmuyassarov commented May 17, 2023 •

edited

Loading

fmuyassarov commented May 24, 2023 •

edited

Loading

fmuyassarov commented Jun 19, 2023 •

edited

Loading

jukkar commented Jun 21, 2023 •

edited

Loading

fmuyassarov commented Jun 21, 2023 •

edited

Loading