🐛 fix: Check Applied condition before evaluating rollout status #1243
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
/hold |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: haoqing0110 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughAdds Applied-condition + ObservedGeneration gating to ManifestWork rollout status evaluation and updates related tests; swaps cloudevents config loader usage to the builder variant in several components; updates Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (86)
📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (5)📚 Learning: 2025-07-23T10:10:42.066ZApplied to files:
📚 Learning: 2025-10-28T02:55:13.893ZApplied to files:
📚 Learning: 2025-09-08T06:10:32.388ZApplied to files:
📚 Learning: 2025-07-01T02:27:10.927ZApplied to files:
📚 Learning: 2025-09-03T08:43:34.751ZApplied to files:
🧬 Code graph analysis (4)pkg/work/hub/manager.go (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (1)
pkg/work/spoke/spokeagent.go (1)
test/integration/work/manifestworkreplicaset_test.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (8)
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 |
haoqing0110
force-pushed
the
fix-rollout-applied-condition-check
branch
from
af4dc06 to
31d8a33
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (86)
go.sumis excluded by!**/*.sumvendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/option.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/write_producer_message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/00version.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminapi.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminoptions.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/api.htmlis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_dynamic.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_windows.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/config.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/consumer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/context.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error_gen.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/event.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/generated_errors.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/glue_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/handle.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/header.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/kafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/LICENSES.txtis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/bundle-import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_windows.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka_mock.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/log.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/metadata.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/misc.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/mockcluster.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/offset.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/producer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/select_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/testconf-example.jsonis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/time.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/apis/cluster/v1alpha1/rollout.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/options/generic.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/constants/constants.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/agentclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/baseclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/sourceclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/metrics/metrics_collector.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options_noop.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/ratelimiter.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/utils/ratelimiter.gois excluded by!vendor/**
📒 Files selected for processing (2)
go.mod(1 hunks)pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
Applied to files:
go.mod
🧬 Code graph analysis (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (1)
vendor/open-cluster-management.io/api/work/v1/types.go (1)
WorkApplied(500-500)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: e2e-hosted
- GitHub Check: unit
- GitHub Check: e2e-singleton
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (2)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (2)
221-221: Good addition of Applied condition check.Retrieving the Applied condition aligns with the PR objective to check hub-side application status before evaluating agent-side conditions.
224-240: No backward compatibility issue—code correctly handles ManifestWorks without Applied condition.The Applied condition is set by the spoke-side controller (pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go:108) during normal manifest reconciliation. ManifestWorks created without the Applied condition is expected behavior, explicitly documented in appliedmanifestwork_reconciler.go:42–47 with graceful handling for missing conditions. The lines 224–240 correctly gate rollout evaluation until the condition exists and observes the current generation—this is proper design, not a bug. Old ManifestWorks will eventually have the condition set as the spoke applies them.
Likely an incorrect or invalid review comment.
Comment on lines
+5
to
+6
| replace open-cluster-management.io/sdk-go => github.com/haoqing0110/sdk-go v0.0.0-20251105081906-aab7c4782f40 | ||
|
|
There was a problem hiding this comment.
Replace personal fork with official upstream release.
The replace directive points to a personal fork (github.com/haoqing0110/sdk-go) with a pseudo-version, which introduces supply chain risk and makes the build non-reproducible. Personal forks should not be used in production code.
Please take one of the following actions:
- Preferred: Upstream your changes to the official
open-cluster-management.io/sdk-gorepository and wait for an official release - Temporary workaround (if urgent): Document why this temporary fork is necessary, link to the upstream PR, and create an issue to track replacing it with the official release
Once the changes are available in the official repository, update to use a proper semantic version:
-replace open-cluster-management.io/sdk-go => github.com/haoqing0110/sdk-go v0.0.0-20251105081906-aab7c4782f40
+// Remove replace directive once sdk-go releases version with required changesCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In go.mod around lines 5-6, the replace directive points to a personal fork
(github.com/haoqing0110/sdk-go) with a pseudo-version; replace it by using the
official upstream module and a proper semantic version once available: remove
the replace directive and set the require to open-cluster-management.io/sdk-go
at the released semver; if an immediate release isn’t available and you must
keep the fork temporarily, add a comment in the repository documenting why the
fork is used, link the upstream PR, and open a tracked issue to revert to
upstream when a release is made (then update go.mod to the official semver and
remove the replace).
f0f9234 to
82c57fa
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1243 +/- ##
=======================================
Coverage 62.15% 62.16%
=======================================
Files 209 209
Lines 16968 16971 +3
=======================================
+ Hits 10547 10550 +3
Misses 5304 5304
Partials 1117 1117
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (1)
902-1135: Comprehensive test coverage, but consider adding one more scenario.The test cases thoroughly cover various combinations of Progressing and Degraded conditions, and correctly inject the Applied condition in all scenarios that should progress past ToApply status.
However, consider adding a test case for:
- Applied condition exists but
Applied.ObservedGeneration != ManifestWork.Generation(should return ToApply)This scenario is explicitly checked in the reconcile logic (lines 234-235 of manifestworkreplicaset_deploy_reconcile.go) but not directly tested here.
Add this test case to verify the Applied ObservedGeneration gating:
{ name: "applied condition with unobserved generation - should return ToApply", manifestWork: &workapiv1.ManifestWork{ ObjectMeta: metav1.ObjectMeta{ Name: "test-mw", Namespace: "cls1", Generation: 2, CreationTimestamp: creationTime, }, Status: workapiv1.ManifestWorkStatus{ Conditions: []metav1.Condition{ { Type: workapiv1.WorkApplied, Status: metav1.ConditionTrue, ObservedGeneration: 1, // Stale generation LastTransitionTime: now, Reason: "Applied", }, { Type: workapiv1.WorkProgressing, Status: metav1.ConditionFalse, ObservedGeneration: 2, LastTransitionTime: now, Reason: "Completed", }, }, }, }, expectedStatus: clustersdkv1alpha1.ToApply, expectedLastTransition: nil, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (86)
go.sumis excluded by!**/*.sumvendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/LICENSEis excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/message.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/option.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/protocol.gois excluded by!vendor/**vendor/github.com/cloudevents/sdk-go/protocol/kafka_confluent/v2/write_producer_message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/LICENSEis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/00version.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminapi.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/adminoptions.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/api.htmlis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_darwin_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_dynamic.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_glibc_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_amd64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_musl_linux_arm64.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/build_windows.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/config.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/consumer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/context.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/error_gen.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/event.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/generated_errors.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/glue_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/handle.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/header.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/kafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/.gitignoreis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/LICENSES.txtis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/README.mdis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/bundle-import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/import.shis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_darwin_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_glibc_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_amd64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_musl_linux_arm64.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/librdkafka_windows.ais excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/librdkafka_vendor/rdkafka_mock.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/log.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/message.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/metadata.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/misc.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/mockcluster.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/offset.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/producer.gois excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/select_rdkafka.his excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/testconf-example.jsonis excluded by!vendor/**vendor/github.com/confluentinc/confluent-kafka-go/v2/kafka/time.gois excluded by!vendor/**vendor/modules.txtis excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/apis/cluster/v1alpha1/rollout.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/addon/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/cluster/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/csr/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/event/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/lease/client.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/options/generic.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/source/client/manifestwork.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/constants/constants.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/agentclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/baseclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/clients/sourceclient.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/metrics/metrics_collector.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/grpc/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/options_noop.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/kafka/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/agentoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/mqtt/sourceoptions.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/options.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/ratelimiter.gois excluded by!vendor/**vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/utils/ratelimiter.gois excluded by!vendor/**
📒 Files selected for processing (6)
go.mod(1 hunks)pkg/registration/register/grpc/spoke_driver.go(3 hunks)pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go(1 hunks)pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go(8 hunks)pkg/work/hub/manager.go(2 hunks)pkg/work/spoke/spokeagent.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- pkg/registration/register/grpc/spoke_driver.go
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
Applied to files:
pkg/work/hub/manager.gopkg/work/spoke/spokeagent.go
📚 Learning: 2025-10-28T02:55:13.893Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1224
File: pkg/registration/register/grpc/spoke_driver.go:89-98
Timestamp: 2025-10-28T02:55:13.893Z
Learning: In pkg/registration/register/grpc/spoke_driver.go (Go), when calling cloudeventscsr.NewAgentClientHolder with GenericClientOptions, the watcher store does not need to be explicitly provided via WithClientWatcherStore. The GenericClientOptions.AgentClient() method automatically creates a default AgentInformerWatcherStore if none is provided, which satisfies the NewAgentClientHolder requirements.
Applied to files:
pkg/work/hub/manager.gopkg/work/spoke/spokeagent.go
📚 Learning: 2025-09-03T08:43:34.751Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: test/integration/work/completedmanifestwork_test.go:216-256
Timestamp: 2025-09-03T08:43:34.751Z
Learning: In TTL=0 ManifestWork deletion tests, avoid waiting for WorkComplete condition because the controller deletes the resource immediately upon completion, creating a race condition where the test tries to check completion status on an already-deleted resource.
Applied to files:
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go
📚 Learning: 2025-07-01T02:27:10.927Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1053
File: vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go:237-237
Timestamp: 2025-07-01T02:27:10.927Z
Learning: In OCM ManifestWork agent client, when a work is being deleted (DeletionTimestamp set and finalizers removed), the agent publishes a status update event (types.UpdateRequestAction) with ResourceDeleted condition set to True to inform the hub that deletion is complete, rather than publishing a delete request event.
Applied to files:
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go
📚 Learning: 2025-09-08T06:10:32.388Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1164
File: test/integration/work/deleteoption_test.go:42-42
Timestamp: 2025-09-08T06:10:32.388Z
Learning: The startWorkAgent function in the OCM test suite uses variadic parameters for decorators (...agentOptionsDecorator), allowing it to be called with just ctx and clusterName without requiring an explicit nil decorator parameter.
Applied to files:
pkg/work/spoke/spokeagent.go
🧬 Code graph analysis (4)
pkg/work/hub/manager.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
NewConfigLoader(24-29)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (1)
vendor/open-cluster-management.io/api/work/v1/types.go (1)
WorkApplied(500-500)
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (1)
vendor/open-cluster-management.io/api/work/v1/types.go (1)
WorkApplied(500-500)
pkg/work/spoke/spokeagent.go (1)
vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/generic/options/builder/optionsbuilder.go (1)
NewConfigLoader(24-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: verify
- GitHub Check: e2e-singleton
- GitHub Check: cloudevents-integration
- GitHub Check: e2e-hosted
- GitHub Check: e2e
🔇 Additional comments (5)
pkg/work/hub/manager.go (1)
21-21: LGTM! Clean refactoring to builder-based config loading.The migration from
generic.NewConfigLoadertobuilder.NewConfigLoaderis straightforward and maintains the same API surface. Error handling and control flow remain unchanged.Also applies to: 80-80
pkg/work/spoke/spokeagent.go (1)
25-25: LGTM! Consistent builder-based config loading.The change aligns with the builder pattern migration seen in
pkg/work/hub/manager.go. The multi-line formatting of the config loader call improves readability.Also applies to: 229-230
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_reconcile.go (2)
231-233: Excellent documentation of the ordering requirement.The comment clearly explains why Applied must be checked before Progressing/Degraded to prevent using stale timestamps from previous generations. This is critical for correct rollout timeout calculations.
221-240: Fix misleading comment about which controller sets WorkApplied condition.Line 224 comment incorrectly states Applied condition is set by "hub controller" when it's actually set by the spoke/agent controller during manifest reconciliation. Update the comment to accurately reflect that Applied represents the spoke controller's view of work application state. The logic itself is correct—returning
ToApplyfor works without an Applied condition is the intended safe behavior, not a breaking change. Existing ManifestWorks will naturally receive the Applied condition as the spoke controller reconciles them.Likely an incorrect or invalid review comment.
pkg/work/hub/controllers/manifestworkreplicasetcontroller/manifestworkreplicaset_deploy_test.go (1)
594-601: Good test setup aligning with the new Applied condition requirement.The test correctly sets the Applied condition with matching ObservedGeneration before simulating the Progressing/Degraded states. This ensures the gating logic allows evaluation of rollout status.
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> Signed-off-by: Qing Hao <[email protected]>
haoqing0110
force-pushed
the
fix-rollout-applied-condition-check
branch
from
82c57fa to
65130d6
Compare
Member
Author
|
/assign @qiujian16 @youngbupark |
Contributor
|
@haoqing0110: GitHub didn't allow me to assign the following users: youngbupark. Note that only open-cluster-management-io members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In 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. |
annelaucg
reviewed
Nov 7, 2025
| // IMPORTANT: Check Applied condition FIRST to ensure the work has been properly applied | ||
| // before checking agent-side conditions. This prevents using stale timestamps from | ||
| // previous generations when conditions update their ObservedGeneration without changing Status. | ||
| if appliedCond == nil || |
There was a problem hiding this comment.
Is this the only change made to fix this issue?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🤖 Generated with Claude Code
Summary
Related issue(s)
#1237
Fixes #
Summary by CodeRabbit
Chores
Refactor
Tests