-
Notifications
You must be signed in to change notification settings - Fork 859
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 failure threshold #1829
Conversation
What's your option? |
Either is fine with me, time is probably more intuitive but users need to keep Kubefed uses the number: https://github.com/kubernetes-sigs/kubefed/blob/v0.9.2/charts/kubefed/charts/controllermanager/templates/kubefedconfig.yaml#L19 |
5da23b8
to
a291bcb
Compare
Thank you, I've addressed your comments |
7be1cc0
to
3ffa906
Compare
I really want something like ClusterData which caches each cluster's last prob time and status. How do you think? |
By the way, we should clean up the data when deleting the cluster. karmada/pkg/controllers/status/cluster_status_controller.go Lines 98 to 102 in 92a134b
|
For "last prob time", kubefed has its own Condition but we are using For "status", kubefed stores it because it will restore the old status, we just return and retry in the next health check, I think they take the same effect So I don't see where we are gonna use it? Please correct me if I'm wrong |
What I mean |
OK, added |
@@ -195,6 +201,30 @@ func (c *ClusterStatusController) syncClusterStatus(cluster *clusterv1alpha1.Clu | |||
return c.updateStatusIfNeeded(cluster, currentClusterStatus) | |||
} | |||
|
|||
func (c *ClusterStatusController) retryOnFailure(online, healthy bool, cluster string) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't tell but seems something doesn't feel right here.
Let me think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have modified as you suggested
e08a0a1
to
a97bc9a
Compare
{ | ||
// the first retry | ||
online: false, | ||
healthy: false, | ||
expectedReady: metav1.ConditionTrue, | ||
}, | ||
{ | ||
// the second retry | ||
online: false, | ||
healthy: false, | ||
expectedReady: metav1.ConditionTrue, | ||
}, | ||
{ | ||
// the third retry | ||
// cluster is still unhealthy after the threshold, set cluster status to not ready | ||
online: false, | ||
healthy: false, | ||
expectedReady: metav1.ConditionFalse, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems these tests interact with each other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are observed statuses ordered by time, because thresholdReadyCondition
depends on previous statuses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this test, we are testing thresholdAdjustedReadyCondition
, we should set up cache
, current condition
, and observed condition
for each case. They should not depend on each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be kind of tricky..How to set up the cache
?(clusterDataMap
and thresholdStartTime
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are testing clusterConditionStore.thresholdAdjustedReadyCondition
where clusterConditionStore
is stateful
func TestThresholdAdjustedReadyCondition2(t *testing.T) {
tests := []struct {
name string
clusterData *clusterData
currentCondition *metav1.Condition
observedCondition *metav1.Condition
expectedCondition *metav1.Condition
}{
{
name: "cluster just joined in ready state",
clusterData: nil, // no cache yet
currentCondition: nil, // no condition was set on cluster object yet
observedCondition: &metav1.Condition{Status: metav1.ConditionTrue},
expectedCondition: &metav1.Condition{Status: metav1.ConditionTrue},
},
{
name: "cluster just joined in not-ready state",
clusterData: nil, // no cache yet
currentCondition: nil, // no condition was set on cluster object yet
observedCondition: &metav1.Condition{Status: metav1.ConditionFalse},
expectedCondition: &metav1.Condition{Status: metav1.ConditionFalse},
},
{
name: "cluster becomes not ready but still not reach threshold",
clusterData: &clusterData{
readyCondition: metav1.ConditionFalse,
thresholdStartTime: time.Now(),
},
currentCondition: &metav1.Condition{Status: metav1.ConditionTrue},
observedCondition: &metav1.Condition{Status: metav1.ConditionFalse},
expectedCondition: &metav1.Condition{Status: metav1.ConditionTrue},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
cache := clusterConditionStore{
clusterDataMap: sync.Map{},
failureThreshold: 10,
}
if test.clusterData != nil {
cache.update("foo", test.clusterData)
}
cluster := &clusterv1alpha1.Cluster{}
cluster.Name = "foo"
if test.currentCondition != nil {
meta.SetStatusCondition(&cluster.Status.Conditions, *test.currentCondition)
}
thresholdReadyCondition := cache.thresholdAdjustedReadyCondition(cluster, test.observedCondition)
if test.expectedCondition.Status != thresholdReadyCondition.Status {
t.Fatalf("expected: %s, but got: %s", test.expectedCondition.Status, thresholdReadyCondition.Status)
}
})
}
} |
Just tried add some cases(not finished). |
b5db3eb
to
339f431
Compare
Signed-off-by: dddddai <dddwq@foxmail.com>
I tested it on my side, I think we are ready to move further. Thanks @dddddai |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Currently cluster status will be set to NotReady immediately if the check failed
To make it more robust, this pr adds cluster failure threshold(default to 30s), which is the duration of failure for the cluster to be considered unhealthy.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Should the flag
cluster-failure-threshold
be the number of failures(default to 3) or the time(default to 30s)? Which is better?Does this PR introduce a user-facing change?: