Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cluster success threshold #1884

Merged
merged 1 commit into from
Jun 17, 2022
Merged

Conversation

dddddai
Copy link
Member

@dddddai dddddai commented May 25, 2022

What type of PR is this?
/kind feature

What this PR does / why we need it:
Cluster success threshold is the duration of successes for the cluster to be considered healthy after recovery.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
We should be careful about the default value as it could slow down the cluster recovery, personally I'd prefer set the default value to 0 for keeping user experience as before, if users do need this threshold, they could set it.

Does this PR introduce a user-facing change?:

`karmada-controller-manager/karmada-agent`: Introduced `--cluster-success-threshold` flag to specify cluster success threshold, default to 30s.

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label May 25, 2022
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 25, 2022
threshold = c.failureThreshold
}

if isConditionTrue(observedReadyCondition) != isConditionTrue(curReadyCondition) &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if isConditionTrue(observedReadyCondition) != isConditionTrue(curReadyCondition) &&
if observedReadyCondition != curReadyCondition &&

I can't see what's the difference between this, and the unit test seems ok with the change I'm suggesting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the condition can be True/False/Unknown, here we should only distinguish "true" and "not true" I think

For example, if the condition is changed from False to Unknown, we should update to Unknown immediately rather than retaining False

Copy link
Member

@RainbowMango RainbowMango Jun 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if the condition is changed from False to Unknown,

The condition should never change from true/false to unknown. Any other examples?

[edit]: Ignore the comments.

pkg/controllers/status/cluster_status_controller.go Outdated Show resolved Hide resolved
pkg/controllers/status/cluster_status_controller.go Outdated Show resolved Hide resolved
@RainbowMango
Copy link
Member

@dddddai
Now we cached the ready status in memory, if it is possible to remove the cache and turn to the LastTransitionTime ?

For a similar idea, I also post at #1945 (comment).

@dddddai
Copy link
Member Author

dddddai commented Jun 16, 2022

@dddddai Now we cached the ready status in memory, if it is possible to remove the cache and turn to the LastTransitionTime ?

For a similar idea, I also post at #1945 (comment).

Not really, we can't tell when the "observed" cluster status changed through LastTransitionTime

@RainbowMango
Copy link
Member

Not really, we can't tell when the "observed" cluster status changed through LastTransitionTime

Give an example?

Signed-off-by: dddddai <dddwq@foxmail.com>
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022
@karmada-bot
Copy link
Collaborator

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2022
@karmada-bot karmada-bot merged commit 4c496a3 into karmada-io:master Jun 17, 2022
@RainbowMango RainbowMango added this to the v1.3 milestone Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants