OCPNODE-4487: replace --system-reserved flags with config drop-in#6044
OCPNODE-4487: replace --system-reserved flags with config drop-in#6044nispriha wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@nispriha: This pull request references OCPNODE-4487 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nispriha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughMove kubelet system-reserved config from ExecStart flags and /etc/node-sizing.env into an auto-generated drop-in at /etc/openshift/kubelet.conf.d/20-auto-sizing.conf. Unit templates create/use that dir and drop the --system-reserved flag. Tests updated accordingly. ChangesKubelet Auto-Sizing Configuration Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
/retest |
1 similar comment
|
/retest |
|
is the control plane unchanged just because we're anxious about breaking something? I feel it'd be better to set consistently TBH |
nispriha
force-pushed
the
njagan/auto-sizing-dropin
branch
from
8c2d700 to
de0039f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e-2of2/kubeletcfg_test.go (1)
361-377: ⚖️ Poor tradeoffTest implementation doesn't match PR description.
The PR verification steps state: "e2e test TestKubeletAutoSizingDropIn verifies worker kubelet.service does not contain --system-reserved or EnvironmentFile=/etc/node-sizing.env."
However, the test only verifies that the drop-in file exists and contains the expected YAML structure. It does not verify that
--system-reservedflags orEnvironmentFile=/etc/node-sizing.envhave been removed from the kubelet.service unit file.Consider either updating the PR description to match the actual test behavior, or enhancing the test to verify the removal of the old mechanism.
Optional: Enhanced test to verify flag removal
// After checking drop-in exists, verify old mechanism removed: unitContent := helpers.ExecCmdOnNode(t, cs, node, "systemctl", "cat", "kubelet.service") require.NotContains(t, unitContent, "--system-reserved", "%s kubelet.service should not contain --system-reserved flag", role) require.NotContains(t, unitContent, "EnvironmentFile=/etc/node-sizing.env", "%s kubelet.service should not source /etc/node-sizing.env", role)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-2of2/kubeletcfg_test.go` around lines 361 - 377, The test TestKubeletAutoSizingDropIn only checks the drop-in file content but the PR description promises verifying removal of the old mechanism; update the test to also fetch the kubelet unit via helpers.ExecCmdOnNode (e.g., run "systemctl cat kubelet.service") and add assertions using require.NotContains to ensure the unit does not include "--system-reserved" and does not reference "EnvironmentFile=/etc/node-sizing.env", or alternatively change the PR description to reflect that the test only validates the drop-in YAML.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@templates/master/01-master-kubelet/_base/units/kubelet.service.yaml`:
- Line 13: The PR summary is misleading because the master kubelet unit (because
of the ExecStartPre creating /etc/openshift/kubelet.conf.d and the kubelet
invocation with --config-dir=/etc/openshift/kubelet.conf.d) will now load
drop-in files such as the kubelet-auto-sizing drop-in, so update the PR summary
to explicitly state that masters (and arbiters if applicable) will pick up
systemReserved via the drop-in mechanism; mention the specific symbols
TestKubeletAutoSizingDropIn and
templates/common/_base/files/kubelet-auto-sizing.yaml and clarify exactly what
"remains unchanged" (e.g., default kubelet behavior and existing static config
files) versus what is new (masters now consume drop-ins from
/etc/openshift/kubelet.conf.d created by ExecStartPre and the --config-dir
flag).
In `@test/e2e-2of2/kubeletcfg_test.go`:
- Around line 367-376: The test currently iterates over roles []string{"worker",
"master"} and validates the kubelet drop-in via helpers.GetRandomNode,
helpers.ExecCmdOnNode and require.Contains; update that role slice to include
"arbiter" (i.e. []string{"worker", "master", "arbiter"}) so the t.Run loop
covers the arbiter nodes and exercises the same assertions against the drop-in
content (the require.Contains checks for "kind: KubeletConfiguration" and
"systemReserved") using the existing helpers and dropInPath variable.
---
Nitpick comments:
In `@test/e2e-2of2/kubeletcfg_test.go`:
- Around line 361-377: The test TestKubeletAutoSizingDropIn only checks the
drop-in file content but the PR description promises verifying removal of the
old mechanism; update the test to also fetch the kubelet unit via
helpers.ExecCmdOnNode (e.g., run "systemctl cat kubelet.service") and add
assertions using require.NotContains to ensure the unit does not include
"--system-reserved" and does not reference
"EnvironmentFile=/etc/node-sizing.env", or alternatively change the PR
description to reflect that the test only validates the drop-in YAML.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d9606d45-a14a-49b8-95af-7d23899f0929
📒 Files selected for processing (9)
pkg/controller/template/kubelet_config_dir_test.gotemplates/arbiter/01-arbiter-kubelet/_base/units/kubelet.service.yamltemplates/arbiter/01-arbiter-kubelet/on-prem/units/kubelet.service.yamltemplates/common/_base/files/kubelet-auto-sizing.yamltemplates/master/01-master-kubelet/_base/units/kubelet.service.yamltemplates/master/01-master-kubelet/on-prem/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/_base/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yamltest/e2e-2of2/kubeletcfg_test.go
💤 Files with no reviewable changes (2)
- templates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yaml
- templates/worker/01-worker-kubelet/_base/units/kubelet.service.yaml
| for _, role := range []string{"worker", "master"} { | ||
| t.Run(role, func(t *testing.T) { | ||
| node := helpers.GetRandomNode(t, cs, role) | ||
| dropIn := helpers.ExecCmdOnNode(t, cs, node, "cat", filepath.Join("/rootfs", dropInPath)) | ||
| require.Contains(t, dropIn, "kind: KubeletConfiguration", | ||
| "%s drop-in should be a KubeletConfiguration", role) | ||
| require.Contains(t, dropIn, "systemReserved:", | ||
| "%s drop-in should contain systemReserved", role) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Test coverage gap: arbiter role not tested.
The test checks worker and master roles but omits arbiter, even though arbiter kubelet.service templates were modified in this PR (added --config-dir in templates/arbiter/01-arbiter-kubelet/*/units/kubelet.service.yaml).
📝 Suggested fix to include arbiter role
- for _, role := range []string{"worker", "master"} {
+ for _, role := range []string{"worker", "master", "arbiter"} {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for _, role := range []string{"worker", "master"} { | |
| t.Run(role, func(t *testing.T) { | |
| node := helpers.GetRandomNode(t, cs, role) | |
| dropIn := helpers.ExecCmdOnNode(t, cs, node, "cat", filepath.Join("/rootfs", dropInPath)) | |
| require.Contains(t, dropIn, "kind: KubeletConfiguration", | |
| "%s drop-in should be a KubeletConfiguration", role) | |
| require.Contains(t, dropIn, "systemReserved:", | |
| "%s drop-in should contain systemReserved", role) | |
| }) | |
| } | |
| for _, role := range []string{"worker", "master", "arbiter"} { | |
| t.Run(role, func(t *testing.T) { | |
| node := helpers.GetRandomNode(t, cs, role) | |
| dropIn := helpers.ExecCmdOnNode(t, cs, node, "cat", filepath.Join("/rootfs", dropInPath)) | |
| require.Contains(t, dropIn, "kind: KubeletConfiguration", | |
| "%s drop-in should be a KubeletConfiguration", role) | |
| require.Contains(t, dropIn, "systemReserved:", | |
| "%s drop-in should contain systemReserved", role) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e-2of2/kubeletcfg_test.go` around lines 367 - 376, The test currently
iterates over roles []string{"worker", "master"} and validates the kubelet
drop-in via helpers.GetRandomNode, helpers.ExecCmdOnNode and require.Contains;
update that role slice to include "arbiter" (i.e. []string{"worker", "master",
"arbiter"}) so the t.Run loop covers the arbiter nodes and exercises the same
assertions against the drop-in content (the require.Contains checks for "kind:
KubeletConfiguration" and "systemReserved") using the existing helpers and
dropInPath variable.
|
/payload-aggregate periodic-ci-openshift-release-master-nightly-5.0-e2e-aws-ovn-upgrade-fips 2 |
|
@ngopalak-redhat: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/test e2e-aws-mco-disruptive |
|
/payload-aggregate periodic-ci-openshift-release-master-nightly-4.22-e2e-aws-ovn-upgrade-fips 1 |
|
@ngopalak-redhat: trigger 0 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command |
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning |
|
@ngopalak-redhat: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3f2682a0-5502-11f1-948e-a3de092345bc-0 |
nispriha
changed the title
|
/retest e2e-aws-mco-disruptive |
|
/test e2e-aws-mco-disruptive |
|
/test e2e-openstack |
| controllerConfig: "./test_data/controller_config_aws.yaml", | ||
| expectedWorkerFlag: true, | ||
| expectedMasterFlag: false, | ||
| expectedMasterFlag: true, |
There was a problem hiding this comment.
No, since all node roles now have --config-dir, it's always true. I have removed the boolean flags and simplified the test to just assert --config-dir is present for all roles.
| recommended_systemreserved_memory=$(echo $recommended_systemreserved_memory | awk '{printf("%d\n",$1 + 0.5)}') # Round off so we avoid float conversions | ||
| echo "SYSTEM_RESERVED_MEMORY=${recommended_systemreserved_memory}Gi">> ${NODE_SIZES_ENV} | ||
| SYSTEM_RESERVED_MEMORY="${recommended_systemreserved_memory}Gi" | ||
| echo "SYSTEM_RESERVED_MEMORY=${SYSTEM_RESERVED_MEMORY}">> ${NODE_SIZES_ENV} |
There was a problem hiding this comment.
what cases do we still need NODE_SIZES_ENV?
There was a problem hiding this comment.
We don't, since all node roles use the drop-in now, nothing reads /etc/node-sizing.env anymore. I have removed all NODE_SIZES_ENV references from the script.
nispriha
force-pushed
the
njagan/auto-sizing-dropin
branch
from
de0039f to
f3de103
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e-2of2/kubeletcfg_test.go (1)
371-374: ⚡ Quick winStrengthen drop-in assertions to validate actual
systemReservedfields.The current checks can pass even if the block exists but lacks required keys. Consider asserting
cpu:,memory:, andephemeral-storage:too.💡 Suggested enhancement
require.Contains(t, dropIn, "kind: KubeletConfiguration", "%s drop-in should be a KubeletConfiguration", role) require.Contains(t, dropIn, "systemReserved:", "%s drop-in should contain systemReserved", role) + require.Contains(t, dropIn, "cpu:", + "%s drop-in should include systemReserved.cpu", role) + require.Contains(t, dropIn, "memory:", + "%s drop-in should include systemReserved.memory", role) + require.Contains(t, dropIn, "ephemeral-storage:", + "%s drop-in should include systemReserved.ephemeral-storage", role) }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-2of2/kubeletcfg_test.go` around lines 371 - 374, The current assertions only check for the presence of the "systemReserved:" block but not that it contains required fields; modify the test in kubeletcfg_test.go (the assertions using require.Contains on dropIn and role) to also assert that dropIn includes the specific keys "cpu:", "memory:", and "ephemeral-storage:" (e.g., require.Contains(t, dropIn, "cpu:", "%s drop-in should contain systemReserved cpu", role) and similarly for memory and ephemeral-storage) so the drop-in not only has the block but the expected reserved resources.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controller/template/kubelet_config_dir_test.go`:
- Around line 59-65: The test iterates ign.Systemd.Units looking for unit.Name
== "kubelet.service" but never asserts that such a unit was found, allowing
false positives; update the loop in kubelet_config_dir_test (the block that
checks ign.Systemd.Units and calls assert.Contains on *unit.Contents) to set a
found boolean when you locate the kubelet.service unit, break, then after the
loop add an explicit assertion (e.g., assert.True or assert.NotNil with a clear
message referencing the platform/role) that the kubelet.service unit was present
before checking its contents.
---
Nitpick comments:
In `@test/e2e-2of2/kubeletcfg_test.go`:
- Around line 371-374: The current assertions only check for the presence of the
"systemReserved:" block but not that it contains required fields; modify the
test in kubeletcfg_test.go (the assertions using require.Contains on dropIn and
role) to also assert that dropIn includes the specific keys "cpu:", "memory:",
and "ephemeral-storage:" (e.g., require.Contains(t, dropIn, "cpu:", "%s drop-in
should contain systemReserved cpu", role) and similarly for memory and
ephemeral-storage) so the drop-in not only has the block but the expected
reserved resources.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8586419-2d99-49af-a488-66145bf0ae17
📒 Files selected for processing (9)
pkg/controller/template/kubelet_config_dir_test.gotemplates/arbiter/01-arbiter-kubelet/_base/units/kubelet.service.yamltemplates/arbiter/01-arbiter-kubelet/on-prem/units/kubelet.service.yamltemplates/common/_base/files/kubelet-auto-sizing.yamltemplates/master/01-master-kubelet/_base/units/kubelet.service.yamltemplates/master/01-master-kubelet/on-prem/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/_base/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yamltest/e2e-2of2/kubeletcfg_test.go
💤 Files with no reviewable changes (2)
- templates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yaml
- templates/worker/01-worker-kubelet/_base/units/kubelet.service.yaml
|
/payload-job periodic-ci-openshift-release-main-nightly-5.0-e2e-aws-disruptive-longrunning |
|
@nispriha: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/c6368b00-584f-11f1-9876-1c7cc9194f05-0 |
nispriha
force-pushed
the
njagan/auto-sizing-dropin
branch
from
f3de103 to
3292bdc
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e-2of2/kubeletcfg_test.go (1)
367-376:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTest coverage gap: arbiter role not tested.
The test validates
workerandmasterroles but omitsarbiter, even though this PR modified arbiter kubelet.service templates to add--config-dir(per the review stack context).Before adding
"arbiter"to the role slice, verify thathelpers.GetRandomNodehandles clusters without arbiter nodes gracefully:#!/bin/bash # Description: Check if GetRandomNode helper has skip/error handling for missing roles rg -nP -A10 -B2 'func GetRandomNode' --type=goIf the helper skips gracefully when no nodes match the role, add
"arbiter"to the slice. Otherwise, wrap the arbiter test in a conditional check that skips when no arbiter nodes exist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-2of2/kubeletcfg_test.go` around lines 367 - 376, Add coverage for the arbiter role: update the role iteration in the test to include "arbiter" (the slice passed to the loop) if helpers.GetRandomNode supports missing-role skipping; otherwise, before calling helpers.GetRandomNode, query whether any arbiter nodes exist and call t.Skip when none are present. Locate the loop that iterates over []string{"worker","master"} and adjust it (or add a conditional t.Skip) around the call to helpers.GetRandomNode and subsequent helpers.ExecCmdOnNode and assertions that inspect the drop-in content ("dropIn", dropInPath) so the test safely handles clusters without arbiter nodes. Ensure you reference helpers.GetRandomNode, helpers.ExecCmdOnNode, and the existing assertions when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@test/e2e-2of2/kubeletcfg_test.go`:
- Around line 367-376: Add coverage for the arbiter role: update the role
iteration in the test to include "arbiter" (the slice passed to the loop) if
helpers.GetRandomNode supports missing-role skipping; otherwise, before calling
helpers.GetRandomNode, query whether any arbiter nodes exist and call t.Skip
when none are present. Locate the loop that iterates over
[]string{"worker","master"} and adjust it (or add a conditional t.Skip) around
the call to helpers.GetRandomNode and subsequent helpers.ExecCmdOnNode and
assertions that inspect the drop-in content ("dropIn", dropInPath) so the test
safely handles clusters without arbiter nodes. Ensure you reference
helpers.GetRandomNode, helpers.ExecCmdOnNode, and the existing assertions when
making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 434dfa8e-c4c4-46a0-a1ec-f100fb21b290
📒 Files selected for processing (9)
pkg/controller/template/kubelet_config_dir_test.gotemplates/arbiter/01-arbiter-kubelet/_base/units/kubelet.service.yamltemplates/arbiter/01-arbiter-kubelet/on-prem/units/kubelet.service.yamltemplates/common/_base/files/kubelet-auto-sizing.yamltemplates/master/01-master-kubelet/_base/units/kubelet.service.yamltemplates/master/01-master-kubelet/on-prem/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/_base/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yamltest/e2e-2of2/kubeletcfg_test.go
💤 Files with no reviewable changes (2)
- templates/worker/01-worker-kubelet/_base/units/kubelet.service.yaml
- templates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yaml
Remove EnvironmentFile=/etc/node-sizing.env and the --system-reserved command-line flag from kubelet.service. The auto-sizing script now writes a KubeletConfiguration drop-in file to /etc/openshift/kubelet.conf.d/20-auto-sizing.conf, which kubelet reads via --config-dir. Add --config-dir to master and arbiter kubelet.service for consistency with workers.
nispriha
force-pushed
the
njagan/auto-sizing-dropin
branch
from
3292bdc to
531c6d4
Compare
|
/test e2e-aws-mco-disruptive |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkg/controller/template/kubelet_config_dir_test.go (1)
59-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire a kubelet.service match so the test can’t silently pass.
On Line 59, the assertion only runs conditionally; if no
kubelet.serviceis found, this test still passes. Please track and assert that at least one kubelet unit was validated (ideally per role) for each platform case.Suggested fix
for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ - for _, cfg := range cfgs { + roleHasKubelet := map[string]bool{} + for _, cfg := range cfgs { role, ok := cfg.Labels[mcfgv1.MachineConfigRoleLabelKey] require.True(t, ok, "Machine config missing role label") + if _, seen := roleHasKubelet[role]; !seen { + roleHasKubelet[role] = false + } @@ for _, unit := range ign.Systemd.Units { if unit.Name == "kubelet.service" && unit.Contents != nil { + roleHasKubelet[role] = true assert.Contains(t, *unit.Contents, "--config-dir=/etc/openshift/kubelet.conf.d", "%s node should have --config-dir for platform %s", role, tc.name) break } } } + for role, found := range roleHasKubelet { + require.True(t, found, "kubelet.service unit not found for %s on platform %s", role, tc.name) + } }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controller/template/kubelet_config_dir_test.go` around lines 59 - 65, The test iterates ign.Systemd.Units and only asserts when it finds a unit with unit.Name == "kubelet.service", so it can silently pass if no kubelet unit exists; modify the loop in kubelet_config_dir_test.go to set a boolean flag (e.g., foundKubelet) when a matching unit is seen and validated (inside the existing for _, unit := range ign.Systemd.Units { if unit.Name == "kubelet.service" ... }), then after the loop assert that foundKubelet is true (assert.True or require.True) with a clear message mentioning the role and tc.name so the test fails if no kubelet.service was present for that platform/role.test/e2e-2of2/kubeletcfg_test.go (1)
367-375:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude arbiter coverage (with skip when arbiter nodes are absent).
This loop currently validates only
workerandmaster, but this PR also changes arbiter kubelet wiring. Please include arbiter in this test path, guarded so clusters without arbiter nodes are skipped instead of failed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e-2of2/kubeletcfg_test.go` around lines 367 - 375, The test loop should include "arbiter" but skip the subtest if the cluster has no arbiter nodes; update the role list to include "arbiter" and, inside the t.Run for arbiter, check for arbiter node existence (preferably via an existing helper such as helpers.HasRole / helpers.CountNodesWithRole or by calling helpers.GetRandomNode and skipping on a nil/absent result) before running the assertions that use helpers.GetRandomNode, helpers.ExecCmdOnNode and dropInPath; if no arbiter node is present call t.Skipf to quietly skip the arbiter subtest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e-2of2/kubeletcfg_test.go`:
- Around line 371-374: The test currently only asserts the presence of the
"systemReserved:" header; update the assertions that reference dropIn and role
to validate concrete entries under systemReserved (e.g., check for "cpu",
"memory", and "ephemeral-storage" keys/values). Replace or augment the
require.Contains calls on dropIn (in this test function) with additional
require.Contains (or regex matches) that look for the expected
"systemReserved:\n" entries such as "systemReserved:\n cpu:",
"systemReserved:\n memory:", and "systemReserved:\n ephemeral-storage:" so the
test verifies actual values are written rather than an empty section.
---
Duplicate comments:
In `@pkg/controller/template/kubelet_config_dir_test.go`:
- Around line 59-65: The test iterates ign.Systemd.Units and only asserts when
it finds a unit with unit.Name == "kubelet.service", so it can silently pass if
no kubelet unit exists; modify the loop in kubelet_config_dir_test.go to set a
boolean flag (e.g., foundKubelet) when a matching unit is seen and validated
(inside the existing for _, unit := range ign.Systemd.Units { if unit.Name ==
"kubelet.service" ... }), then after the loop assert that foundKubelet is true
(assert.True or require.True) with a clear message mentioning the role and
tc.name so the test fails if no kubelet.service was present for that
platform/role.
In `@test/e2e-2of2/kubeletcfg_test.go`:
- Around line 367-375: The test loop should include "arbiter" but skip the
subtest if the cluster has no arbiter nodes; update the role list to include
"arbiter" and, inside the t.Run for arbiter, check for arbiter node existence
(preferably via an existing helper such as helpers.HasRole /
helpers.CountNodesWithRole or by calling helpers.GetRandomNode and skipping on a
nil/absent result) before running the assertions that use helpers.GetRandomNode,
helpers.ExecCmdOnNode and dropInPath; if no arbiter node is present call t.Skipf
to quietly skip the arbiter subtest.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2d184d49-0a8b-4734-9192-40137339b56b
📒 Files selected for processing (9)
pkg/controller/template/kubelet_config_dir_test.gotemplates/arbiter/01-arbiter-kubelet/_base/units/kubelet.service.yamltemplates/arbiter/01-arbiter-kubelet/on-prem/units/kubelet.service.yamltemplates/common/_base/files/kubelet-auto-sizing.yamltemplates/master/01-master-kubelet/_base/units/kubelet.service.yamltemplates/master/01-master-kubelet/on-prem/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/_base/units/kubelet.service.yamltemplates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yamltest/e2e-2of2/kubeletcfg_test.go
💤 Files with no reviewable changes (2)
- templates/worker/01-worker-kubelet/on-prem/units/kubelet.service.yaml
- templates/worker/01-worker-kubelet/_base/units/kubelet.service.yaml
| require.Contains(t, dropIn, "kind: KubeletConfiguration", | ||
| "%s drop-in should be a KubeletConfiguration", role) | ||
| require.Contains(t, dropIn, "systemReserved:", | ||
| "%s drop-in should contain systemReserved", role) |
There was a problem hiding this comment.
Validate concrete systemReserved entries, not only the section header.
The current assertions pass even if systemReserved is present but empty. Add checks for expected entries (for example cpu, memory, and ephemeral-storage) so the test actually verifies values are written.
Suggested fix
require.Contains(t, dropIn, "kind: KubeletConfiguration",
"%s drop-in should be a KubeletConfiguration", role)
require.Contains(t, dropIn, "systemReserved:",
"%s drop-in should contain systemReserved", role)
+ require.Contains(t, dropIn, "cpu:",
+ "%s drop-in should contain systemReserved cpu", role)
+ require.Contains(t, dropIn, "memory:",
+ "%s drop-in should contain systemReserved memory", role)
+ require.Contains(t, dropIn, "ephemeral-storage:",
+ "%s drop-in should contain systemReserved ephemeral-storage", role)
})
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e-2of2/kubeletcfg_test.go` around lines 371 - 374, The test currently
only asserts the presence of the "systemReserved:" header; update the assertions
that reference dropIn and role to validate concrete entries under systemReserved
(e.g., check for "cpu", "memory", and "ephemeral-storage" keys/values). Replace
or augment the require.Contains calls on dropIn (in this test function) with
additional require.Contains (or regex matches) that look for the expected
"systemReserved:\n" entries such as "systemReserved:\n cpu:",
"systemReserved:\n memory:", and "systemReserved:\n ephemeral-storage:" so the
test verifies actual values are written rather than an empty section.
|
@nispriha: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
- What I did
Remove
EnvironmentFile=/etc/node-sizing.envand the--system-reservedcommand-line flag fromkubelet.service. The auto-sizing script (dynamic-system-reserved-calc.sh) now writes aKubeletConfigurationdrop-in file to/etc/openshift/kubelet.conf.d/20-auto-sizing.conf, which kubelet reads via--config-dirflag. Add--config-dirto master and arbiterkubelet.servicefor consistency with workers.- How to verify it
go test ./pkg/controller/kubelet-config/... -vpassesgo test ./pkg/controller/template/... -vpasses (verifies --config-dir is present on all node roles)TestKubeletAutoSizingDropInverifies the auto-sizing drop-in exists with systemReserved values- Description for the changelog
Kubelet now receives systemReserved values via a KubeletConfiguration drop-in instead of --system-reserved command-line flags.
Summary by CodeRabbit
Refactor
Tests