8000 fix API `/_admin/cluster/removeServer` (#13485) · tylde/arangodb@d770534 · GitHub 8000
[go: up one dir, main page]

Skip to content

Commit d770534

Browse files
authored
fix API /_admin/cluster/removeServer (arangodb#13485)
1 parent 5c31daa commit d770534

File tree

10 files changed

+240
-2
lines changed

10 files changed

+240
-2
lines changed

CHANGELOG

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
devel
22
-----
33

4+
* Fix `/_admin/cluster/removeServer` API.
5+
This often returned HTTP 500 with an error message "Need open Array" due to
6+
an internal error when setting up agency preconditions.
7+
48
* Remove logging startup options `--log.api-enabled` and `--log.keep-logrotate`
59
for all client tools (arangosh, arangodump, arangorestore etc.), as these
610
options are only meaningful for arangod.

arangod/Agency/AsyncAgencyComm.h

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
///
2121
/// @author Lars Maier
2222
////////////////////////////////////////////////////////////////////////////////
23+
2324
#ifndef ARANGOD_CLUSTER_ASYNC_AGENCY_COMM_H
2425
#define ARANGOD_CLUSTER_ASYNC_AGENCY_COMM_H 1
2526

@@ -37,6 +38,7 @@
3738

3839
#include "Agency/PathComponent.h"
3940
#include "Basics/ResultT.h"
41+
#include "Basics/debugging.h"
4042
#include "Futures/Future.h"
4143
#include "Network/Methods.h"
4244

@@ -52,20 +54,33 @@ struct AsyncAgencyCommResult {
5254

5355
[[nodiscard]] bool fail() const { return !ok(); }
5456

55-
VPackSlice slice() const { return response->slice(); }
57+
VPackSlice slice() const {
58+
TRI_ASSERT(response != nullptr);
59+
return response->slice();
60+
}
5661

5762
std::shared_ptr<velocypack::Buffer<uint8_t>> copyPayload() const {
63+
TRI_ASSERT(response != nullptr);
5864
return response->copyPayload();
5965
}
66+
6067
std::shared_ptr<velocypack::Buffer<uint8_t>> stealPayload() const {
68+
TRI_ASSERT(response != nullptr);
6169
return response->stealPayload();
6270
}
71+
6372
std::string payloadAsString() const {
73+
TRI_ASSERT(response != nullptr);
6474
return response->payloadAsString();
6575
}
66-
std::size_t payloadSize() const { return response->payloadSize(); }
76+
77+
std::size_t payloadSize() const {
78+
TRI_ASSERT(response != nullptr);
79+
return response->payloadSize();
80+
}
6781

6882
arangodb::fuerte::StatusCode statusCode() const {
83+
TRI_ASSERT(response != nullptr);
6984
return response->statusCode();
7085
}
7186

arangod/Agency/TransactionBuilder.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ struct envelope {
127127
private:
128128
friend write_trx;
129129
precs_trx(builder_ptr b) : _builder(std::move(b)) {
130+
_builder->close();
130131
_builder->openObject();
131132
}
132133
builder_ptr _builder;

arangod/RestHandler/RestAdminClusterHandler.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -415,13 +415,22 @@ RestAdminClusterHandler::FutureVoid RestAdminClusterHandler::tryDeleteServer(
415415
resetResponse(rest::ResponseCode::OK);
416416
return futures::makeFuture();
417417
} else if (result.statusCode() == fuerte::StatusPreconditionFailed) {
418+
TRI_IF_FAILURE("removeServer::noRetry") {
419+
generateError(result.asResult());
420+
return futures::makeFuture();
421+
}
418422
return retryTryDeleteServer(std::move(ctx));
419423
}
420424
}
421425
generateError(result.asResult());
422426
return futures::makeFuture();
423427
});
424428
}
429+
430+
TRI_IF_FAILURE("removeServer::noRetry") {
431+
generateError(Result(TRI_ERROR_HTTP_PRECONDITION_FAILED));
432+
return futures::makeFuture();
433+
}
425434

