-
Notifications
You must be signed in to change notification settings - Fork 136
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
[YUNIKORN-2504] Support canonical labels for queue/applicationId in scheduler #860
Conversation
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.
To make it easier for the reviewer to read, add some notes.
- Can start from 'task.sanityCheckBeforeScheduling()' in applications.go
- Can check e2e tests in basic_scheduling_test and recover_and_restart first.
if err := task.sanityCheckBeforeScheduling(); err == nil { | ||
// if the task is not ready for scheduling, we keep it in New state | ||
// if the task pod is bounded and have conflicting metadata, we move the task to Rejected state | ||
err, rejectTask := task.sanityCheckBeforeScheduling() |
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.
Perform a sanity check before move this task to Pending state.
Before this PR, sanity check only check PVC's readiness
- If sanity check passed, move task state from 'New' -> 'Pending'
- If sanity check failed, task state remains in 'New' (Will be checked again in next schedule cycle)
After this PR (Sanity check check PVC and Pod Metadata)
- if sanity check passed, 'New' -> 'Pending'
- if sanity check fails due to PVC -> 'New' (No change)
- if sanity check fails due to a unbound pod with inconsistent metadata (AppID/Label), move task state from 'New' to 'Rejected'
Design decision: Only reject unbound pods because we don't want to failed existing running pod after restart YK.
func failTaskPodWithReasonAndMsg(task *Task, reason string, msg string) { | ||
podCopy := task.GetTaskPod().DeepCopy() | ||
podCopy.Status = v1.PodStatus{ |
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.
Moved failTaskPodWithReasonAndMsg() to task.go
change
- podCopy := task.GetTaskPod().DeepCopy()
to - podCopy := task.pod.DeepCopy()
to prevent deadlock when task state machine is handling TaskRejected event. (WLock)
constants.CanonicalLabelApplicationID: app.GetApplicationID(), | ||
constants.CanonicalLabelQueueName: app.GetQueue(), |
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.
Note:
- We can directly use canonical representation for placeholder here. The newer version shim allows legacy and canonical representation metadata coexists.
func (task *Task) postTaskRejected(reason string) { | ||
// if task is rejected because of conflicting metadata, we should fail the pod with reason | ||
if strings.Contains(reason, constants.TaskPodInconsistMetadataFailure) { | ||
task.failTaskPodWithReasonAndMsg(constants.TaskRejectedFailure, reason) |
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.
Design decision:
- Fail the pod if the task's reject reason is inconsistent metadata (PodInconsistentMetadata).
3bf0269
to
1d4979b
Compare
1d4979b
to
197e0ca
Compare
Mark this PR as draft after the discussion of the community sync-up. To be changed:
|
Close this draft PR and another PR for formal review was created. |
What is this PR for?
Support canonical Queue/ApplicationId labels in Pod, allows it coexist with the existing metadata.
Check metadata consistency before move task state from 'New' to 'Pending',
Run the pod metadata check in task.sanityCheckBeforeScheduling()
What type of PR is it?
Todos
Admission Controller should fail the pod request too if the metadata is inconsistent. Will create another Jira once this PR got approved.
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2504
How should this be tested?
Run e2e test:
Run below simple sleep pods:
Screenshots (if appropriate)
Below is the screenshot without admission controller:
Questions:
NA