[YUNIKORN-2235] Add new RESTful API for retrieving application #750

laysfire · 2023-12-07T12:57:16Z

What is this PR for?

This PR add 2 RESTful API.
One for retrieving one application object directly via /ws/v1/partition/{partitionName}/application/{applicationID}.
The other for listing application IDs via /ws/v1/partition/{partitionName}/applications/{state}.

What type of PR is it?

Todos

N/A

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2235

How should this be tested?

It's been locally tested using go test

Screenshots (if appropriate)

N/A

Questions:

N/A

pbacsko

See comments.

pkg/scheduler/partition.go

pkg/webservice/handlers.go

pkg/scheduler/partition.go

pkg/webservice/handlers.go

pkg/webservice/handlers_test.go

codecov · 2023-12-08T14:31:58Z

Codecov Report

Attention: 35 lines in your changes are missing coverage. Please review.

Comparison is base (ebf7107) 77.94% compared to head (b88f25d) 77.91%.
Report is 20 commits behind head on master.

Files	Patch %	Lines
pkg/metrics/scheduler.go	29.41%	24 Missing ⚠️
pkg/metrics/queue.go	68.00%	8 Missing ⚠️
pkg/webservice/handlers.go	96.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #750      +/-   ##
==========================================
- Coverage   77.94%   77.91%   -0.03%     
==========================================
  Files          82       82              
  Lines       13373    13523     +150     
==========================================
+ Hits        10424    10537     +113     
- Misses       2622     2659      +37     
  Partials      327      327

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

chia7712

nice patch! a couple of comments left.

chia7712 · 2023-12-13T08:00:56Z

pkg/webservice/handlers.go

@@ -60,6 +60,12 @@ const (
 	NodeDoesNotExists        = "Node not found"
 )

+var allowedAppStates = map[string]bool{


Pardon me, why only partial states are allowed?

It was discussed upstream w/ Wilfred. We don't want to return all kinds of states. A comment here would be useful.

@laysfire could you share the discussion to me? Also, adding the comment here can helper readers in the future :)

chia7712 · 2023-12-13T08:05:55Z

pkg/webservice/dao/application_info.go

@@ -56,3 +56,7 @@ type PlaceholderDAOInfo struct {
 	Replaced      int64            `json:"replaced,omitempty"`
 	TimedOut      int64            `json:"timedout,omitempty"`
 }
+
+type ApplicationIDsDAOInfo struct {
+	AppIDs []string


Does it need json tag?

For another, other responses name the id-related field "applicationId". Should we keep the consistency?

yes, i will add json tag & keep consistency

pkg/webservice/handlers.go

chia7712 · 2023-12-13T08:19:26Z

pkg/scheduler/partition.go

@@ -425,6 +425,10 @@ func (pc *PartitionContext) removeAppInternal(appID string) *objects.Application
 	return app
 }

+func (pc *PartitionContext) GetApplication(appID string) *objects.Application {


this exposed method should get lock (pc.RLock()/defer pc.RUnlock()), and the internal method should be unlocked version.

hi @chia7712 thanks for review.
The are so many places call the internal method.
Which way do you think is better?
First,

Add RLock/RUnlock in exposed method & Remove RLock/RUnlock in internal method

Add explicit RLock/RUnlock snippet in every place that call the internal method

Second,
Just change the exposed name

Add RLock/RUnlock in exposed method & Remove RLock/RUnlock in internal method

yep

Add explicit RLock/RUnlock snippet in every place that call the internal method

How about just replacing internal method getApplication by public version GetApplication? Small change is good at avoiding new bugs :)

Also, you can file new Jira as follow-up to improve lock usage if you observe any weird use case.

It might be a package internal method until this point. Lots of places calling it but they are all in the package. The partition is the place where the events from the shim get processed and the scheduling cycle runs from. That means we have multiple go routines that could be changing values stored in the partition. The locking makes sure we have no data races and a consistent view.
The internal method was locked for that reason and needs to stay like that.

pbacsko

Some new comments.

pkg/scheduler/partition.go

pkg/webservice/handlers.go

pbacsko · 2023-12-13T09:49:12Z

pkg/webservice/handlers.go

@@ -60,6 +60,12 @@ const (
 	NodeDoesNotExists        = "Node not found"
 )

