OCPBUGS-99272: Auto-remove capi-provider resource limits with user-override detection#9047
OCPBUGS-99272: Auto-remove capi-provider resource limits with user-override detection#9047sdminonne 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
jira/severity-important
|
@sdminonne: This pull request references Jira Issue OCPBUGS-99272, 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. |
|
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:
📝 WalkthroughWalkthroughThe workload reconciler now passes existing workload annotations into Sequence Diagram(s)sequenceDiagram
participant WorkloadReconciler
participant ExistingWorkload
participant DefaultOptions
participant PodTemplate
WorkloadReconciler->>ExistingWorkload: read annotations
WorkloadReconciler->>DefaultOptions: pass annotations
DefaultOptions->>PodTemplate: reconcile container resources
DefaultOptions->>PodTemplate: write removal tracking and resource hash
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
do-not-merge/needs-area
do-not-merge/work-in-progress
![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)
support/controlplane-component/defaults_test.go (1)
691-802: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required test-case description format.
support/controlplane-component/defaults_test.go#L691-L802: rename new table cases to start withWhen ... it should ....support/controlplane-component/defaults_test.go#L921-L1072: rename newt.Runcases to start withWhen ... it should ....As per coding guidelines, “Always use
When ... it should ...format for describing test cases when creating unit tests.”🤖 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 `@support/controlplane-component/defaults_test.go` around lines 691 - 802, Rename the new table-test cases in support/controlplane-component/defaults_test.go lines 691-802 to begin with “When ... it should ...”, preserving their existing behavior and descriptions. Apply the same naming format to the new t.Run cases in support/controlplane-component/defaults_test.go lines 921-1072; update only the test descriptions.Source: Coding guidelines
🤖 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 `@support/controlplane-component/defaults.go`:
- Around line 182-189: Update the pod-template resource hashing around
computeContainerResourcesHash to include both regular containers and init
containers in separate hash fields within the resourceHashAnnotation value.
Ensure changes, including removal of init-container resources, alter the
annotation and trigger reconciliation; apply the same adjustment to the
corresponding logic near the additional referenced section.
---
Nitpick comments:
In `@support/controlplane-component/defaults_test.go`:
- Around line 691-802: Rename the new table-test cases in
support/controlplane-component/defaults_test.go lines 691-802 to begin with
“When ... it should ...”, preserving their existing behavior and descriptions.
Apply the same naming format to the new t.Run cases in
support/controlplane-component/defaults_test.go lines 921-1072; update only the
test descriptions.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: eb259718-4e48-4674-816b-a0ea9a87ee30
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/hypershift/v1beta1/hostedcluster_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (4)
api/hypershift/v1beta1/hostedcluster_types.gosupport/controlplane-component/controlplane-component.gosupport/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9047 +/- ##
==========================================
+ Coverage 44.44% 44.48% +0.03%
==========================================
Files 774 774
Lines 96977 97041 +64
==========================================
+ Hits 43105 43166 +61
- Misses 50897 50899 +2
- Partials 2975 2976 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
openshift-ci
Bot
added
area/api
|
@sdminonne: 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. |
sdminonne
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdminonne 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
support/controlplane-component/defaults.go (1)
811-811: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDon't discard the
json.Marshalerror.Although marshalling
ResourceRequirementsis unlikely to fail, silently dropping the error and hashing a possibly-empty payload would produce a misleading, stable hash. Propagate or at least log it. As per path instructions, "Never ignore error returns".🤖 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 `@support/controlplane-component/defaults.go` at line 811, Handle the error returned by json.Marshal(resources) instead of discarding it. Update the surrounding hashing flow to propagate or log the marshalling failure before using data, ensuring a failed marshal cannot produce a misleading hash.Source: Path instructions
🤖 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 `@support/controlplane-component/defaults_test.go`:
- Around line 844-872: The affected subtest names in the current test block,
TestMarkLimitsRemoved, and TestLimitRemovalAndPreservation do not follow the
required convention. Rename each case to use “When … it should …”, preserving
the existing scenario and expected outcome in the wording.
---
Nitpick comments:
In `@support/controlplane-component/defaults.go`:
- Line 811: Handle the error returned by json.Marshal(resources) instead of
discarding it. Update the surrounding hashing flow to propagate or log the
marshalling failure before using data, ensuring a failed marshal cannot produce
a misleading hash.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3e1cad36-3403-4864-a24a-f82900ee305a
📒 Files selected for processing (3)
support/controlplane-component/controlplane-component.gosupport/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.go
sdminonne
changed the title
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@support/controlplane-component/defaults.go`:
- Line 820: Handle the error returned by json.Marshal in the resource hashing
flow instead of discarding it. Update the surrounding function to propagate the
marshaling error to its caller, preserving normal hash generation only when
marshaling succeeds.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 83547164-22df-46de-a12d-b5df58a601f4
📒 Files selected for processing (3)
support/controlplane-component/controlplane-component.gosupport/controlplane-component/defaults.gosupport/controlplane-component/defaults_test.go
…erride detection Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
capi-providerdeployment during reconciliation, which prevented manifest-driven limit removal from taking effect on upgradecapi-provideronly — all other components retain the existing behavior of unconditionally preserving existing resourcescapi-providerdeployment has them, automatically remove the limits and record the action via alimits-removedannotationTest plan
limitsAlreadyRemovedannotation parsingmarkLimitsRemovedannotation writing (including deduplication)computeContainerResourcesHashstability and order independencee2e-aws-upgrade-hypershift-operator— validates the rollout is safe🤖 Generated with Claude Code
Summary by CodeRabbit
capi-providercontainer resource reconciliation: when manifests omit limits, the operator now removes limits as needed while honoring prior user re-adds.setDefaultOptionscall signature.