✨ Manifest completion #1033
✨ Manifest completion #1033
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1033 +/- ##
==========================================
- Coverage 63.08% 62.68% -0.41%
==========================================
Files 208 209 +1
Lines 20520 20595 +75
==========================================
- Hits 12945 12909 -36
- Misses 6469 6579 +110
- Partials 1106 1107 +1
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:
|
|
Anyone understand why ManifestWork deletion isn't working on I see this note about skipped tests on the makefile
Maybe related to that? Since deleting work relies on seeing updates to the work status conditions. Should I add an exclusion for these tests to cloudevents? |
@skeeey thought? I think the main reason is here #1033 (review). The cloudevent client does not have permission to delete the work. |
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
Outdated
Show resolved
Hide resolved
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go
Outdated
Show resolved
Hide resolved
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go
Outdated
Show resolved
Hide resolved
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
Outdated
Show resolved
Hide resolved
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go
Outdated
Show resolved
Hide resolved
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go
Outdated
Show resolved
Hide resolved
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go
Outdated
Show resolved
Hide resolved
bhperry
force-pushed
the
manifest-completion
branch
3 times, most recently
from
8aa8126 to
f243782
Compare
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
Outdated
Show resolved
Hide resolved
bhperry
force-pushed
the
manifest-completion
branch
from
3c34374 to
be8e7ba
Compare
bhperry
force-pushed
the
manifest-completion
branch
from
be8e7ba to
77f3f7f
Compare
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
bhperry
force-pushed
the
manifest-completion
branch
from
77f3f7f to
08633df
Compare
## Walkthrough
This change introduces new utilities and logic for managing and aggregating manifest conditions in ManifestWork resources, including new functions for finding, pruning, and aggregating conditions. Controller logic and tests are updated to leverage these utilities, improving condition evaluation, aggregation, and completion detection. Test infrastructure and Makefile integration are also enhanced.
## Changes
| Files / Areas | Change Summary |
|--------------------------------------------------------------------------------------------------|----------------|
| pkg/work/helper/helpers.go | Added `FindManifestCondition` to locate a manifest condition by resource metadata, ignoring ordinal. |
| pkg/work/spoke/conditions/helpers.go | New file: Adds utilities for condition rule detection, pruning, and aggregation of manifest conditions. |
| pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go | Early exit in sync if `WorkComplete` condition is true; prevents further reconciliation. |
| pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go | Updated `applyManifests` and `applyOneManifest` signatures to accept work status; skips applying manifests marked as complete. |
| pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go | Refactored test setup and validation, added fields for delete options and existing conditions, improved condition assertions, and added a test for completed work. |
| pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go | Refactored to use new condition rule evaluation and aggregation utilities; prunes outdated conditions and removes old aggregation logic. |
| pkg/work/spoke/controllers/statuscontroller/availablestatus_controller_test.go | Enhanced test cases for condition rules, aggregation, and removal; improved validation logic and test coverage. |
| test/integration-test.mk | Appended `${ARGS}` to integration test targets to allow passing extra runtime arguments. |
| test/integration/util/recorder.go | Added `ConditionNotFound` and `CheckExpectedConditions` for centralized, detailed condition presence/matching checks. |
| test/integration/work/conditionrules_test.go | Replaced local condition check helper with `util.CheckExpectedConditions`; removed obsolete imports and function. |
| test/integration/work/work_test.go | Added "Work completion" test context to verify completion logic and condition rule aggregation in ManifestWork. |
| go.mod | Updated `open-cluster-management.io/api` module to a newer pseudo-version. |
## Suggested labels
`lgtm`, `approved`
## Suggested reviewers
- qiujian16📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings⏰ 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 (2)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go (1)
186-194: LGTM!Good implementation of the completion check that prevents reapplying completed manifests. Correctly uses
meta.IsStatusConditionTrueas suggested in the previous review.
🧹 Nitpick comments (1)
pkg/work/spoke/conditions/helpers.go (1)
91-126: Fix trailing space in message string.The logic is correct, but there's a trailing space in the message that should be removed.
return metav1.Condition{ Type: workapiv1.WorkAvailable, Status: metav1.ConditionUnknown, Reason: "ResourcesStatusUnknown", ObservedGeneration: generation, - Message: "cannot get any available resource ", + Message: "cannot get any available resource", }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumvendor/open-cluster-management.io/api/work/v1/types.gois excluded by!vendor/**
📒 Files selected for processing (11)
pkg/work/helper/helpers.go(1 hunks)pkg/work/spoke/conditions/helpers.go(1 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go(1 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go(5 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go(9 hunks)pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go(2 hunks)pkg/work/spoke/controllers/statuscontroller/availablestatus_controller_test.go(6 hunks)test/integration-test.mk(2 hunks)test/integration/util/recorder.go(2 hunks)test/integration/work/conditionrules_test.go(9 hunks)test/integration/work/work_test.go(3 hunks)
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: skeeey
PR: open-cluster-management-io/ocm#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.
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (2)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#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.
Learnt from: skeeey
PR: open-cluster-management-io/ocm#1053
File: pkg/server/services/addon/addon_test.go:208-225
Timestamp: 2025-06-27T09:46:24.579Z
Learning: In the Open Cluster Management addon service, deletion actions are not expected to occur. The OnDelete handler method is implemented as a no-op, and test coverage for deletion is not needed since this operation is not part of the service's expected functionality.
test/integration/work/conditionrules_test.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#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.
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#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.
test/integration/work/work_test.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#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.
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#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.
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go (1)
Learnt from: skeeey
PR: open-cluster-management-io/ocm#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.
🧬 Code Graph Analysis (3)
test/integration/work/conditionrules_test.go (1)
test/integration/util/recorder.go (2)
CheckExpectedConditions(93-112)ConditionNotFound(17-17)
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (1)
pkg/work/spoke/conditions/helpers.go (2)
AggregateManifestConditions(36-84)PruneConditionsGeneratedByConditionRules(27-31)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go (1)
pkg/work/helper/helpers.go (1)
FindManifestCondition(401-411)
⏰ 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). (8)
- GitHub Check: cloudevents-integration
- GitHub Check: unit
- GitHub Check: integration
- GitHub Check: verify
- GitHub Check: build
- GitHub Check: e2e-singleton
- GitHub Check: e2e
- GitHub Check: e2e-hosted
🔇 Additional comments (28)
pkg/work/helper/helpers.go (1)
400-411: LGTM! Clean and efficient helper function.The
FindManifestConditionfunction is well-implemented with proper use of the existingresetOrdinalhelper for consistent comparison. The function correctly normalizes ordinal fields before matching and returns a pointer for efficiency.pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (1)
136-139: LGTM! Proper early exit for completed ManifestWork.This early exit condition correctly prevents unnecessary processing of ManifestWork resources that are already complete, aligning with the PR objectives to ignore updates to completed resources.
test/integration-test.mk (1)
34-67: LGTM! Consistent test infrastructure enhancement.The addition of
${ARGS}to all test execution commands provides flexible runtime argument passing, supporting enhanced integration test scenarios. The change is consistently applied across all test targets.test/integration/work/conditionrules_test.go (2)
140-145: LGTM! Improved condition checking with centralized utility.The refactoring to use
util.CheckExpectedConditionscentralizes condition validation logic and provides consistent error handling. This improves code maintainability and test reliability.
342-346: LGTM! Proper use of ConditionNotFound sentinel.The use of
util.ConditionNotFoundcorrectly asserts that the "NotWellKnown" condition should not be present, demonstrating proper testing of condition absence scenarios.test/integration/util/recorder.go (2)
15-18: LGTM! Clear sentinel value for condition absence testing.The
ConditionNotFoundconstant with value "-" provides a clear and intuitive way to assert that a condition should not exist in test scenarios.
93-112: LGTM! Comprehensive condition checking utility.The
CheckExpectedConditionsfunction provides excellent centralized condition validation with:
- Support for both presence and absence checking via the
ConditionNotFoundsentinel- Detailed error aggregation for comprehensive feedback
- Proper reuse of existing
MatchConditionfunction- Clear and descriptive error messages for debugging
This utility significantly improves test code maintainability and consistency across the codebase.
pkg/work/spoke/conditions/helpers.go (4)
13-21: LGTM!Clear and well-documented helper for identifying conditions generated by rules.
23-31: LGTM!Effective implementation for pruning stale conditions. The logic correctly identifies conditions from deleted rules by checking the observed generation.
33-84: LGTM!Well-structured aggregation logic that properly collects and combines manifest conditions. Good practice to always include the Available condition and ensure consistent ordering.
128-155: LGTM!Good implementation for aggregating conditions from rules. The status determination logic follows the correct precedence, and the message handling appropriately distinguishes between single and multiple conditions.
test/integration/work/work_test.go (2)
47-47: LGTM!Good test infrastructure improvement using the functional options pattern for test-specific ManifestWork customization.
Also applies to: 82-82, 87-89
881-952: LGTM!Comprehensive test for the work completion feature. Properly validates that manifest condition rules correctly aggregate to the WorkComplete condition.
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (2)
134-143: LGTM!Good refactoring that improves modularity by leveraging the new condition helpers. The flow is cleaner with centralized aggregation and proper pruning of stale conditions.
199-219: LGTM!Well-implemented method that properly evaluates condition rules and updates conditions in place. Good documentation explaining why pruning is necessary due to multiple controllers managing conditions.
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go (1)
136-136: LGTM!Correct addition of workStatus parameter to enable manifest completion checking.
Also applies to: 145-145, 148-148, 160-160
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller_test.go (3)
502-508: LGTM!Good enhancement to the test structure that enables comprehensive validation of both manifest-level and work-level conditions.
571-679: LGTM!Comprehensive test coverage for condition aggregation and pruning scenarios. Good attention to detail in setting the generation for proper pruning validation.
Also applies to: 685-687
733-756: LGTM!Well-structured validation logic using the CheckExpectedConditions utility. The helper function update provides good flexibility for test setup.
Also applies to: 761-772
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler_test.go (9)
111-113: Good refactoring to centralize condition validation logic.The use of
util.CheckExpectedConditionsimproves consistency across the test suite and reduces code duplication.
120-120: Good extension to support delete option testing.The addition of
deleteOptionfield enables testing of the new TTL-based deletion functionality mentioned in the PR objectives.
129-129: Proper addition of existing work conditions support.The
existingWorkConditionsfield and its initialization enable testing scenarios where ManifestWork resources already have conditions set, which is crucial for testing the completion logic.Also applies to: 152-152
206-209: Well-implemented builder method following established patterns.The
withExistingWorkConditionmethod maintains consistency with the existing builder pattern and provides a clean API for test setup.
233-252: Excellent enhancement to handle different work action types.The switch statement properly handles both patch and delete actions, improving type safety and providing clear error messages for unexpected scenarios. The logic that prevents condition validation on delete actions is sound.
256-261: Improved work condition validation with better error handling.The use of
util.CheckExpectedConditionsmaintains consistency, and the enhanced error handling that reports all errors before failing is a good improvement.
264-272: Good refactoring to reduce code duplication.The
newManifestWorkhelper method centralizes ManifestWork creation logic, making test setup more consistent and maintainable across all test cases.
365-368: Well-designed test case for the new completion functionality.This test case properly validates that ManifestWork resources with the Complete condition set to True are ignored during reconciliation, which aligns with the PR objectives.
373-373: Consistent adoption of the new helper method.All test cases now use
newManifestWork()which improves consistency and maintainability across the test suite.Also applies to: 403-403, 542-542, 583-583
Signed-off-by: Ben Perry <[email protected]>
Signed-off-by: Ben Perry <[email protected]>
|
/approve I think it is ready to merge, hold in case anyone want to take another look. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bhperry, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
openshift-merge-bot
bot
merged commit c5e776c
into
open-cluster-management-io:main
Summary
Related issue(s)
open-cluster-management-io/api#378
open-cluster-management-io/enhancements#139
#842
Summary by CodeRabbit