OCPBUGS-61228: Bump Route generation when spec is updated #550
Conversation
Kubernetes resources should bump their generation every time a field on spec is changed. This allows controllers to keep track of the desired state and the current state. This change: * initializes the generation of a resource if it does not exist * updates the generation of a resource when a field on spec is changed * adds some unit tests to assure the desired behavior
openshift-ci-robot
added
jira/severity-moderate
WalkthroughImplements generation tracking for Route resources: set Generation=1 on create and increment Generation on Spec changes during updates; metadata or status-only changes do not affect Generation. Adds unit tests validating allocation, initial generation, metadata-no-op, and spec-change increment. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Strategy as Route Strategy
participant Alloc as Hostname Allocator
rect rgb(240,248,255)
note over Client,Strategy: Create Route
Client->>Strategy: PrepareForCreate(route)
Strategy->>Alloc: AllocateHostname(route)
Alloc-->>Strategy: mygeneratedhost.com
Strategy-->>Client: route.Generation = 1
end
rect rgb(245,245,245)
note over Client,Strategy: Update Route
Client->>Strategy: PrepareForUpdate(newRoute, oldRoute)
Strategy->>Strategy: Semantic.DeepEqual(oldRoute.Spec, newRoute.Spec)
alt Specs equal
Strategy-->>Client: Keep Generation (unchanged)
else Specs differ
Strategy-->>Client: newRoute.Generation = oldRoute.Generation + 1
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@rikatz: This pull request references Jira Issue OCPBUGS-61228, 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. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rikatz: This pull request references Jira Issue OCPBUGS-61228, 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. |
|
@rikatz: This pull request references Jira Issue OCPBUGS-61228, which is valid. 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. |
![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 (4)
pkg/route/apiserver/registry/route/strategy.go (1)
153-157: Also freeze Generation for status updatesStatus updates must never alter Generation. Mirror old Generation here to prevent spoofing.
func (routeStatusStrategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { newRoute := obj.(*routeapi.Route) oldRoute := old.(*routeapi.Route) newRoute.Spec = oldRoute.Spec + newRoute.Generation = oldRoute.Generation }pkg/route/apiserver/registry/route/strategy_test.go (3)
1146-1148: Fix failure message: printing wrong variableUse newRoute.Generation in the “got” branch.
- t.Fatalf("Expected generation after metadata update to still be 1, got %d", simpleRoute.Generation) + t.Fatalf("Expected generation after metadata update to still be 1, got %d", newRoute.Generation)
1153-1155: Fix failure message: printing wrong variableSame issue here; print newRoute.Generation.
- t.Fatalf("Expected generation after spec update to be 2, got %d", simpleRoute.Generation) + t.Fatalf("Expected generation after spec update to be 2, got %d", newRoute.Generation)
1116-1156: Add a guard test for client-supplied Generation on metadata-only updatesRecommend a test that attempts to set newRoute.Generation to a bogus value while leaving Spec unchanged, and asserts it remains equal to old.Generation after PrepareForUpdate. This complements the proposed strategy fix.
I can draft this test if you’d like.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
pkg/route/apiserver/registry/route/strategy.go(3 hunks)pkg/route/apiserver/registry/route/strategy_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/route/apiserver/registry/route/strategy_test.go (2)
pkg/route/apiserver/registry/route/strategy.go (1)
NewStrategy(46-55)vendor/github.com/openshift/api/route/v1/types.go (1)
Route(48-61)
🔇 Additional comments (3)
pkg/route/apiserver/registry/route/strategy.go (2)
8-8: Semantic equality import: LGTMUsing apiequality.Semantic.DeepEqual is the right choice for spec comparisons.
70-70: Initialize Generation on create: LGTMSetting Generation to 1 on create aligns with kube conventions.
pkg/route/apiserver/registry/route/strategy_test.go (1)
1116-1134: Good coverage of generation lifecycleNice assertions for: pre-create (0), post-create (1), and no bump on metadata-only update.
|
/assign @Miciah |
|
I doubt I can approve in this repository but let's give a try. /approve |
|
thanks! I think per https://github.com/openshift/openshift-apiserver/blob/main/pkg/route/OWNERS we need @knobunc to approve it. Maybe you and Miciah should also be added to approvers? :) |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87, knobunc, rikatz 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
|
Test plan executed:
As a follow up PR, I will use the OTE existing here and add some e2e tests for this case (and others), but for now I am keeping it separated to not overload the PR with non-related changes |
|
/verified by "TestRouteGenerationManagement" |
openshift-ci-robot
added
the
verified
|
@rikatz: 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. |
|
@rikatz: 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. |
|
@rikatz: Jira Issue Verification Checks: Jira Issue OCPBUGS-61228 Jira Issue OCPBUGS-61228 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. |
|
6 participants
Kubernetes resources should bump their generation every time a field on spec is changed. This allows controllers to keep track of the desired state and the current state.
This change:
Summary by CodeRabbit
New Features
Bug Fixes
Tests