+var allowedAppStates = map[string]bool{


It was discussed upstream w/ Wilfred. We don't want to return all kinds of states. A comment here would be useful.

manirajv06 · 2023-12-14T06:24:30Z

@laysfire I have shared my views in jira. Please check.

laysfire · 2023-12-14T12:35:24Z

@manirajv06 @chia7712 @pbacsko Address some comment, please help review this again.

chia7712 · 2023-12-14T12:43:54Z

pkg/webservice/routes.go

+	route{
+		"Scheduler",
+		"GET",
+		"/ws/v1/partition/:partition/applications/:state",


this endpoint is under discussion. https://issues.apache.org/jira/browse/YUNIKORN-2235

Updated jira with more details. Please check.

pkg/webservice/handlers.go

wilfred-s · 2023-12-19T06:32:34Z

Dropped in a large comment in the jira:

laysfire · 2023-12-20T09:07:49Z

@manirajv06 @wilfred-s @chia7712 @pbacsko please help review again.
Summary are as follows:

Expose GetApplication on partition with nolock version
Make getApplication handler handle /ws/v1/partition/{partitionName}/application/{applicationID}
Add getPartitionApplicationByState to retrieve applications by types
We only allow 3 state: Active(fake), Rejected and Completed
We only allow New, Accepted, Starting, Running, Completing, Failing and Resuming status for Active state
I do not modify the return DAOs, like clean allocations for completed application. I think we can done this in follow up jira.

pkg/webservice/handlers.go

pbacsko

Two questions.

pbacsko

Almost. One more round.

pkg/webservice/handlers.go

pbacsko · 2023-12-26T17:01:45Z

pkg/webservice/handlers.go

+	partition := vars.ByName(strings.ToLower("partition"))
+	appState := vars.ByName(strings.ToLower("state"))


Partitions are case-sensitive, so drop that.
For appState, the proper approach is:

appState := strings.ToLower(vars.ByName("state"))

pkg/webservice/handlers.go

laysfire · 2023-12-28T03:33:38Z

@pbacsko Thanks for review. Have support case-insensitive for appState & activeState. But i add many strings.ToLower in code. I'm not sure if this is a good practice?

pbacsko

I agree that this change too much distracting stuff. Let's get rid of the ToLower() calls and just use plain string literals. See my comments.

pbacsko · 2024-01-02T00:30:48Z

pkg/webservice/handlers.go

+	allowedAppActiveStates[strings.ToLower(objects.New.String())] = true
+	allowedAppActiveStates[strings.ToLower(objects.Accepted.String())] = true
+	allowedAppActiveStates[strings.ToLower(objects.Starting.String())] = true
+	allowedAppActiveStates[strings.ToLower(objects.Running.String())] = true
+	allowedAppActiveStates[strings.ToLower(objects.Completing.String())] = true
+	allowedAppActiveStates[strings.ToLower(objects.Failing.String())] = true
+	allowedAppActiveStates[strings.ToLower(objects.Resuming.String())] = true


We might need a better naming here. It's a bit difficult to distingush between allowedAppStates and allowedAppActiveStates. At least some comments:

var allowedStatesMsg string // returned error message when the requested application state is invalid var allowedActiveStatesMsg string // returned error message when the actual state of the application is not an active state like Running, Accepted, etc. var allowedAppStates map[string]bool // ??? (this one is a bit tough to explain, we mix two existing states with a higher level one which) var allowedAppActiveStates map[string]bool // list of application states that are valid for filtering

We can actually just drop allowedAppStates because it only contains 3 values. Readability is important, so this is better:

if appState != "active" && appState != "rejected" && appState != "completed" { buildJSONErrorResponse(w, "Only following application states are allowed: active, rejected, completed", http.StatusBadRequest) return }

You can also just drop the strings.ToLower() calls, it makes the code a bit clumsy. Just write:

allowedAppActiveStates["new"] = true allowedAppActiveStates["accepted"] = true allowedAppActiveStates["starting"] = true ...

Obviously this is hard-coded and we no longer reference the code which define the states, but something for something. However, it's pretty unlikely that the state machine will ever change. You can add a comment to application_state.go above the const section:

// Application states are used for filtering in the webservice handlers. Please check&update the logic if needed if the state machine is modified

Sometimes we need to be pragmatic.

pkg/webservice/handlers.go

pbacsko

+1 LGTM

I'll let others review it, too.

chia7712

@laysfire thanks for this nice feature. two small comments left. please take a look.

chia7712 · 2024-01-03T17:44:59Z

pkg/webservice/handlers.go

+		return
+	}
+	if appState != "active" && appState != "rejected" && appState != "completed" {
+		buildJSONErrorResponse(w, "Only following application states are allowed: active, rejected, completed", http.StatusBadRequest)


Could we move this line to line#683? for example:

case "active": if status := strings.ToLower(r.URL.Query().Get("status")); status != "" { if !allowedAppActiveStates[status] { buildJSONErrorResponse(w, allowedActiveStatesMsg, http.StatusBadRequest) return } for _, app := range partitionContext.GetApplications() { if strings.ToLower(app.CurrentState()) == status { appList = append(appList, app) } } } else { appList = partitionContext.GetApplications() } case "rejected": appList = partitionContext.GetRejectedApplications() case "completed": appList = partitionContext.GetCompletedApplications() default: buildJSONErrorResponse(w, "Only following application states are allowed: active, rejected, completed", http.StatusBadRequest) return }

Cool. Move #659 to "default" branch make code clearer and default branch would be possible to hit.

chia7712 · 2024-01-03T17:51:24Z

pkg/webservice/handlers.go

+	case "active":
+		if status := strings.ToLower(r.URL.Query().Get("status")); status != "" {
+			if !allowedAppActiveStates[status] {
+				buildJSONErrorResponse(w, allowedActiveStatesMsg, http.StatusBadRequest)


Should allowedActiveStatesMsg be renamed to allowedActiveStatusMsg if the query key is called "statue"?

chia7712

@laysfire sorry that I am overengineering today. one small comment left.

BTW, please file follow-up to doc the new APIs

chia7712 · 2024-01-04T14:35:42Z

pkg/webservice/handlers.go

+	for k := range allowedAppActiveStates {
+		activeStates = append(activeStates, k)
+	}
+	allowedActiveStatusMsg = fmt.Sprintf("Only following active states are allowed: %s", strings.Join(activeStates, ","))


Could you change active states to active statuses? The query key is status so I prefer naming consistency.

@chia7712 Thanks for review. In order to keep consistency, should change allowedAppActiveStates to allowedAppActiveStatuses?

chia7712

LGTM

pbacsko assigned laysfire Dec 7, 2023

pbacsko requested changes Dec 7, 2023

View reviewed changes

pbacsko requested review from manirajv06, wilfred-s and zhuqi-lucas December 7, 2023 15:05

laysfire requested a review from pbacsko December 8, 2023 07:38

chia7712 reviewed Dec 13, 2023

View reviewed changes

pbacsko requested changes Dec 13, 2023

View reviewed changes

laysfire force-pushed the YUNIKORN-2235 branch from e49ab1e to 80417b4 Compare December 14, 2023 10:21

chia7712 reviewed Dec 14, 2023

View reviewed changes

pbacsko reviewed Dec 15, 2023

View reviewed changes

pkg/webservice/handlers.go Outdated Show resolved Hide resolved

YUNIKORN-2235. Add new RESTful API for retrieving application

17c25b2

laysfire force-pushed the YUNIKORN-2235 branch from 684dd38 to 17c25b2 Compare December 20, 2023 08:46

pbacsko reviewed Dec 20, 2023

View reviewed changes

pkg/webservice/handlers.go Outdated Show resolved Hide resolved

pbacsko reviewed Dec 20, 2023

View reviewed changes

pkg/webservice/handlers.go Outdated Show resolved Hide resolved

pbacsko requested changes Dec 20, 2023

View reviewed changes

code review

ea1554a

laysfire requested a review from pbacsko December 26, 2023 03:03

pbacsko requested changes Dec 26, 2023

View reviewed changes

pbacsko reviewed Dec 26, 2023

View reviewed changes

pkg/webservice/handlers.go Outdated Show resolved Hide resolved

code review

ad02026

laysfire requested a review from pbacsko December 28, 2023 03:33

pbacsko requested changes Jan 2, 2024

View reviewed changes

code review

c2199db

laysfire requested a review from pbacsko January 3, 2024 09:09

pbacsko approved these changes Jan 3, 2024

View reviewed changes

chia7712 reviewed Jan 3, 2024

View reviewed changes

code review

41ee501

chia7712 reviewed Jan 4, 2024

View reviewed changes

code review

b88f25d

chia7712 approved these changes Jan 5, 2024

View reviewed changes

chia7712 closed this in 77e19f6 Jan 5, 2024

		partition := vars.ByName(strings.ToLower("partition"))
		appState := vars.ByName(strings.ToLower("state"))

[YUNIKORN-2235] Add new RESTful API for retrieving application #750

[YUNIKORN-2235] Add new RESTful API for retrieving application #750

Conversation

laysfire commented Dec 7, 2023 • edited Loading

What is this PR for?

What type of PR is it?

Todos

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

pbacsko left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 8, 2023 • edited Loading

Codecov Report

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laysfire Dec 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbacsko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manirajv06 commented Dec 14, 2023

laysfire commented Dec 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilfred-s commented Dec 19, 2023

laysfire commented Dec 20, 2023

pbacsko left a comment

Choose a reason for hiding this comment

pbacsko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laysfire commented Dec 28, 2023

pbacsko left a comment

Choose a reason for hiding this comment

pbacsko Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

pbacsko left a comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

laysfire commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 8, 2023 •

edited

Loading

laysfire Dec 13, 2023 •

edited

Loading

pbacsko Jan 2, 2024 •

edited

Loading