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

show preemptions in nomad plan CLI #4823

Merged
merged 4 commits into from
Nov 8, 2018
Merged

show preemptions in nomad plan CLI #4823

merged 4 commits into from
Nov 8, 2018

Conversation

preetapan
Copy link
Contributor

This PR augments the output of nomad plan with info about preemptions.

It uses a tiered approach to make sure the CLI doesn't show every single allocid if there are many preemptions. Instead, it groups things and shows summary counts.

There are three cases:

1 - Number of preempted allocations less than 10

Alloc ID  Job ID  Task Group
alloc1    jobID1  test
alloc2    jobID2  test2

2 - Number of unique jobids amongst all preemptions less than 10

Job ID  Namespace  Job Type  Preemptions
job0    test1       batch        3
job1    test2       batch        3
job2    test3       service      3
job3    test4       batch        3

3 - Number of unique jobids amongst all preemptions greater than 10

Job Type  Preemptions
batch     10
service   20

Builds on top of #4794, which should be merged first.

for _, alloc := range resp.Annotations.PreemptedAllocs {
allocs = append(allocs, fmt.Sprintf("%s|%s|%s", alloc.ID, alloc.JobID, alloc.TaskGroup))
}
c.Ui.Output(formatList(allocs))
Copy link
Member

Choose a reason for hiding this comment

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

[nit] Feel free to return early here and remove else. It would reduce nesting and make the big else clause easier to follow.

@@ -7,8 +7,12 @@ import (
"strings"
"testing"

"strconv"
Copy link
Member

Choose a reason for hiding this comment

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

goimport misformatting

"github.com/hashicorp/nomad/testutil"
"github.com/mitchellh/cli"
require2 "github.com/stretchr/testify/require"
Copy link
Member

Choose a reason for hiding this comment

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

require2?

@preetapan preetapan merged commit 8573c9f into master Nov 8, 2018
@preetapan preetapan deleted the f-preemption-plan-cli branch November 8, 2018 15:55
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants