Extend Binding API for graceful eviction #2273
Conversation
karmada-bot
added
kind/api-change
karmada-bot
added
the
size/L
RainbowMango
force-pushed
the
pr_gracefull_eviction_api
branch
3 times, most recently
from
fdee34b
to
a392a7d
Compare
|
/assign |
| // The intended workflow is: | ||
| // 1. Once the controller(such as 'taint-manager') decided to evict the resource that | ||
| // is referenced by current ResourceBinding or ClusterResourceBinding from a target | ||
| // cluster, it removes(or scale down the replicas) the target from Clusters(.spec.Clusters) |
There was a problem hiding this comment.
(or scale down the replicas)
Do we need to move the scale down replicas into GracefulEvictionTasks?
There was a problem hiding this comment.
Yes, e.g.
spec:
clusters:
- name: member2
replicas: 10
- name: member1
replicas: 10evict 5 replicas from member1, then
spec:
clusters:
- name: member2
replicas: 10
- name: member1
replicas: 5
gracefulEvictionTasks:
- name: member1
replicas: 5
...There was a problem hiding this comment.
This may cause problems. .spec.clusters describes the current cluster replicas distribution status. When the number of member2 replicas changes from 10 to 5, the number of workload replicas will be directly changed when ensuringWork in binding-controller. This will conflict with the eviction behavior.
There was a problem hiding this comment.
When the number of member2 replicas changes from 10 to 5,
member2 do you mean member1?
There was a problem hiding this comment.
Clerical error. It does refer to member1.
There was a problem hiding this comment.
Yeah, that would be too complicated, and given the real use case is uncertain yet, I suppose we can remove the replicas from gracefulEvictionTasks. Now only focus on evicting the entire resource from clusters.
@Garrybest What do you say?
[edit] we can't remove the replicas, just focus on the eviction scenario of taint-manager.
| // to take over the evicting workload(resource). | ||
| // 3. The graceful eviction controller takes care of the graceful eviction tasks and | ||
| // performs the final removal after the workload(resource) is available on the substitute | ||
| // cluster or exceed the grace termination period(defaults to 10 minutes). |
There was a problem hiding this comment.
the grace termination period(defaults to 10 minutes).
Is this a configurable parameter?
There was a problem hiding this comment.
Yes, probably. And a new field might be needed to hold the configurable parameter.
| // If SchedulerObservedGeneration is less than the generation in metadata means the scheduler hasn't confirmed | ||
| // the scheduling result or hasn't done the schedule yet. | ||
| // +optional | ||
| SchedulerObservedGeneration int64 `json:"schedulerObservedGeneration,omitempty"` |
There was a problem hiding this comment.
So what if the evicition task has not been done yet but there comes a scaling-up event, how to deal with this situation by using SchedulerObservedGeneration?
There was a problem hiding this comment.
I'm not sure if I understand you correctly.
A general idea in my mind is that scheduler shouldn't schedule the resource(in binding) to a cluster which already in one of the graceful eviction tasks.
RainbowMango
force-pushed
the
pr_gracefull_eviction_api
branch
from
a392a7d
to
f1b67a9
Compare
Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>
RainbowMango
force-pushed
the
pr_gracefull_eviction_api
branch
from
f1b67a9
to
89f9c96
Compare
|
/lgtm |
|
/lgtm |
RainbowMango
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: 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 api-change
/kind feature
What this PR does / why we need it:
This PR proposed the API change which is used to perform graceful eviction.
Which issue(s) this PR fixes:
Part of #2281
Special notes for your reviewer:
Does this PR introduce a user-facing change?: