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

[YUNIKORN-2982] Send event when preemption occurs #1000

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rhh777
Copy link

@rhh777 rhh777 commented Nov 25, 2024

What is this PR for?

When a preemption occurs, send an event to be displayed in the victim's event list.

What type of PR is it?

  • - Improvement

What is the Jira issue?

YUNIKORN-2982

How should this be tested?

Trigger preemption

Screenshots

image

Questions:

  • - At present, the default interval for event sending is 2 seconds. When sending a preemptive event, the task may have already been removed, triggering an exception that the corresponding task cannot be found, resulting in the failure of event sending.
    Narrowing the time interval can be a temporary solution, but I haven't thought of a more suitable solution at the moment.
image

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

See comments.

Comment on lines +639 to +640
// send event
message := fmt.Sprintf("Preempted by %v from application %v in %v", p.ask.GetTag("kubernetes.io/meta/podName"), p.ask.applicationID, p.application.queuePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Message creation should be in the Send* method
  2. We usually don't refer to pod names in the "core", since it's a generic scheduler and we try to stay away from K8s-specific concepts. We use allocationKey.
  3. We need a test to validate that the event is generated in case of a preemption. Use a MockEventSystem to do that. Just replace the Application.appEvents with:
mockEvents := mock.NewEventSystem()
app.appEvents = schedEvt.NewApplicationEvents(mockEvents)

You can re-use an already existing test which triggers preemption and check the contents of mockEvents.Events.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at preemption_test.go, the simplest test case is TestTryPreemption. You can add the extra code there to verify the event.

if !ae.eventSystem.IsEventTrackingEnabled() {
return
}
event := events.CreateAppEventRecord(appID, message, allocKey, si.EventRecord_REMOVE, si.EventRecord_ALLOC_PREEMPT, allocated)
Copy link
Contributor

@pbacsko pbacsko Nov 25, 2024

Choose a reason for hiding this comment

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

This must be a type of REQUEST. That will result in a real K8s event being sent to the target pod.

The removal of the allocation will be a natural consequence of that, which will flow through the scheduler, so an app-specific event is not needed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants