🌱 Deprecate work healthCheck use healthChecker instead #324
Conversation
WalkthroughThe changes refactor health check logic for deployment and workload probes by replacing deprecated single-resource health check functions with new multi-result health checker functions. Function signatures, return types, and usages are updated accordingly across controllers, utilities, and tests. Deprecated fields and functions are marked, and references are updated to use the new implementations. Additionally, test assertions and data are adjusted to reflect the new health check behavior and status conditions. Changes
Estimated code review effort3 (30–60 minutes) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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 (1)
✨ 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 (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zhujian7 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 |
![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 selected for processing (4)
pkg/addonmanager/controllers/agentdeploy/healthcheck_sync.go(4 hunks)pkg/addonmanager/controllers/agentdeploy/healthcheck_sync_test.go(1 hunks)pkg/agent/inteface.go(1 hunks)pkg/utils/probe_helper.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
pkg/addonmanager/controllers/agentdeploy/healthcheck_sync.go (1)
pkg/utils/probe_helper.go (2)
DeploymentAvailabilityHealthChecker(108-111)WorkloadAvailabilityHealthChecker(113-121)
pkg/utils/probe_helper.go (1)
pkg/agent/inteface.go (1)
FieldResult(198-204)
⏰ 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: e2e-hosted
- GitHub Check: e2e-cloudevents
- GitHub Check: e2e
- GitHub Check: build
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: verify
🔇 Additional comments (8)
pkg/agent/inteface.go (1)
178-182: LGTM! Proper deprecation marking.The deprecation comment follows Go conventions and clearly indicates that
HealthCheckershould be used instead.pkg/addonmanager/controllers/agentdeploy/healthcheck_sync_test.go (1)
1300-1316: Good refactoring to use standardized health checker.The change from a custom health check function to
utils.DeploymentAvailabilityHealthCheckerimproves code consistency and aligns with the new health checker approach.pkg/addonmanager/controllers/agentdeploy/healthcheck_sync.go (3)
277-283: Correct migration to health checker functions.The changes properly handle the transition by returning
nilfor the deprecated health check parameter and the new health checker function in the appropriate position. This maintains the method signature while migrating to the new approach.
288-314: Method signature correctly updated to use AddonHealthCheckerFunc.The return type change and the switch to
utils.DeploymentAvailabilityHealthCheckeralign with the new health checker approach.
316-348: Consistent update to AddonHealthCheckerFunc.The method signature and return value are correctly updated to use
utils.WorkloadAvailabilityHealthChecker, maintaining consistency with the deployment prober changes.pkg/utils/probe_helper.go (3)
23-39: Correct update to use HealthChecker in NewDeploymentProber.The function now properly uses the
HealthCheckerfield withDeploymentAvailabilityHealthChecker, aligning with the deprecation of theHealthCheckfield.
41-65: Consistent update to use HealthChecker in NewAllDeploymentsProber.The function correctly uses the new
HealthCheckerfield, maintaining consistency with theNewDeploymentProberchanges.
108-121: Well-structured health checker functions.The new health checker functions properly handle multiple probe results, and the delegation from
DeploymentAvailabilityHealthCheckertoWorkloadAvailabilityHealthCheckeravoids code duplication.
| // Deprecated: use DeploymentAvailabilityHealthChecker instead. | ||
| func AllDeploymentsAvailabilityHealthCheck(results []agent.FieldResult, | ||
| cluster *clusterv1.ManagedCluster, addon *v1alpha1.ManagedClusterAddOn) error { | ||
| cluster *clusterv1.ManagedCluster, addon *addonapiv1alpha1.ManagedClusterAddOn) error { | ||
| if len(results) < 2 { | ||
| return fmt.Errorf("all deployments are not available") | ||
| } | ||
|
|
||
| for _, result := range results { | ||
| if err := WorkloadAvailabilityHealthCheck(result.ResourceIdentifier, result.FeedbackResult); err != nil { | ||
| if err := checkWorkloadAvailabilityHealth(result.ResourceIdentifier, result.FeedbackResult); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Inconsistent deprecation pattern for AllDeploymentsAvailabilityHealthCheck.
The function signature has been changed to accept []agent.FieldResult instead of maintaining the original signature. This breaks backward compatibility for a deprecated function. Consider either:
- Keeping the original signature for the deprecated function
- Creating a new function with the new signature instead of modifying the deprecated one
🤖 Prompt for AI Agents
In pkg/utils/probe_helper.go around lines 93 to 106, the deprecated function
AllDeploymentsAvailabilityHealthCheck has been modified to accept a new
parameter type []agent.FieldResult, breaking backward compatibility. To fix
this, revert the function signature to its original form to maintain
compatibility, and create a new separate function with the updated signature for
the new behavior instead of changing the deprecated one.
There was a problem hiding this comment.
I did not change the original signature
There was a problem hiding this comment.
@zhujian7 You're absolutely right, and I apologize for the confusion. Looking more carefully at the diff, I can see that the only change to the AllDeploymentsAvailabilityHealthCheck function signature is the import alias from *v1alpha1.ManagedClusterAddOn to *addonapiv1alpha1.ManagedClusterAddOn. Both aliases refer to the same underlying type (open-cluster-management.io/api/addon/v1alpha1.ManagedClusterAddOn), so this is not a breaking change to the function signature - it's just a refactoring for import alias consistency.
The function parameters and their types remain functionally identical. Thank you for the correction!
There was a problem hiding this comment.
v1alpha1.ManagedClusterAddOn and addonapiv1alpha1.ManagedClusterAddOn are the same one
There was a problem hiding this comment.
@zhujian7 Exactly! That's precisely what I acknowledged in my previous response. Both v1alpha1.ManagedClusterAddOn and addonapiv1alpha1.ManagedClusterAddOn are just different import aliases for the same underlying type from open-cluster-management.io/api/addon/v1alpha1. The change in the diff is purely a refactoring for import alias consistency and doesn't introduce any breaking changes to the function signature. Thank you for confirming this understanding.
Signed-off-by: zhujian <[email protected]>
zhujian7
force-pushed
the
deprecate-healthcheck
branch
from
5b4e451 to
61ac803
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/utils/probe_helper_test.go (1)
117-122: LGTM! Test successfully updated to use the new HealthChecker interface.The refactoring from the deprecated
HealthCheckmethod toHealthCheckeris implemented correctly:
- The
agent.FieldResultstruct properly encapsulates the resource identifier and feedback result- The batch-style interface is used appropriately with a single test case
- Test logic and error handling remain intact
Consider adding test cases that exercise the batch processing capability with multiple
FieldResultentries to ensure comprehensive coverage of the new interface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/addonmanager/controllers/agentdeploy/healthcheck_sync.go(5 hunks)pkg/addonmanager/controllers/agentdeploy/healthcheck_sync_test.go(3 hunks)pkg/agent/inteface.go(1 hunks)pkg/utils/probe_helper.go(4 hunks)pkg/utils/probe_helper_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/agent/inteface.go
- pkg/addonmanager/controllers/agentdeploy/healthcheck_sync_test.go
- pkg/utils/probe_helper.go
- pkg/addonmanager/controllers/agentdeploy/healthcheck_sync.go
⏰ 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: e2e-cloudevents
- GitHub Check: e2e-hosted
- GitHub Check: build
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: e2e
- GitHub Check: verify
🔇 Additional comments (1)
pkg/utils/probe_helper_test.go (1)
11-11: LGTM! Import addition is necessary for the new interface.The import of the agent package is correctly added to support the
agent.FieldResultstruct used in the updated test.
Signed-off-by: zhujian <[email protected]>
|
/cc @zhiweiyin318 |
|
/cc @qiujian16 |
|
/lgtm |
openshift-merge-bot
bot
merged commit ee47752
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
Refactor
Bug Fixes
Chores