Configure number of hugepages for nodes: nr_hugepages #74
Conversation
|
Warning Rate limit exceeded@klinch0 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 17 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
WalkthroughA new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @klinch0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces the capability to configure huge pages within the talos.config for Cozystack deployments. This enhancement allows users to specify the number of huge pages to allocate, which can significantly improve the performance of memory-intensive workloads by optimizing memory management at the kernel level.
Highlights
- Huge Page Configuration: I've added a new configuration option to allow specifying the number of huge pages (
nr_hugepages) for machines. - Sysctl Integration: The
talos.configtemplate now conditionally sets thevm.nr_hugepagessysctl based on the provided value. - Default Value: A default value of
0fornr_hugepageshas been added tovalues.yaml, ensuring backward compatibility and optional usage.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds support for configuring huge pages (vm.nr_hugepages) via a new Helm value nr_hugepages. The implementation in the Talos configuration template has a critical issue where it introduces a duplicate sysctls key, which will cause the huge pages configuration to be ignored. I've provided a comment with details on how to fix this.
| {{- if gt .Values.nr_hugepages 0 }} | ||
| sysctls: | ||
| vm.nr_hugepages: {{ .Values.nr_hugepages | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
This introduces a new sysctls block under machine. However, there is another sysctls block defined on line 20 of this file. This will result in a YAML with a duplicate key, where the second definition will overwrite this one, and the vm.nr_hugepages setting will be lost.
Please remove this block and add the logic to the existing sysctls block around line 20. It should look something like this:
sysctls:
{{- if gt .Values.nr_hugepages 0 }}
vm.nr_hugepages: {{ .Values.nr_hugepages | quote }}
{{- end }}
net.ipv4.neigh.default.gc_thresh1: "4096"
net.ipv4.neigh.default.gc_thresh2: "8192"
net.ipv4.neigh.default.gc_thresh3: "16384"![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/cozystack/values.yaml (1)
13-13: Consider naming convention consistency and add documentation.The parameter name uses snake_case while other similar parameters in this file use camelCase (e.g.,
clusterDomain,floatingIP). Consider usingnrHugepagesfor consistency, or document if there's a specific reason for the snake_case naming.Also consider adding a comment to document the parameter's purpose:
+# Number of hugepages to allocate (0 disables hugepages) nr_hugepages: 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/cozystack/templates/_helpers.tpl(1 hunks)charts/cozystack/values.yaml(1 hunks)
| {{- if gt .Values.nr_hugepages 0 }} | ||
| sysctls: | ||
| vm.nr_hugepages: {{ .Values.nr_hugepages | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
Critical: Duplicate sysctls sections will cause configuration conflicts.
The template creates two separate sysctls sections under machine - one conditionally for hugepages (lines 4-5) and another unconditionally for network settings (lines 20-23). This creates duplicate YAML keys which can cause parsing errors or unexpected behavior where one section overrides the other.
The hugepages sysctl should be merged into the existing sysctls section. Apply this fix:
- {{- if gt .Values.nr_hugepages 0 }}
- sysctls:
- vm.nr_hugepages: {{ .Values.nr_hugepages | quote }}
- {{- end }}
{{- if eq .MachineType "controlplane" }}
nodeLabels:
node.kubernetes.io/exclude-from-external-load-balancers:
$patch: delete
{{- end }}
type: {{ .MachineType }}
kubelet:
nodeIP:
validSubnets:
{{- toYaml .Values.advertisedSubnets | nindent 8 }}
extraConfig:
cpuManagerPolicy: static
maxPods: 512
sysctls:
+ {{- if gt .Values.nr_hugepages 0 }}
+ vm.nr_hugepages: {{ .Values.nr_hugepages | quote }}
+ {{- end }}
net.ipv4.neigh.default.gc_thresh1: "4096"
net.ipv4.neigh.default.gc_thresh2: "8192"
net.ipv4.neigh.default.gc_thresh3: "16384"🤖 Prompt for AI Agents
In charts/cozystack/templates/_helpers.tpl around lines 3 to 6, the template
creates a separate sysctls section for hugepages, which duplicates the sysctls
section defined later for network settings, causing YAML key conflicts. To fix
this, remove the separate sysctls block for hugepages and instead merge the
vm.nr_hugepages setting into the existing sysctls section so all sysctl settings
are under a single sysctls key.
klinch0
force-pushed
the
feature/add-hp-configure
branch
2 times, most recently
from
5b464b0 to
0712ad5
Compare
klinch0
changed the title
Signed-off-by: kklinch0 <kklinch0@gmail.com>
klinch0
force-pushed
the
feature/add-hp-configure
branch
from
0712ad5 to
eba3b4a
Compare
Summary by CodeRabbit