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 reources management for nodes #508

Merged
merged 3 commits into from
Dec 23, 2022
Merged

Add reources management for nodes #508

merged 3 commits into from
Dec 23, 2022

Conversation

puppetninja
Copy link
Contributor

@puppetninja puppetninja commented Nov 12, 2022

This is a duplicated one of #502, PR generated from official repo branch to run all the CI jobs

This PR would set the resource manage for octez nodes and sidecar in the same pod. For bakers, there are no resources management for it however

Users can manage the node resources for octez node placement. Additionally, users can use k8s autoscalers to scale out node pods based on certain criteria
https://kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale
This scaling mechanism is for pod, so both octez node and sidecar resources should be specified for the pod resource usage to be calculated. For now, pods with bakers running inside is not supported.

Copy link
Contributor

@nicolasochem nicolasochem left a comment

Choose a reason for hiding this comment

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

This still requires "documentation" in the chart's default values.yaml

What we typically do is provide a commented-out example in the values file, with some additional explanatory commentary.

charts/tezos/templates/_containers.tpl Outdated Show resolved Hide resolved
charts/tezos/values.yaml Outdated Show resolved Hide resolved
@puppetninja puppetninja force-pushed the resources branch 3 times, most recently from 4da46d5 to 8f984eb Compare December 13, 2022 15:19
"image" "octez"
-}}

{{- if hasKey $.node_vals "resources" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like you can simply pass "resources" $.node.vals.resources and if the key is absent it would pass null, and still work? So you can avoid setting $bakerParams at all and just modify the dict passed to include "tezos.generic_container".

Copy link
Contributor

@nicolasochem nicolasochem left a comment

Choose a reason for hiding this comment

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

Why assign requests to the baker as well as the node? Then if you set requests of 3300Mi on a baking node, the pod actually requires 6600Mi. and more if you have more bakers. Can we set a request for the node only, so the value configured ends up being the total pod request?

@puppetninja puppetninja force-pushed the resources branch 5 times, most recently from d291af7 to c56dfb6 Compare December 22, 2022 21:16
@nicolasochem
Copy link
Contributor

@harryttd @orcutt989 all issues resolved, please review again

@orcutt989
Copy link
Contributor

@nicolasochem @harryttd There are still requested changes on this PR. Please dismiss them if they have been addressed.

@harryttd
Copy link
Contributor

I'm reviewing right now

Copy link
Contributor

@harryttd harryttd left a comment

Choose a reason for hiding this comment

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

Almost ready. I think i understand the PR more fully now. I didn't get that the scaler wouldn't scale if it was targeting specific containers as opposed to the entire pod resources. So if it couldn't find any resources defined on any container it wouldn't scale.

charts/tezos/values.yaml Outdated Show resolved Hide resolved
@puppetninja puppetninja requested a review from harryttd December 23, 2022 09:15
@puppetninja puppetninja merged commit 864edbe into master Dec 23, 2022
@harryttd harryttd deleted the resources branch December 23, 2022 18:56
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.

4 participants