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

Add `POST /_node/$node/_restart` endpoint #1543

Merged
merged 6 commits into from Aug 16, 2018

Conversation

Projects
None yet
3 participants
@iilyak
Contributor

iilyak commented Aug 7, 2018

Overview

We need to be able to restart CouchDB from integration test suite.
We used to have this feature, but it was removed.
This PR brings this functionality back under /_node/$node/_restart.

Testing recommendations

  1. compile and start CouchDB using dev/run
make && dev/run --admin=adm:pass
  1. try to access new endpoint as non authenticated user (you shouldn't be able to)
curl -s -X POST 127.0.0.1:35984/_node/node1@127.0.0.1/_restart
  1. note number of ets tables on the node
 curl -s -u adm:pass -X GET 127.0.0.1:35984/_node/node1@127.0.0.1/_system | jq '.ets_table_count'
  1. trigger restart of the node (the call should succeed)
curl -s -u adm:pass -X POST 127.0.0.1:35984/_node/node1@127.0.0.1/_restart
  1. keep checking the number of ets tables on the node. The call should return null few times and then it should increase gradually.
 curl -s -u adm:pass -X GET 127.0.0.1:35984/_node/node1@127.0.0.1/_system | jq '.ets_table_count'

Related Issues or Pull Requests

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@wohali

This comment has been minimized.

Member

wohali commented Aug 9, 2018

I would recommend removing the _restart handler from rel/overlay/etc/default.ini while you're at it, the code in couch_httpd_misc_handlers:handle_restart_req, and for the win, update the JS test suite to use this instead of the printf "restart" approach.

Once you do that, we can completely remove the stdout handling code from dev/run in #1406.

FYI @iilyak if you create these branches on the main apache/couchdb repo instead of in the cloudant/couchdb one, I can check in changes on your branches to help you, rather than asking you to do all the work :) Otherwise, I'd have to issue PRs against cloudant/couchdb otherwise, and that feels clunky.

@wohali

This comment has been minimized.

Member

wohali commented Aug 9, 2018

Here's the code in the js suite that would need to be changed:

https://github.com/apache/couchdb/blob/master/test/javascript/test_setup.js#L86
https://github.com/apache/couchdb/blob/master/test/javascript/run#L88-L92

Should be a snap to replace the first with a call to CouchDB.request() (see https://github.com/apache/couchdb/blob/master/test/javascript/test_setup.js#L73-L77 for an example) and the second just needs the special handling removed.

Doing this would also let you confirm that the endpoint you're adding works with an actual test case or two. 😉

@iilyak

This comment has been minimized.

Contributor

iilyak commented Aug 14, 2018

I want to provide a status update.

There is a slight bump on the way to update JS test suite to use new endpoint.
When _restart is invoked it reinitialize all of the OTP applications, but it doesn't restart beam itself (same approach as in couchdb 1x). This means that uptime is not updated. The uptime returned from _system is used to check that the restart is completed.

I am working on finding the solution.

@iilyak iilyak force-pushed the cloudant:implement-node-restart branch from 9abf607 to d3b5339 Aug 15, 2018

@iilyak

This comment has been minimized.

Contributor

iilyak commented Aug 15, 2018

@wohali I pushed a commit to remove _restart from default.ini however I am not sure if we need to follow a protocol and send a proposal via ML in this case. We are not deprecating the endpoint we are changing a default. I am considering to revert that commit for now.

@wohali

This comment has been minimized.

Member

wohali commented Aug 15, 2018

@iilyak You're re-introducting a new endpoint, so that's fine. You removed the node-local one, right? I'm not sure it actually worked correctly in 2.0 so I don't personally consider that a deprecation.

@jaydoane

This comment has been minimized.

Contributor

jaydoane commented Aug 15, 2018

This LGTM. Tested as instructed:

$ curl localhost:35984/_node/node1@127.0.0.1/_restart
{"error":"unauthorized","reason":"You are not a server admin."}

$ curl -s -u adm:pass -X GET 127.0.0.1:35984/_node/node1@127.0.0.1/_system | jq '.ets_table_count'
115

$ curl -s -u adm:pass -X POST 127.0.0.1:35984/_node/node1@127.0.0.1/_restart
{"ok":true}

$ curl -s -u adm:pass -X GET 127.0.0.1:35984/_node/node1@127.0.0.1/_system | jq '.ets_table_count'
108

I also confirmed uptimes increment correctly on restarted node:

(node1@127.0.0.1)1> couch_app:uptime().
10038
(node1@127.0.0.1)2> couch_app:uptime().
11332

@iilyak iilyak force-pushed the cloudant:implement-node-restart branch from 7133eff to ee308fa Aug 15, 2018

@iilyak

This comment has been minimized.

Contributor

iilyak commented Aug 15, 2018

@wohali You removed the node-local one, right?

Yes I did remove it.

@iilyak iilyak force-pushed the cloudant:implement-node-restart branch from ee308fa to 9be1213 Aug 15, 2018

@iilyak

This comment has been minimized.

Contributor

iilyak commented Aug 16, 2018

@wohali Is there anything else I should do? Are you planing to do more reviews or I can merge it?

@wohali

This comment has been minimized.

Member

wohali commented Aug 16, 2018

@iilyak I'd prefer it if someone with more Erlang experience than I could do a review, but I have no objection at this point. Thanks!

@iilyak

This comment has been minimized.

Contributor

iilyak commented Aug 16, 2018

There is a +1 from @jaydoane

@iilyak iilyak merged commit b847adb into apache:master Aug 16, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@iilyak iilyak deleted the cloudant:implement-node-restart branch Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment