Remove deprecated flags in controller manager options#7271
Conversation
karmada-bot
added
the
kind/cleanup
karmada-bot
added
the
size/XS
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on code cleanup by eliminating two deprecated and unused command-line flags from the controller manager's options. This action streamlines the codebase, reduces potential confusion for users, and improves overall maintainability by removing obsolete configurations. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
dahuo98
changed the title
There was a problem hiding this comment.
Pull request overview
Removes deprecated CLI flags from karmada-controller-manager option registration, as part of the release cleanup work to fully drop flags deprecated in #7126.
Changes:
- Removed
--cluster-lease-durationand--cluster-lease-renew-interval-fractionflag registration from controller-manager options.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| flags.DurationVar(&o.LeaderElection.RetryPeriod.Duration, "leader-elect-retry-period", defaultElectionRetryPeriod.Duration, ""+ | ||
| "The duration the clients should wait between attempting acquisition and renewal "+ | ||
| "of a leadership. This is only applicable if leader election is enabled.") | ||
| flags.DurationVar(&o.ClusterLeaseDuration.Duration, "cluster-lease-duration", 40*time.Second, | ||
| "Specifies the expiration period of a cluster lease.") | ||
| _ = flags.MarkDeprecated("cluster-lease-duration", "The flag --cluster-lease-duration has been marked deprecated because it has never been used, and will be removed in a future release.") | ||
| flags.Float64Var(&o.ClusterLeaseRenewIntervalFraction, "cluster-lease-renew-interval-fraction", 0.25, | ||
| "Specifies the cluster lease renew interval fraction.") | ||
| _ = flags.MarkDeprecated("cluster-lease-renew-interval-fraction", "The flag --cluster-lease-renew-interval-fraction has been marked deprecated because it has never been used, and will be removed in a future release.") | ||
| flags.DurationVar(&o.ClusterSuccessThreshold.Duration, "cluster-success-threshold", 30*time.Second, "The duration of successes for the cluster to be considered healthy after recovery.") | ||
| flags.DurationVar(&o.ClusterFailureThreshold.Duration, "cluster-failure-threshold", 30*time.Second, "The duration of failure for the cluster to be considered unhealthy.") |
There was a problem hiding this comment.
Removing these flag bindings also removes the only place ClusterLeaseDuration and ClusterLeaseRenewIntervalFraction get non-zero defaults. NewOptions() currently leaves them at zero values, but opts.Validate() requires ClusterLeaseDuration > 0 and 0 < ClusterLeaseRenewIntervalFraction < 1, so karmada-controller-manager will now fail to start. Set defaults for these fields in NewOptions() (e.g., 40s and 0.25 as before) or remove/adjust the validation + downstream usage if they are truly no longer needed.
There was a problem hiding this comment.
As per discussed here #7009 (comment), we will stop populating these fields. I removed the validation checks on these 2 fields
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly removes two deprecated flags, --cluster-lease-duration and --cluster-lease-renew-interval-fraction, from the controller manager. The code change itself is straightforward and correct. I have added one comment regarding the formatting of the release note in the pull request description to ensure it aligns with the project's style guide.
I am having trouble creating individual review comments. Click here to see my feedback.
cmd/controller-manager/app/options/options.go (188-193)
The release note in the pull request description does not adhere to the repository's style guide. According to the style guide, release notes for this category of change should use the simple past tense.
For example:
`karmada-controller-manager`: Removed the deprecated flags `--cluster-lease-duration` and `--cluster-lease-renew-interval-fraction`.References
- Release notes for categories like features, fixes, and cleanups should use the simple past tense (e.g., 'Fixed...', 'Added...', 'Removed...'). The current release note uses present tense ('are removed now') instead of the simple past tense. (link)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7271 +/- ##
==========================================
+ Coverage 41.99% 42.02% +0.03%
==========================================
Files 874 874
Lines 53542 53537 -5
==========================================
+ Hits 22483 22497 +14
+ Misses 29372 29347 -25
- Partials 1687 1693 +6
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:
|
dahuo98
force-pushed
the
remove-deprecated-flags-from-karmada-controller-manager
branch
from
a15bd8c to
63c294d
Compare
karmada-bot
added
size/M
dahuo98
force-pushed
the
remove-deprecated-flags-from-karmada-controller-manager
branch
from
63c294d to
74dab4b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if o.ClusterStatusUpdateFrequency.Duration <= 0 { | ||
| errs = append(errs, field.Invalid(newPath.Child("ClusterStatusUpdateFrequency"), o.ClusterStatusUpdateFrequency, "must be greater than 0")) | ||
| } | ||
| if o.ClusterLeaseDuration.Duration <= 0 { | ||
| errs = append(errs, field.Invalid(newPath.Child("ClusterLeaseDuration"), o.ClusterLeaseDuration, "must be greater than 0")) | ||
| } | ||
| if o.ClusterLeaseRenewIntervalFraction <= 0 || o.ClusterLeaseRenewIntervalFraction >= 1 { | ||
| errs = append(errs, field.Invalid(newPath.Child("ClusterLeaseRenewIntervalFraction"), o.ClusterLeaseRenewIntervalFraction, "must be greater than 0 and less than 1")) | ||
| } | ||
| if o.ClusterMonitorPeriod.Duration <= 0 { | ||
| errs = append(errs, field.Invalid(newPath.Child("ClusterMonitorPeriod"), o.ClusterMonitorPeriod, "must be greater than 0")) | ||
| } |
There was a problem hiding this comment.
The validation for ClusterLeaseDuration / ClusterLeaseRenewIntervalFraction has been removed, but those fields are still present on Options (see cmd/controller-manager/app/options/options.go). If the fields are meant to be removed along with the flags, consider deleting them from the struct; if they’re intentionally kept for internal wiring, consider keeping basic validation or setting explicit defaults to prevent invalid/zero values from silently propagating.
karmada-bot
added
the
do-not-merge/work-in-progress
|
/assign |
dahuo98
force-pushed
the
remove-deprecated-flags-from-karmada-controller-manager
branch
from
74577e5 to
450056a
Compare
|
/retest |
karmada-bot
removed
the
do-not-merge/work-in-progress
@zhzhuang-zju Ready for review. Removed the deprecated flags as well as validation checks on these values. The e2e tests are a bit flaky and one of them failed initially. They passed with a retry. |
|
Thanks |
zhzhuang-zju
left a comment
zhzhuang-zju
left a comment
There was a problem hiding this comment.
Thanks, others LGTM
dahuo98
force-pushed
the
remove-deprecated-flags-from-karmada-controller-manager
branch
from
450056a to
2e8e0c9
Compare
karmada-bot
added
size/L
Signed-off-by: dahuo98 <sxdahuo@gmail.com>
dahuo98
force-pushed
the
remove-deprecated-flags-from-karmada-controller-manager
branch
from
32fcea4 to
67d01c0
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
karmada-bot
added
the
approved
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Removes deprecated flags
--cluster-lease-durationand--cluster-lease-renew-interval-fractionfor controller managerThese flags were previously deprecated in #7126
Which issue(s) this PR fixes:
Part of #7009
Special notes for your reviewer:
Does this PR introduce a user-facing change?: