add event when pod specify a not exist schedulerName #117407
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
size/S
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @olderTaoist. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
the
needs-priority
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olderTaoist The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
sig/scheduling
|
/cc PTAL @chendave @sanposhiho |
|
@olderTaoist: GitHub didn't allow me to request PR reviews from the following users: PTAL. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/assign |
pkg/scheduler/eventhandlers.go
Outdated
There was a problem hiding this comment.
We cannot go with this implementation. This scheduler doesn't match with SchedulerName, it doesn't mean the cluster doesn't have the scheduler matched with SchedulerName
Other schedulers may be working in another Pod/process.
There was a problem hiding this comment.
we need notify the SchedulerName not in kube-scheduler regardless of whether other schedulers are deployed, as for other scheduler(kube-batch、volcano) we can do nothing.
There was a problem hiding this comment.
regardless of whether other schedulers are deployed
I don't agree. SchedulerName is the API to define which scheduler is responsible for the Pod, and if SchedulerName doesn't match with this scheduler, ignores that Pod: this behavior does completely make sense.
If we record FailedScheduling here, how another scheduler (matching with SchedulerName) could work for this Pod?
There was a problem hiding this comment.
maybe the FailedScheduling maybe confuse，i chage the content of event message. as fellow
0s Warning FindScheduler pod/web-0 test scheduler not in [default-scheduler]
There was a problem hiding this comment.
This will cause multiple warning events if there are multiple schedulers, and actually someone scheduler match the given SchedulerName
There was a problem hiding this comment.
Again, I would say we cannot go with this. As @lowang-bh said, if multiple schedulers are there, all Pods will get the warning events even if everything works well.
There was a problem hiding this comment.
I agree with @sanposhiho, we shouldn't record an event because the even stream is shared between all controllers handling the pod, and so this will be confusing because the assumption here is that the other scheduler will pick up the pod and handle it. Also, here you are recording an event on each update, which will spam the event stream.
I would be onboard with a log that gets printed on add only (this will probably require adding a dedicated event handler though).
There was a problem hiding this comment.
I agree with @sanposhiho, we shouldn't record an event because the even stream is shared between all controllers handling the pod, and so this will be confusing because the assumption here is that the other scheduler will pick up the pod and handle it. Also, here you are recording an event on each update, which will spam the event stream.
i consider the scenario where the update event occurs，because the && operator, don't record a event when pod was scheduled.
There was a problem hiding this comment.
In the scenarios I encountered, there were few custom schedulers based on kube-schedule implementations, more like kube-batch、hived、volcano.
Do you have any good methods？let me try
olderTaoist
force-pushed
the
feature-scheduler-name
branch
from
eea2d34 to
6dcdd17
Compare
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
dims
added
the
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
7 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #116982
Special notes for your reviewer:
Does this PR introduce a user-facing change?