You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@yunikorn.apache.org by wi...@apache.org on 2023/11/27 05:53:32 UTC
(yunikorn-core) branch master updated: [YUNIKORN-2163] Fix HTTP status codes in REST handlers (#729)
This is an automated email from the ASF dual-hosted git repository.
wilfreds pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/yunikorn-core.git
The following commit(s) were added to refs/heads/master by this push:
new 3b9c9661 [YUNIKORN-2163] Fix HTTP status codes in REST handlers (#729)
3b9c9661 is described below
commit 3b9c966157962787ec4e03bb3ca3c7093870b48b
Author: Peter Bacsko <pb...@cloudera.com>
AuthorDate: Mon Nov 27 16:47:25 2023 +1100
[YUNIKORN-2163] Fix HTTP status codes in REST handlers (#729)
An object does not exist: 400 Bad Request --> 404 Not Found
Internal metrics is disabled: 501 Not Implemented --> 500 Internal Server Error
Event tracking disabled: 400 Bad Request --> 500 Internal Server Error
removal of dead code for writing a new config
Closes: #729
Signed-off-by: Wilfred Spiegelenburg <wi...@apache.org>
---
pkg/webservice/handlers.go | 47 ++++++++++++--------------------
pkg/webservice/handlers_test.go | 59 ++++++++++++++---------------------------
2 files changed, 37 insertions(+), 69 deletions(-)
diff --git a/pkg/webservice/handlers.go b/pkg/webservice/handlers.go
index 382dd230..ee4ecf22 100644
--- a/pkg/webservice/handlers.go
+++ b/pkg/webservice/handlers.go
@@ -358,7 +358,7 @@ func getNodeUtilisation(w http.ResponseWriter, r *http.Request) {
writeHeaders(w)
partitionContext := schedulerContext.GetPartitionWithoutClusterID(configs.DefaultPartition)
if partitionContext == nil {
- buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusInternalServerError)
return
}
// calculate the dominant resource based on root queue usage and size
@@ -422,7 +422,7 @@ func getApplicationHistory(w http.ResponseWriter, r *http.Request) {
// There is nothing to return but we did not really encounter a problem
if imHistory == nil {
- buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusNotImplemented)
+ buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusInternalServerError)
return
}
// get a copy of the records: if the array contains nil values they will always be at the
@@ -439,7 +439,7 @@ func getContainerHistory(w http.ResponseWriter, r *http.Request) {
// There is nothing to return but we did not really encounter a problem
if imHistory == nil {
- buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusNotImplemented)
+ buildJSONErrorResponse(w, "Internal metrics collection is not enabled.", http.StatusInternalServerError)
return
}
// get a copy of the records: if the array contains nil values they will always be at the
@@ -507,20 +507,7 @@ func checkHealthStatus(w http.ResponseWriter, r *http.Request) {
}
}
-func buildUpdateResponse(err error, w http.ResponseWriter) {
- if err == nil {
- w.WriteHeader(http.StatusOK)
- if _, err = w.Write([]byte("Configuration updated successfully")); err != nil {
- buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
- }
- } else {
- log.Log(log.REST).Info("Configuration update failed with errors",
- zap.Error(err))
- buildJSONErrorResponse(w, err.Error(), http.StatusConflict)
- }
-}
-
-func getPartitions(w http.ResponseWriter, r *http.Request) {
+func getPartitions(w http.ResponseWriter, _ *http.Request) {
writeHeaders(w)
lists := schedulerContext.GetPartitionMapClone()
@@ -543,7 +530,7 @@ func getPartitionQueues(w http.ResponseWriter, r *http.Request) {
if partition != nil {
partitionQueuesDAOInfo = partition.GetPartitionQueues()
} else {
- buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound)
return
}
if err := json.NewEncoder(w).Encode(partitionQueuesDAOInfo); err != nil {
@@ -566,7 +553,7 @@ func getPartitionNodes(w http.ResponseWriter, r *http.Request) {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
}
} else {
- buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound)
}
}
@@ -583,7 +570,7 @@ func getPartitionNode(w http.ResponseWriter, r *http.Request) {
nodeID := vars.ByName("node")
node := partitionContext.GetNode(nodeID)
if node == nil {
- buildJSONErrorResponse(w, NodeDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, NodeDoesNotExists, http.StatusNotFound)
return
}
nodeDao := getNodeDAO(node)
@@ -591,7 +578,7 @@ func getPartitionNode(w http.ResponseWriter, r *http.Request) {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
}
} else {
- buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound)
}
}
@@ -611,12 +598,12 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
}
partitionContext := schedulerContext.GetPartitionWithoutClusterID(partition)
if partitionContext == nil {
- buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound)
return
}
queue := partitionContext.GetQueue(queueName)
if queue == nil {
- buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusNotFound)
return
}
@@ -647,17 +634,17 @@ func getApplication(w http.ResponseWriter, r *http.Request) {
}
partitionContext := schedulerContext.GetPartitionWithoutClusterID(partition)
if partitionContext == nil {
- buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, PartitionDoesNotExists, http.StatusNotFound)
return
}
queue := partitionContext.GetQueue(queueName)
if queue == nil {
- buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, QueueDoesNotExists, http.StatusNotFound)
return
}
app := queue.GetApplication(application)
if app == nil {
- buildJSONErrorResponse(w, ApplicationDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, ApplicationDoesNotExists, http.StatusNotFound)
return
}
appDao := getApplicationDAO(app)
@@ -830,7 +817,7 @@ func getMetrics(w http.ResponseWriter, r *http.Request) {
promhttp.Handler().ServeHTTP(w, r)
}
-func getUsersResourceUsage(w http.ResponseWriter, r *http.Request) {
+func getUsersResourceUsage(w http.ResponseWriter, _ *http.Request) {
writeHeaders(w)
userManager := ugm.GetUserManager()
usersResources := userManager.GetUsersResources()
@@ -857,7 +844,7 @@ func getUserResourceUsage(w http.ResponseWriter, r *http.Request) {
}
userTracker := ugm.GetUserManager().GetUserTracker(user)
if userTracker == nil {
- buildJSONErrorResponse(w, UserDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, UserDoesNotExists, http.StatusNotFound)
return
}
var result = userTracker.GetUserResourceUsageDAOInfo()
@@ -893,7 +880,7 @@ func getGroupResourceUsage(w http.ResponseWriter, r *http.Request) {
}
groupTracker := ugm.GetUserManager().GetGroupTracker(group)
if groupTracker == nil {
- buildJSONErrorResponse(w, GroupDoesNotExists, http.StatusBadRequest)
+ buildJSONErrorResponse(w, GroupDoesNotExists, http.StatusNotFound)
return
}
var result = groupTracker.GetGroupResourceUsageDAOInfo()
@@ -906,7 +893,7 @@ func getEvents(w http.ResponseWriter, r *http.Request) {
writeHeaders(w)
eventSystem := events.GetEventSystem()
if !eventSystem.IsEventTrackingEnabled() {
- buildJSONErrorResponse(w, "Event tracking is disabled", http.StatusBadRequest)
+ buildJSONErrorResponse(w, "Event tracking is disabled", http.StatusInternalServerError)
return
}
diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go
index d67c63e0..0d976687 100644
--- a/pkg/webservice/handlers_test.go
+++ b/pkg/webservice/handlers_test.go
@@ -386,9 +386,9 @@ func TestApplicationHistory(t *testing.T) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusNotImplemented, resp.statusCode, "app history handler returned wrong status")
+ assert.Equal(t, http.StatusInternalServerError, resp.statusCode, "app history handler returned wrong status")
assert.Equal(t, errInfo.Message, "Internal metrics collection is not enabled.", jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusNotImplemented)
+ assert.Equal(t, errInfo.StatusCode, http.StatusInternalServerError)
// init should return null and thus no records
imHistory = history.NewInternalMetricsHistory(5)
@@ -441,9 +441,9 @@ func TestContainerHistory(t *testing.T) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusNotImplemented, resp.statusCode, "container history handler returned wrong status")
+ assert.Equal(t, http.StatusInternalServerError, resp.statusCode, "container history handler returned wrong status")
assert.Equal(t, errInfo.Message, "Internal metrics collection is not enabled.", jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusNotImplemented)
+ assert.Equal(t, errInfo.StatusCode, http.StatusInternalServerError)
// init should return null and thus no records
imHistory = history.NewInternalMetricsHistory(5)
@@ -550,25 +550,6 @@ func TestGetConfigJSON(t *testing.T) {
configs.SetConfigMap(map[string]string{})
}
-func TestBuildUpdateResponseSuccess(t *testing.T) {
- resp := &MockResponseWriter{}
- buildUpdateResponse(nil, resp)
- assert.Equal(t, http.StatusOK, resp.statusCode, "Response should be OK")
-}
-
-func TestBuildUpdateResponseFailure(t *testing.T) {
- resp := &MockResponseWriter{}
- err := fmt.Errorf("ConfigMapUpdate failed")
- buildUpdateResponse(err, resp)
-
- var errInfo dao.YAPIError
- err1 := json.Unmarshal(resp.outputBytes, &errInfo)
- assert.NilError(t, err1, unmarshalError)
- assert.Equal(t, http.StatusConflict, resp.statusCode, "Status code is wrong")
- assert.Assert(t, strings.Contains(string(errInfo.Message), err.Error()), "Error message should contain the reason")
- assert.Equal(t, errInfo.StatusCode, http.StatusConflict)
-}
-
func TestGetClusterUtilJSON(t *testing.T) {
setup(t, configDefault, 1)
@@ -1308,36 +1289,36 @@ func assertPartitionExists(t *testing.T, resp *MockResponseWriter) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError)
+ assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError)
assert.Equal(t, errInfo.Message, PartitionDoesNotExists, jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest)
+ assert.Equal(t, errInfo.StatusCode, http.StatusNotFound)
}
func assertQueueExists(t *testing.T, resp *MockResponseWriter) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError)
+ assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError)
assert.Equal(t, errInfo.Message, QueueDoesNotExists, jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest)
+ assert.Equal(t, errInfo.StatusCode, http.StatusNotFound)
}
func assertApplicationExists(t *testing.T, resp *MockResponseWriter) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError)
+ assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError)
assert.Equal(t, errInfo.Message, ApplicationDoesNotExists, jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest)
+ assert.Equal(t, errInfo.StatusCode, http.StatusNotFound)
}
func assertUserExists(t *testing.T, resp *MockResponseWriter) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError)
+ assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError)
assert.Equal(t, errInfo.Message, UserDoesNotExists, jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest)
+ assert.Equal(t, errInfo.StatusCode, http.StatusNotFound)
}
func assertUserNameExists(t *testing.T, resp *MockResponseWriter) {
@@ -1353,9 +1334,9 @@ func assertGroupExists(t *testing.T, resp *MockResponseWriter) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError)
+ assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError)
assert.Equal(t, errInfo.Message, GroupDoesNotExists, jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest)
+ assert.Equal(t, errInfo.StatusCode, http.StatusNotFound)
}
func assertGroupNameExists(t *testing.T, resp *MockResponseWriter) {
@@ -1371,9 +1352,9 @@ func assertNodeIDExists(t *testing.T, resp *MockResponseWriter) {
var errInfo dao.YAPIError
err := json.Unmarshal(resp.outputBytes, &errInfo)
assert.NilError(t, err, unmarshalError)
- assert.Equal(t, http.StatusBadRequest, resp.statusCode, statusCodeError)
+ assert.Equal(t, http.StatusNotFound, resp.statusCode, statusCodeError)
assert.Equal(t, errInfo.Message, NodeDoesNotExists, jsonMessageError)
- assert.Equal(t, errInfo.StatusCode, http.StatusBadRequest)
+ assert.Equal(t, errInfo.StatusCode, http.StatusNotFound)
}
func TestValidateQueue(t *testing.T) {
@@ -1561,7 +1542,7 @@ func TestGetEventsWhenTrackingDisabled(t *testing.T) {
req, err := http.NewRequest("GET", "/ws/v1/events/batch", strings.NewReader(""))
assert.NilError(t, err)
- readIllegalRequest(t, req, "Event tracking is disabled")
+ readIllegalRequest(t, req, http.StatusInternalServerError, "Event tracking is disabled")
}
func addEvents(t *testing.T) (appEvent, nodeEvent, queueEvent *si.EventRecord) {
@@ -1627,15 +1608,15 @@ func checkIllegalBatchRequest(t *testing.T, query, msg string) {
t.Helper()
req, err := http.NewRequest("GET", "/ws/v1/events/batch?"+query, strings.NewReader(""))
assert.NilError(t, err)
- readIllegalRequest(t, req, msg)
+ readIllegalRequest(t, req, http.StatusBadRequest, msg)
}
-func readIllegalRequest(t *testing.T, req *http.Request, errMsg string) {
+func readIllegalRequest(t *testing.T, req *http.Request, statusCode int, errMsg string) {
t.Helper()
rr := httptest.NewRecorder()
handler := http.HandlerFunc(getEvents)
handler.ServeHTTP(rr, req)
- assert.Equal(t, http.StatusBadRequest, rr.Code)
+ assert.Equal(t, statusCode, rr.Code)
jsonBytes := make([]byte, 256)
n, err := rr.Body.Read(jsonBytes)
assert.NilError(t, err, "cannot read response body")
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@yunikorn.apache.org
For additional commands, e-mail: issues-help@yunikorn.apache.org