Fix(rbg): update role not trigger rolling update#89
Conversation
Syspretor
force-pushed
the
fix/fix-update-not-effect
branch
from
d973de1 to
bc57435
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the container comparison logic by replacing custom field-by-field comparison functions with a simpler approach that uses API server defaults normalization and reflect.DeepEqual. The change removes approximately 370 lines of test code and 150 lines of production code, significantly simplifying the codebase.
Key changes:
- Introduced a normalization approach using
legacyscheme.Scheme.Defaultto apply API server defaults before comparing containers - Replaced detailed field-by-field container comparison with a single
reflect.DeepEqualcall - Removed three helper functions:
containerEqual,envVarsEqual, andvolumeMountsEqual
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/reconciler/pod_reconciler.go | Replaced custom container comparison functions with a normalization-based approach using legacyscheme, reducing complexity while maintaining comparison functionality |
| pkg/reconciler/pod_reconciler_test.go | Removed detailed unit tests for container, env vars, and volume mount comparison functions that are no longer needed |
Comments suppressed due to low confidence (1)
pkg/reconciler/pod_reconciler_test.go:1
- The removal of
Test_containerEqual,Test_envVarsEqual, andTestContainerEqualeliminates comprehensive test coverage for container field comparisons including commands, args, env vars with different orders, startup/liveness/readiness probes, volume mounts, and resources. The new normalization approach inpodSpecEquallacks dedicated tests. Consider adding tests that verify the normalization behavior handles API server defaults correctly, especially for fields like ImagePullPolicy that had special handling in the old code.
package reconciler
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| normalizeByApiserverDefaults := func(c corev1.Container) corev1.Container { | ||
| c.Env = utils.FilterSystemEnvs(c.Env) | ||
| p := &corev1.Pod{Spec: corev1.PodSpec{Containers: []corev1.Container{c}}} | ||
| legacyscheme.Scheme.Default(p) | ||
| return p.Spec.Containers[0] | ||
| } |
There was a problem hiding this comment.
The normalizeByApiserverDefaults function creates a new Pod object and applies scheme defaults for each container comparison in the loop. This approach is inefficient when comparing multiple containers. Consider creating a helper function that normalizes all containers once before the comparison loop, or cache the normalized containers to avoid repeated Pod allocations and scheme default applications.
Syspretor
force-pushed
the
fix/fix-update-not-effect
branch
from
bc57435 to
babed6d
Compare
Syspretor
force-pushed
the
fix/fix-update-not-effect
branch
from
babed6d to
2a8f79b
Compare
Ⅰ. Motivation
role.templateis compared with the StatefulSet’s template. However, the StatefulSet’s template has been processed by the API server and includes defaulted fields (e.g., ReadinessProbe) that are absent in role.template. This causes RBG to continually think the StatefulSet needs an update and repeatedly set the StatefulSet’s partition to the replicas, preventing the intended changes from triggering a rolling update.In this PR, we apply the API server’s defaults to role.template before performing the deep-equality comparison with the StatefulSet’s template.
role.templateand the StatefulSet’s template, only a subset of fields was considered, leaving out fields such as SecurityContext.In this PR, we sort the Container list and compare the entire Container objects directly, without focusing on specific inner fields.
Ⅱ. Modifications
Ⅲ. Does this pull request fix one issue?
fixes #85
Ⅳ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅴ. Describe how to verify it
VI. Special notes for reviews
Checklist
make fmt.