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

api: add related evals to eval details #12305

Merged
merged 3 commits into from
Mar 17, 2022
Merged

api: add related evals to eval details #12305

merged 3 commits into from
Mar 17, 2022

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Mar 16, 2022

The related query param is used to indicate that the request should
return a list of related (next, previous, and blocked) evaluations.

Closes #11858

@lgfa29 lgfa29 added this to the 1.3.0 milestone Mar 16, 2022
continue
}

eval, err := s.EvalByID(ws, current)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This call will add the eval watch channel to the watchset, which means we can have multiple channels in ws. I don't know if there are any performance implications in this approach.

A alternative would be to set the watchset using the job ID prefix. This would use a single watch channel, but may fire in situations where an eval for another with the same prefix changes.

Comment on lines +10640 to +10659
type EvaluationStub struct {
ID string
Namespace string
Priority int
Type string
TriggeredBy string
JobID string
NodeID string
DeploymentID string
Status string
StatusDescription string
WaitUntil time.Time
NextEval string
PreviousEval string
BlockedEval string
CreateIndex uint64
ModifyIndex uint64
CreateTime int64
ModifyTime int64
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This list is fairly arbitrary and will be mostly dictated by the UI. Status and StatusDescription are probably the most important right now, but since this is only returned in an opt-in request I added most of the fields.

Using an Evalution directly in RelatedEvals caused a recursive issue while decoding the struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

just making a notes of the stuff left out:

- JobModifyIndex
- NodeModifyIndex
- Wait
- FailedTGAllocs
- ClassEligibility
- EscapedComputedClass
- QuotaLimitReached
- AnnotatePlan
- QueuedAllocations
- SnapshotIndex

@@ -10586,6 +10587,10 @@ type Evaluation struct {
// to constraints or lacking resources.
BlockedEval string

// RelatedEvals is a list of all the evaluations that are related (next,
// previous, or blocked) to this one. It may be nil if not requested.
RelatedEvals []*EvaluationStub
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally this was a map, but a list would be a little easier for the UI to consume.

Copy link
Member

Choose a reason for hiding this comment

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

I ran into a similar kerfuffle when working on fuzzy search, we should probably have some kind of methodology for agreeing on a schema before starting implementation work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the biggest problem for me is that I don't fully understand the UI data layer 😅

I wrote some questionable JS code to validate this endpoint and a list seemed to align better with how data is loaded into the UI. But I will validate this with someone that knows what they are doing.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM; just some thoughts

todo = append(todo, eval.RelatedIDs()...)
relatedEvals = append(relatedEvals, eval.Stub())
done[eval.ID] = true
}
Copy link
Member

Choose a reason for hiding this comment

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

Currently the only way out of this loop is to scan all related evals (or experience an error). Is there a way to limit the scanning we do, like on the size of the accumulated relatedEvals or even just a hard stop at like 20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hum...good question. I think the problem with setting an upper bound is that the response then becomes incomplete, and the idea of this flag is to give all the data needed quickly. But guarding against potential infinite loops, or slow requests sounds like a good idea. I don't know what a good number would be, so perhaps we can have a time limit, like 1s?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the most correct thing to do would be to plumb the Request.Context all the way down here. (You could make the same argument for all calls into the state store, it's just that this one feels less strongly bounded than the others)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point. Is this the channel I should be listening for connection close? https://pkg.go.dev/github.com/hashicorp/yamux#Session.CloseChan

Copy link
Member

Choose a reason for hiding this comment

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

I think we want the http request context

https://pkg.go.dev/net/http#Request.Context

stemming from the request handler http.Request

https://github.com/hashicorp/nomad/blob/main/command/agent/eval_endpoint.go#L10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok. Unfortunately that's currently not possible 😬
#7119

Copy link
Member

Choose a reason for hiding this comment

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

Whelp so much for that! It's probably fine as-is anyway, I mean how many related evals could there be anyway ... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

fwiw, another endpoint returns all the evaluations for a job in one go, so this is strictly less than that... can't be too bad, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, good point. One of the differences here is that we have a (potentially) infinite loop if the todo list never gets to zero. But I think we're safe because we always pop and check if eval has been processed already.

@@ -10586,6 +10587,10 @@ type Evaluation struct {
// to constraints or lacking resources.
BlockedEval string

// RelatedEvals is a list of all the evaluations that are related (next,
// previous, or blocked) to this one. It may be nil if not requested.
RelatedEvals []*EvaluationStub
Copy link
Member

Choose a reason for hiding this comment

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

I ran into a similar kerfuffle when working on fuzzy search, we should probably have some kind of methodology for agreeing on a schema before starting implementation work

lgfa29 and others added 3 commits March 17, 2022 11:13
The `related` query param is used to indicate that the request should
return a list of related (next, previous, and blocked) evaluations.

Co-authored-by: Jasmine Dahilig <[email protected]>
@lgfa29 lgfa29 force-pushed the f-api-related-evals branch from a835636 to c8a690f Compare March 17, 2022 17:07
@vercel vercel bot temporarily deployed to Preview – nomad March 17, 2022 17:07 Inactive
@lgfa29 lgfa29 merged commit 81687c1 into main Mar 17, 2022
@lgfa29 lgfa29 deleted the f-api-related-evals branch March 17, 2022 17:56
@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 Oct 25, 2022
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.

Add related evaluations to API
3 participants