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

helm: add charts for memtierd and memory-qos #118

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

fmuyassarov
Copy link
Collaborator

This commit adds two new Helm charts for installing memory-qos(#115) and memtierd(#117) NRI plugins via Helm.

@fmuyassarov fmuyassarov changed the title helm: add helm charts for memtierd and memory-qos helm: add charts for memtierd and memory-qos Sep 28, 2023
docs/resource-policy/installation.md Outdated Show resolved Hide resolved
docs/resource-policy/installation.md Outdated Show resolved Hide resolved
docs/resource-policy/installation.md Outdated Show resolved Hide resolved
@fmuyassarov fmuyassarov force-pushed the add-charts branch 2 times, most recently from 3855ddf to 48020e4 Compare October 3, 2023 06:36
@fmuyassarov
Copy link
Collaborator Author

@kad @askervin PR is now updated. But I have not added #121 changes, which I think we need. But since it is under active reviews, perhaps we can do it once the patch has landed.

Both plugins are now outside the resource management dir, and the corresponding images are called
ghcr.io/containers/nri-plugins/nri-memtierd & ghcr.io/containers/nri-plugins/nri-memory-qos.

Same with the labels and DaemonSet object naming, nri-memtierd & nri-memory-qos.

@askervin
Copy link
Collaborator

askervin commented Oct 3, 2023

Cool, thanks @fmuyassarov! This works fine already. The last wishes from me:

  • Would you add the track-working-set-size class to deployments/helm/memtierd/templates/configmap.yaml
  • Is it possible to make this volume mount a configurable option in helm installation?

After adding the track-working-set-size class and helm install it should be possible to deploy test/e2e/files/nri-memtierd-test-pod.yaml without errors.

Deploying test/e2e/files/nri-memory-qos-test-pod.yaml already works fine after helm installing nri-memory-qos. Great!

@fmuyassarov fmuyassarov force-pushed the add-charts branch 2 times, most recently from 6819d3d to d7bce76 Compare October 3, 2023 09:15
@fmuyassarov
Copy link
Collaborator Author

Cool, thanks @fmuyassarov! This works fine already. The last wishes from me:

* Would you add the [track-working-set-size](https://github.com/askervin/nri-plugins/blob/5RA_memtierd_host_root/deployment/overlays/memtierd/sample-configmap.yaml#L31) class to deployments/helm/memtierd/templates/configmap.yaml

* Is it possible to make [this](https://github.com/askervin/nri-plugins/blob/5RA_memtierd_host_root/deployment/overlays/memtierd/daemonset.yaml#L46) volume mount a configurable option in helm installation?

After adding the track-working-set-size class and helm install it should be possible to deploy test/e2e/files/nri-memtierd-test-pod.yaml without errors.

Deploying test/e2e/files/nri-memory-qos-test-pod.yaml already works fine after helm installing nri-memory-qos. Great!

@askervin done. PTAL.

@askervin
Copy link
Collaborator

askervin commented Oct 3, 2023

@fmuyassarov, sorry, I overlooked a couple of things... there are also other cases where deployments/helm/memtierd/templates/daemonset.yaml has diverged from deployment/overlays/memtierd/daemonset.yaml.

We do not want to pass the --host-root parameter, nor do we want to mount the whole host root filesystem. Also the --run-dir /run-dir is an essential argument in order to get memtierd.output files into the directory that user gives with outputDir to helm.

Setting outputDir seems to work here nicely.

deployment/helm/memory-qos/Chart.yaml Outdated Show resolved Hide resolved
deployment/helm/memory-qos/templates/_helpers.tpl Outdated Show resolved Hide resolved
deployment/helm/memtierd/Chart.yaml Outdated Show resolved Hide resolved
deployment/helm/memtierd/templates/daemonset.yaml Outdated Show resolved Hide resolved
@fmuyassarov
Copy link
Collaborator Author

@askervin I think we also need - --cgroups-dir - /sys/fs/cgroup ?

@askervin
Copy link
Collaborator

askervin commented Oct 3, 2023

@askervin I think we also need - --cgroups-dir - /sys/fs/cgroup ?

Yes we do.

It's good to check that helm template --set outputDir=/var/run/nri-memtierd nri-memtierd deployment/helm/memtierd output includes the daemonset that will be created wtih kustomize, too, but it actually has the outputDir mounted that is just commented in the kustomize daemonset.

This commit adds two new Helm charts for installing memory-qos
and memtierd NRI plugins via Helm.

Signed-off-by: Feruzjon Muyassarov <[email protected]>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a mental note that the documentation is a total mess. Installation instructions are under "Resource policy plugins" but the actual plugin documentation under "Memory plugins" 🤪 I don't think should fix the mess in this PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ow right. I will submit a patch for that and also for selectorLabel for TA and balloons plugins. Thanks for noticing.

Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fmuyassarov for working on this. I don't have anything to add for the initial version. We should carry over some of the things (like labels vs selectorLabels) to the other charts as well.
LGTM

@fmuyassarov
Copy link
Collaborator Author

Thanks @fmuyassarov for working on this. I don't have anything to add for the initial version. We should carry over some of the things (like labels vs selectorLabels) to the other charts as well. LGTM

Submitted in #124. Thanks for the reviews @marquiz

@klihub klihub merged commit 325cfbf into containers:main Oct 5, 2023
3 checks passed
@fmuyassarov fmuyassarov deleted the add-charts branch October 5, 2023 10:04
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

Successfully merging this pull request may close these issues.

5 participants