426435
return retryTryDeleteServer(std::move(ctx));
427436
} else {

js/common/bootstrap/errors.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,10 @@
4848
"ERROR_HTTP_NOT_FOUND" : { "code" : 404, "message" : "not found" },
4949
"ERROR_HTTP_METHOD_NOT_ALLOWED" : { "code" : 405, "message" : "method not supported" },
5050
"ERROR_HTTP_NOT_ACCEPTABLE" : { "code" : 406, "message" : "request not acceptable" },
51+
"ERROR_HTTP_CONFLICT" : { "code" : 409, "message" : "conflict" },
5152
"ERROR_HTTP_PRECONDITION_FAILED" : { "code" : 412, "message" : "precondition failed" },
5253
"ERROR_HTTP_SERVER_ERROR" : { "code" : 500, "message" : "internal server error" },
54+
"ERROR_HTTP_NOT_IMPLEMENTED" : { "code" : 501, "message" : "not implemented" },
5355
"ERROR_HTTP_SERVICE_UNAVAILABLE" : { "code" : 503, "message" : "service unavailable" },
5456
"ERROR_HTTP_GATEWAY_TIMEOUT" : { "code" : 504, "message" : "gateway timeout" },
5557
"ERROR_HTTP_CORRUPTED_JSON" : { "code" : 600, "message" : "invalid JSON object" },

lib/Basics/errors.dat

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,10 @@ ERROR_HTTP_FORBIDDEN,403,"forbidden","Will be raised when the operation is forbi
4747
ERROR_HTTP_NOT_FOUND,404,"not found","Will be raised when an URI is unknown."
4848
ERROR_HTTP_METHOD_NOT_ALLOWED,405,"method not supported","Will be raised when an unsupported HTTP method is used for an operation."
4949
ERROR_HTTP_NOT_ACCEPTABLE,406,"request not acceptable","Will be raised when an unsupported HTTP content type is used for an operation, or if a request is not acceptable for a leader or follower."
50+
ERROR_HTTP_CONFLICT,409,"conflict","Will be raised when a conflict occurs in an HTTP operation."
5051
ERROR_HTTP_PRECONDITION_FAILED,412,"precondition failed","Will be raised when a precondition for an HTTP request is not met."
5152
ERROR_HTTP_SERVER_ERROR,500,"internal server error","Will be raised when an internal server is encountered."
53+
ERROR_HTTP_NOT_IMPLEMENTED,501,"not implemented","Will be raised when an API is called this is not implemented in general, or not implemented for the current setup."
5254
ERROR_HTTP_SERVICE_UNAVAILABLE,503,"service unavailable","Will be raised when a service is temporarily unavailable."
5355
ERROR_HTTP_GATEWAY_TIMEOUT,504,"gateway timeout","Will be raised when a service contacted by ArangoDB does not respond in a timely manner."
5456

lib/Basics/voc-errors.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,10 @@ void TRI_InitializeErrorMessages() {
4848
REG_ERROR(ERROR_HTTP_NOT_FOUND, "not found");
4949
REG_ERROR(ERROR_HTTP_METHOD_NOT_ALLOWED, "method not supported");
5050
REG_ERROR(ERROR_HTTP_NOT_ACCEPTABLE, "request not acceptable");
51+
REG_ERROR(ERROR_HTTP_CONFLICT, "conflict");
5152
REG_ERROR(ERROR_HTTP_PRECONDITION_FAILED, "precondition failed");
5253
REG_ERROR(ERROR_HTTP_SERVER_ERROR, "internal server error");
54+
REG_ERROR(ERROR_HTTP_NOT_IMPLEMENTED, "not implemented");
5355
REG_ERROR(ERROR_HTTP_SERVICE_UNAVAILABLE, "service unavailable");
5456
REG_ERROR(ERROR_HTTP_GATEWAY_TIMEOUT, "gateway timeout");
5557
REG_ERROR(ERROR_HTTP_CORRUPTED_JSON, "invalid JSON object");

lib/Basics/voc-errors.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,11 @@ constexpr int TRI_ERROR_HTTP_METHOD_NOT_ALLOWED
210210
/// operation, or if a request is not acceptable for a leader or follower.
211211
constexpr int TRI_ERROR_HTTP_NOT_ACCEPTABLE = 406;
212212

213+
/// 409: ERROR_HTTP_CONFLICT
214+
/// "conflict"
215+
/// Will be raised when a conflict occurs in an HTTP operation.
216+
constexpr int TRI_ERROR_HTTP_CONFLICT = 409;
217+
213218
/// 412: ERROR_HTTP_PRECONDITION_FAILED
214219
/// "precondition failed"
215220
/// Will be raised when a precondition for an HTTP request is not met.
@@ -220,6 +225,12 @@ constexpr int TRI_ERROR_HTTP_PRECONDITION_FAILED
220225
/// Will be raised when an internal server is encountered.
221226
constexpr int TRI_ERROR_HTTP_SERVER_ERROR = 500;
222227

228+
/// 501: ERROR_HTTP_NOT_IMPLEMENTED
229+
/// "not implemented"
230+
/// Will be raised when an API is called this is not implemented in general, or
231+
/// not implemented for the current setup.
232+
constexpr int TRI_ERROR_HTTP_NOT_IMPLEMENTED = 501;
233+
223234
/// 503: ERROR_HTTP_SERVICE_UNAVAILABLE
224235
/// "service unavailable"
225236
/// Will be raised when a service is temporarily unavailable.

lib/Rest/GeneralResponse.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,7 @@ rest::ResponseCode GeneralResponse::responseCode(int code) {
422422
case TRI_ERROR_CLUSTER_FOLLOWER_TRANSACTION_COMMIT_PERFORMED:
423423
return ResponseCode::CONFLICT;
424424

425+
case TRI_ERROR_HTTP_PRECONDITION_FAILED:
425426
case TRI_ERROR_CLUSTER_CREATE_COLLECTION_PRECONDITION_FAILED:
426427
return ResponseCode::PRECONDITION_FAILED;
427428

@@ -445,6 +446,7 @@ rest::ResponseCode GeneralResponse::responseCode(int code) {
445446
case TRI_ERROR_CLUSTER_CONNECTION_LOST:
446447
return ResponseCode::SERVICE_UNAVAILABLE;
447448

449+
case TRI_ERROR_HTTP_NOT_IMPLEMENTED:
448450
case TRI_ERROR_CLUSTER_UNSUPPORTED:
449451
case TRI_ERROR_NOT_IMPLEMENTED:
450452
case TRI_ERROR_ONLY_ENTERPRISE:
Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
/* jshint globalstrict:false, strict:false, maxlen: 200 */
2+
/* global fail, assertEqual, assertMatch, assertTrue */
3+
4+
// //////////////////////////////////////////////////////////////////////////////
5+
// / DISCLAIMER
6+
// /
7+
// / Copyright 2018 ArangoDB GmbH, Cologne, Germany
8+
// /
9+
// / Licensed under the Apache License, Version 2.0 (the "License")
10+
// / you may not use this file except in compliance with the License.
11+
// / You may obtain a copy of the License at
12+
// /
13+
// / http://www.apache.org/licenses/LICENSE-2.0
14+
// /
15+
// / Unless required by applicable law or agreed to in writing, software
16+
// / distributed under the License is distributed on an "AS IS" BASIS,
17+
// / WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
18+
// / See the License for the specific language governing permissions and
19+
// / limitations under the License.
20+
// /
21+
// / Copyright holder is triAGENS GmbH, Cologne, Germany
22+
// /
23+
// / @author Jan Steemann
24+
// //////////////////////////////////////////////////////////////////////////////
25+
26+
const jsunity = require('jsunity');
27+
const { getEndpointById, getEndpointsByType, getServersByType } = require('@arangodb/test-helper');
28+
const request = require('@arangodb/request');
29+
30+
function endpointToURL(endpoint) {
31+
if (endpoint.substr(0, 6) === 'ssl://') {
32+
return 'https://' + endpoint.substr(6);
33+
}
34+
let pos = endpoint.indexOf('://');
35+
if (pos === -1) {
36+
return 'http://' + endpoint;
37+
}
38+
return 'http' + endpoint.substr(pos);
39+
}
40+
41+
/// @brief set failure point
42+
function debugCanUseFailAt(endpoint) {
43+
let res = request.get({
44+
url: endpoint + '/_admin/debug/failat',
45+
});
46+
return res.status === 200;
47+
}
48+
49+
/// @brief set failure point
50+
function debugSetFailAt(endpoint, failAt) {
51+
let res = request.put({
52+
url: endpoint + '/_admin/debug/failat/' + failAt,
53+
body: ""
54+
});
55+
if (res.status !== 200) {
56+
throw "Error setting failure point";
57+
}
58+
}
59+
60+
function debugClearFailAt(endpoint) {
61+
let res = request.delete({
62+
url: endpoint + '/_admin/debug/failat',
63+
body: ""
64+
});
65+
if (res.status !== 200) {
66+
throw "Error removing failure points";
67+
}
68+
}
69+
70+
function adminClusterSuite() {
71+
'use strict';
72+
73+
return {
74+
75+
setUp: function () {
76+
getEndpointsByType("coordinator").forEach((ep) => debugClearFailAt(ep));
77+
},
78+
79+
tearDown: function () {
80+
getEndpointsByType("coordinator").forEach((ep) => debugClearFailAt(ep));
81+
},
82+
83+
testRemoveServerNonExisting: function () {
84+
let coords = getEndpointsByType('coordinator');
85+
assertTrue(coords.length > 0);
86+
87+
// this is assumed to be an invalid server id, so the operation must fail
88+
let res = request.post({ url: coords[0] + "/_admin/cluster/removeServer", body: "testmann123456", json: true });
89+
assertEqual(404, res.status);
90+
},
91+
92+
testRemoveNonFailedCoordinatorString: function () {
93+
let coords = getServersByType('coordinator');
94+
assertTrue(coords.length > 0);
95+
96+
let coordinatorId = coords[0].id;
97+
let ep = coords[0].endpoint;
98+
// make removeServer fail quickly in case precondition is not met. if we don't set this, it will cycle for 60s
99+
debugSetFailAt(endpointToURL(ep), "removeServer::noRetry");
100+
let res = request.post({ url: endpointToURL(ep) + "/_admin/cluster/removeServer", body: coordinatorId, json: true });
101+
// as the coordinator is still in use, we expect "precondition failed"
102+
assertEqual(412, res.status);
103+
},
104+
105+
testRemoveNonFailedCoordinatorObject: function () {
106+
let coords = getServersByType('coordinator');
107+
assertTrue(coords.length > 0);
108+
109+
let coordinatorId = coords[0].id;
110+
let ep = coords[0].endpoint;
111+
// make removeServer fail quickly in case precondition is not met. if we don't set this, it will cycle for 60s
112+
debugSetFailAt(endpointToURL(ep), "removeServer::noRetry");
113+
let res = request.post({ url: endpointToURL(ep) + "/_admin/cluster/removeServer", body: { server: coordinatorId }, json: true });
114+
// as the coordinator is still in use, we expect "precondition failed"
115+
assertEqual(412, res.status);
116+
},
117+
118+
testRemoveNonFailedDBServersString: function () {
119+
let coords = getServersByType('coordinator');
120+
assertTrue(coords.length > 0);
121+
122+
let coordinatorId = coords[0].id;
123+
let ep = coords[0].endpoint;
124+
// make removeServer fail quickly in case precondition is not met. if we don't set this, it will cycle for 60s
125+
debugSetFailAt(endpointToURL(ep), "removeServer::noRetry");
126+
127+
let dbservers = getServersByType('dbserver');
128+
assertTrue(dbservers.length > 0);
129+
dbservers.forEach((dbs) => {
130+
let res = request.post({ url: endpointToURL(ep) + "/_admin/cluster/removeServer", body: dbs.id, json: true });
131+
// as the coordinator is still in use, we expect "precondition failed"
132+
assertEqual(412, res.status);
133+
});
134+
},
135+
136+
testRemoveNonFailedDBServersObject: function () {
137+
let coords = getServersByType('coordinator');
138+
assertTrue(coords.length > 0);
139+
140+
let coordinatorId = coords[0].id;
141+
let ep = coords[0].endpoint;
142+
// make removeServer fail quickly in case precondition is not met. if we don't set this, it will cycle for 60s
143+
debugSetFailAt(endpointToURL(ep), "removeServer::noRetry");
144+
145+
let dbservers = getServersByType('dbserver');
146+
assertTrue(dbservers.length > 0);
147+
dbservers.forEach((dbs) => {
148+
let res = request.post({ url: endpointToURL(ep) + "/_admin/cluster/removeServer", body: { server: dbs.id }, json: true });
149+
// as the coordinator is still in use, we expect "precondition failed"
150+
assertEqual(412, res.status);
151+
});
152+
},
153+
154+
testCleanoutServerStringNonExisting: function () {
155+
let coords = getServersByType('coordinator');
156+
assertTrue(coords.length > 0);
157+
158+
let coordinatorId = coords[0].id;
159+
let ep = coords[0].endpoint;
160+
161+
// this is assumed to be an invalid server id, so the operation must fail
162+
let res = request.post({ url: endpointToURL(ep) + "/_admin/cluster/cleanOutServer", body: "testmann123456", json: true });
163+
// the server expects an object with a "server" attribute, but no string
164+
assertEqual(400, res.status);
165+
},
166+
167+
testCleanoutServerObjectNonExisting: function () {
168+
let coords = getServersByType('coordinator');
169+
assertTrue(coords.length > 0);
170+
171+
let coordinatorId = coords[0].id;
172+
let ep = coords[0].endpoint;
173+
174+
// this is assumed to be an invalid server id, so the operation must fail
175+
let res = request.post({ url: endpointToURL(ep) + "/_admin/cluster/cleanOutServer", body: { server: "testmann123456" }, json: true });
176+
assertEqual(202, res.status);
177+
assertTrue(res.json.hasOwnProperty("id"));
178+
assertEqual("string", typeof res.json 57AE .id);
179+
assertMatch(/^\d+/, res.json.id);
180+
},
181+
182+
};
183+
}
184+
185+
let ep = getEndpointsByType('coordinator');
186+
if (ep.length && debugCanUseFailAt(ep[0])) {
187+
// only run when failure tests are available
188+
jsunity.run(adminClusterSuite);
189+
}
190+
return jsunity.done();

0 commit comments

Comments
 (0)
0