OCPBUGS-78195: fix(sync-global-pullsecret): compare content ignoring trailing newlines - #7638
Conversation
Remove newline preservation logic that caused E2E mismatch. Add KubeletRestarter interface for testability with mock injection. Generated with Claude Code 2.1.31 (Claude Opus 4.5) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Signed-off-by: Ahmed Abdalla <aabdelre@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
WalkthroughThe changes introduce a KubeletRestarter interface to abstract kubelet restart logic, enabling testability. A concrete realKubeletRestarter implementation replaces direct calls to signalKubeletToRestartProcess(). GlobalPullSecretSyncer is updated to use this abstraction with error handling and retry flows. File-content comparison now ignores trailing newlines to avoid unnecessary updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
the
jira/invalid-bug
|
@devguyio: This pull request references Jira Issue OCPBUGS-67262, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
openshift-ci
Bot
added
do-not-merge/needs-area
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/test e2e-aws-miminal |
|
/test e2e-aws-minimal |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: devguyio The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
|
@devguyio: This pull request references Jira Issue OCPBUGS-67262, which is invalid:
Comment 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@devguyio: This pull request references Jira Issue OCPBUGS-67262, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
/test e2e-aws e2e-aks |
|
/auto-cc |
|
/test all |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In `@sync-global-pullsecret/sync-global-pullsecret_test.go`:
- Around line 21-24: The tests reference MockKubeletRestarter which is missing;
regenerate and commit the generated mock for the syncglobalpullsecret package so
the type exists. Run the provided mockgen command (or the go:generate target) to
produce sync-global-pullsecret_mock.go in package syncglobalpullsecret exposing
dbusConn and KubeletRestarter, then add/commit that file so symbols like
MockKubeletRestarter used by the tests compile.
| setupKubeletMock func(*MockKubeletRestarter) | ||
| expectedErrorContains []string | ||
| expectedFinalContent string | ||
| expectError bool |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore the directory structure
find . -type f -name "*sync-global-pullsecret*" | head -20Repository: openshift/hypershift
Length of output: 170
🏁 Script executed:
# Check if MockKubeletRestarter is defined anywhere in the codebase
rg "MockKubeletRestarter" --type goRepository: openshift/hypershift
Length of output: 1236
🏁 Script executed:
# Look for mock generation directives
rg "go:generate" sync-global-pullsecret/ --type goRepository: openshift/hypershift
Length of output: 254
🏁 Script executed:
# Check if the generated mock file exists
ls -la sync-global-pullsecret/sync-global-pullsecret_mock.goRepository: openshift/hypershift
Length of output: 164
🏁 Script executed:
# If it exists, check if MockKubeletRestarter is defined in it
if [ -f sync-global-pullsecret/sync-global-pullsecret_mock.go ]; then
rg "type MockKubeletRestarter" sync-global-pullsecret/sync-global-pullsecret_mock.go
echo "---"
rg "func NewMockKubeletRestarter" sync-global-pullsecret/sync-global-pullsecret_mock.go
fiRepository: openshift/hypershift
Length of output: 46
🏁 Script executed:
# Also read the lines in the test file to see actual context
sed -n '21,24p' sync-global-pullsecret/sync-global-pullsecret_test.go
echo "---"
sed -n '213,224p' sync-global-pullsecret/sync-global-pullsecret_test.goRepository: openshift/hypershift
Length of output: 589
MockKubeletRestarter is undefined; tests won't compile.
Static analysis reports MockKubeletRestarter missing. The generated mock file does not exist. Please regenerate and commit the mock (run go:generate or mockgen directly) so the type exists in package syncglobalpullsecret.
mockgen -destination=sync-global-pullsecret/sync-global-pullsecret_mock.go -package=syncglobalpullsecret ./sync-global-pullsecret . dbusConn,KubeletRestarter
Also applies to: 213-224
🧰 Tools
🪛 golangci-lint (2.5.0)
[error] 21-21: : # github.com/openshift/hypershift/sync-global-pullsecret [github.com/openshift/hypershift/sync-global-pullsecret.test]
sync-global-pullsecret/sync-global-pullsecret_test.go:21:31: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:32:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:45:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:60:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:73:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:100:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:116:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:142:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:154:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:166:30: undefined: MockKubeletRestarter
sync-global-pullsecret/sync-global-pullsecret_test.go:166:30: too many errors
(typecheck)
🤖 Prompt for AI Agents
In `@sync-global-pullsecret/sync-global-pullsecret_test.go` around lines 21 - 24,
The tests reference MockKubeletRestarter which is missing; regenerate and commit
the generated mock for the syncglobalpullsecret package so the type exists. Run
the provided mockgen command (or the go:generate target) to produce
sync-global-pullsecret_mock.go in package syncglobalpullsecret exposing dbusConn
and KubeletRestarter, then add/commit that file so symbols like
MockKubeletRestarter used by the tests compile.
|
all three failures were infra failures |
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
Good catch on this issue :). I have no hard feelings to merge this, but I think it's over engineering, maybe this could be handled in a simpler way, WDYT?
|
|
||
| // KubeletRestarter is an interface for restarting the kubelet service. | ||
| // This allows tests to inject a mock implementation. | ||
| type KubeletRestarter interface { |
There was a problem hiding this comment.
Is this interface necessary at all? I mean:
- The interfaces usually are there to expose a function in a contract for a generic behavior. In this case this is pretty specific
- The problem statement is the content comparison between files, could we change the code in that sense?
There was a problem hiding this comment.
The interface is necessary to allow for mocking Kubelet restarting. What alternative are you proposing?
There was a problem hiding this comment.
Not at all, the issue this PR is trying to address (IMHO) is the comparison between the content processed in the HCCO vs the file content in the Kubelet. Alternative for kubelet restart, we can do the same we have already, something like:
// Global variable for testing
var kubeletRestartFunc = signalKubeletToRestartProcess
// In tests
func TestCheckAndFixFile(t *testing.T) {
originalFunc := kubeletRestartFunc
defer func() { kubeletRestartFunc = originalFunc }()
kubeletRestartFunc = func() error {
return nil // or whatever error you want to test
}
}
There was a problem hiding this comment.
the issue this PR is trying to address (IMHO)
agreed, and that includes improving the unit tests of the function where that logic no?
the existing unit tests were confusing because even in the happy path cases, it expected errors:
{
name: "file exists with different content",
description: "file exists with different content, kubelet restart fails, rollback succeeds",
initialContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`,
secretContent: `{"auths":{"test.registry.com":{"auth":"dGVzdDp0ZXN0"}}}`,
rollbackShouldFail: false,
expectedErrorContains: []string{
"failed to restart kubelet after 3 attempts",
"rolled back changes",
},
expectedFinalContent: `{"auths":{"old.registry.com":{"auth":"b2xkOnRlc3Q="}}}`,
expectError: true,
},if I'm reading that unit test, I would think that if the file exists with different content, the sync function should succeed and and there should be no errors, so why is expectError set to `true?
I understand this was done because kubelet restart wasn't mocked. So here's my thought process:
There are two ways that are the convention in this repo:
- Use a hand-made mock or a toggles (as you mentioned)
type kubeletRestarter = func() error- Use mocking library for generating mocks.
My understanding is that we started recently adopting the gomock approach. For gomock, you need an interface. In both cases we need an extra type, whether it's an interface or an extra function type it depends IMHO on what you're trying to achieve with that extra type.
I'm fine switching to a hand-made mock and drop the gomock if that'll unblock this PR
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
/area control-plane-operator |
openshift-ci
Bot
added
area/control-plane-operator
|
/test all |
enxebre
added
the
acknowledge-critical-fixes-only
|
/verified by @gaol |
openshift-ci-robot
added
the
verified
|
@gaol: This PR has been marked as verified by 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. |
|
@devguyio: all tests passed! 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. |
|
@devguyio: Jira Issue OCPBUGS-67262: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-67262 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. 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. |
|
/cherrypick release-4.21 |
|
@devguyio: new pull request created: #7809 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 kubernetes-sigs/prow repository. |
|
/cherrypick release-4.21 |
|
@devguyio: new pull request created: #7914 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 kubernetes-sigs/prow repository. |
devguyio
changed the title
|
@devguyio: Jira Issue Verification Checks: Jira Issue OCPBUGS-78195 Jira Issue OCPBUGS-78195 has been moved to the MODIFIED state and will move to the VERIFIED state when the change is available in an accepted nightly payload. 🕓 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. |
…ut, and trim trailing newlines in probe The kubelet-config-verifier DaemonSet had three issues: 1. It copied openshift-config/pull-secret to kube-system at creation time. When the preceding propagation subtest restored the pull secret via t.Cleanup, the snapshot became stale. Fix: mount kube-system/original-pull-secret — the same secret the global-pull-secret-syncer uses as its source. 2. When HCCO updates original-pull-secret, it recalculates the configSeed hash and triggers a syncer DS pod restart. The verifier started checking hashes while the syncer was still restarting and had not yet written the updated content to disk. Fix: wait for the syncer DS rollout to complete (all pods updated and ready) before deploying the verifier. 3. The ignition server writes /var/lib/kubelet/config.json as compact JSON while HCCO syncs original-pull-secret in the user-supplied format (often pretty-printed with trailing newlines). The syncer handles this via bytes.TrimRight (openshift#7638), but the verifier compared raw md5sums — the trailing newline caused a permanent hash mismatch. Fix: use printf '%s' "$(cat FILE)" to strip trailing newlines before hashing, matching the syncer's bytes.TrimRight semantics. Additional changes: - Move t.Cleanup for pull secret restore to the parent test so the dummy entry stays in place during the verifier subtest. - Add fallback sentinels to the readiness probe so missing files produce distinct hashes instead of empty-string false positives. - Add UpdatedNumberScheduled check to waitForDaemonSetReady to detect in-progress rollouts. - Separate the DS readiness check into its own subtest for correct failure attribution. - Add stage-by-stage diagnostic logging to the propagation wait. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…ut, and trim trailing newlines in probe The kubelet-config-verifier DaemonSet had three issues: 1. It copied openshift-config/pull-secret to kube-system at creation time. When the preceding propagation subtest restored the pull secret via t.Cleanup, the snapshot became stale. Fix: mount kube-system/original-pull-secret — the same secret the global-pull-secret-syncer uses as its source. 2. When HCCO updates original-pull-secret, it recalculates the configSeed hash and triggers a syncer DS pod restart. The verifier started checking hashes while the syncer was still restarting and had not yet written the updated content to disk. Fix: wait for the syncer DS rollout to complete (all pods updated and ready) before deploying the verifier. 3. The ignition server writes /var/lib/kubelet/config.json as compact JSON while HCCO syncs original-pull-secret in the user-supplied format (often pretty-printed with trailing newlines). The syncer handles this via bytes.TrimRight (openshift#7638), but the verifier compared raw md5sums — the trailing newline caused a permanent hash mismatch. Fix: use printf '%s' "$(cat FILE)" to strip trailing newlines before hashing, matching the syncer's bytes.TrimRight semantics. Additional changes: - Move t.Cleanup for pull secret restore to the parent test so the dummy entry stays in place during the verifier subtest. - Add fallback sentinels to the readiness probe so missing files produce distinct hashes instead of empty-string false positives. - Add UpdatedNumberScheduled check to waitForDaemonSetReady to detect in-progress rollouts. - Separate the DS readiness check into its own subtest for correct failure attribution. - Add stage-by-stage diagnostic logging to the propagation wait. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…ut, and trim trailing newlines in probe The kubelet-config-verifier DaemonSet had three issues: 1. It copied openshift-config/pull-secret to kube-system at creation time. When the preceding propagation subtest restored the pull secret via t.Cleanup, the snapshot became stale. Fix: mount kube-system/original-pull-secret — the same secret the global-pull-secret-syncer uses as its source. 2. When HCCO updates original-pull-secret, it recalculates the configSeed hash and triggers a syncer DS pod restart. The verifier started checking hashes while the syncer was still restarting and had not yet written the updated content to disk. Fix: wait for the syncer DS rollout to complete (all pods updated and ready) before deploying the verifier. 3. The ignition server writes /var/lib/kubelet/config.json as compact JSON while HCCO syncs original-pull-secret in the user-supplied format (often pretty-printed with trailing newlines). The syncer handles this via bytes.TrimRight (openshift#7638), but the verifier compared raw md5sums — the trailing newline caused a permanent hash mismatch. Fix: use printf '%s' "$(cat FILE)" to strip trailing newlines before hashing, matching the syncer's bytes.TrimRight semantics. Additional changes: - Move t.Cleanup for pull secret restore to the parent test so the dummy entry stays in place during the verifier subtest. - Add fallback sentinels to the readiness probe so missing files produce distinct hashes instead of empty-string false positives. - Add UpdatedNumberScheduled check to waitForDaemonSetReady to detect in-progress rollouts. - Separate the DS readiness check into its own subtest for correct failure attribution. - Add stage-by-stage diagnostic logging to the propagation wait. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
…ut, and trim trailing newlines in probe The kubelet-config-verifier DaemonSet had three issues: 1. It copied openshift-config/pull-secret to kube-system at creation time. When the preceding propagation subtest restored the pull secret via t.Cleanup, the snapshot became stale. Fix: mount kube-system/original-pull-secret — the same secret the global-pull-secret-syncer uses as its source. 2. When HCCO updates original-pull-secret, it recalculates the configSeed hash and triggers a syncer DS pod restart. The verifier started checking hashes while the syncer was still restarting and had not yet written the updated content to disk. Fix: wait for the syncer DS rollout to complete (all pods updated and ready) before deploying the verifier. 3. The ignition server writes /var/lib/kubelet/config.json as compact JSON while HCCO syncs original-pull-secret in the user-supplied format (often pretty-printed with trailing newlines). The syncer handles this via bytes.TrimRight (openshift#7638), but the verifier compared raw md5sums — the trailing newline caused a permanent hash mismatch. Fix: use printf '%s' "$(cat FILE)" to strip trailing newlines before hashing, matching the syncer's bytes.TrimRight semantics. Additional changes: - Move t.Cleanup for pull secret restore to the parent test so the dummy entry stays in place during the verifier subtest. - Add fallback sentinels to the readiness probe so missing files produce distinct hashes instead of empty-string false positives. - Add UpdatedNumberScheduled check to waitForDaemonSetReady to detect in-progress rollouts. - Separate the DS readiness check into its own subtest for correct failure attribution. - Add stage-by-stage diagnostic logging to the propagation wait. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
7 participants
What this PR does / why we need it:
The global pull secret syncer tried to preserve new lines in kubelet config but that caused an E2E mismatch.
This PR fixes that and enhances the sync-global-pullsecret unit tests.
Which issue(s) this PR fixes:
Fixes OCPBUGS-67262
Special notes for your reviewer:
Checklist: