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

scheduler: fix reconnecting allocations getting rescheduled #24165

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

mismithhisler
Copy link
Member

@mismithhisler mismithhisler commented Oct 10, 2024

In scenarios where a disconnected client reconnects with failed replacement allocs, the reconcile strategy for picking the correct allocation was not working. These changes allow evaluation of multiple replacement allocs, and the stopping of allocs that are in a client terminal status.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

This is looking great @mismithhisler!

I ran a test coverage report via go test -v -count=1 -cover -coverprofile=$HOME/cover.out ./scheduler && go tool cover -html=$HOME/cover.out and found that we're missing coverage in two places in this method. Normally we're not sticklers for test coverage but this is clearly a very tricky area of the code. The two areas are:

  • The block starting if stopReconnecting (ref)
  • The block where keepAlloc == replacementAlloc. (ref).

Let's try to add a test that covers these two areas, to make sure we're not missing anything. And don't forget to run make cl to add a change log entry!

scheduler/reconcile.go Show resolved Hide resolved
scheduler/reconcile.go Show resolved Hide resolved
@mismithhisler mismithhisler added backport/1.7.x backport to 1.7.x release line backport/1.8.x backport to 1.8.x release line backport/1.9.x backport to 1.9.x release line backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent and removed backport/1.7.x backport to 1.7.x release line backport/1.8.x backport to 1.8.x release line labels Oct 10, 2024
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/ent/1.7.x+ent Changes are backported to 1.7.x+ent backport/ent/1.8.x+ent Changes are backported to 1.8.x+ent backport/1.9.x backport to 1.9.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants