connect: Try to re-connect to proxy #54

dvoytik · 2017-08-01T16:30:49Z

Try to re-connect to proxy if connection is lost (e.g. proxy died)
with RECONNECT_TIMEOUT_S timeout.

This patch is needed to implement high availability of cc-proxy.

Fixes #53.

Signed-off-by: Dmitry Voytik dmitry.voytik@huawei.com

amshinde · 2017-08-03T17:46:58Z

While we try to reconnect to proxy on errors while reading from proxy socket, I think we should do the same for failures to write to the proxy socket.

While this is a good start for ensuring high availability, I would like to mention that we need to keep track of messages for which the shim has not received any acknowledgement from the proxy and attempt to resend them. Right now the shim does not keep track of the responses received from proxy for the messages that it sends. I will open a separate issue for that so that it can be tackled in a separate PR.

sboeuf · 2017-08-04T07:53:39Z

src/shim.c

@@ -703,6 +704,9 @@ handle_proxy_message(struct cc_shim *shim)

 	fr = read_frame(shim);
 	if ( !fr) {
+		if (! reconnect_to_proxy(shim)) {


You're making lots of assumption by calling this after you got a fr == NULL. Indeed, looking at read_frame() function, there are several reasons why we could get an error, and they are not all related to the proxy disconnection.

@sboeuf, thanks for the review.

Okay. Re-connection when header length or header version is wrong doesn't make much sense. If I change for these cases instead of returning false to abort()? Would it make sense for you?

If I understand you well, you want to change the return false with abort() in case you get header length or header version which is wrong ? Because you consider those cases like real issues and we should just terminate the shim, right ?

Sorry for the delayed response.

So read_frame() can return NULL in case when read_wire_data() fails --> we re-connect.
Also read_frame() return NULL in cases:
shim == NULL or shim->proxy_sock_fd < 0
fr->header.header_len < MIN_HEADER_WORD_SIZE
fr->header.version != PROXY_VERSION
~~My proposition is to handle these cases the same as failures with allocation --> calling abort(). Doing this way will make read_frame() return NULL only in cases when read_wire_date() failed.~~

UPDATE:
Maybe it's not the best idea to address this problem as part of this PR. For simplicity I reworked the patch to handle only the case when recv() returns 0 ("a stream socket peer has performed an orderly shutdown").

jodh-intel · 2017-08-07T13:42:33Z

src/shim.c

+		shim_error("Could not send connect command to proxy\n");
+		goto try_again;
+	}
+	add_pollfd(poll_fds, PROXY_SOCK_INDEX, shim->proxy_sock_fd,


Could you add a comment explaining why this is being called again for PROXY_SOCK_INDEX.

Okay. Will add a comment here. Thanks.

jodh-intel · 2017-08-07T13:44:28Z

src/shim.c

+	// How many seconds shim tries to reconnect to proxy
+	const int RECONNECT_TIMEOUT_S = 10;
+	// Period between each attempt in milliseconds
+	const int RECONNECT_POLL_MS = 333;


It might be useful if these two variables could be specified as getopt options, defaulting to #define'd values you're using here (as that would allow the defaults to be modified at build time and runtime).

Thanks for the review. Makes sense. Will redo the patch.

jodh-intel · 2017-08-07T13:46:49Z

src/shim.c

+	// Period between each attempt in milliseconds
+	const int RECONNECT_POLL_MS = 333;
+
+	shim_warning("Reconnecting to proxy\n");


Could you reference RECONNECT_TIMEOUT_S in this message so it is clear from the log how long the shim will wait before giving up. I appreciate that you are logging that on error but our "rule of thumb" is: "if the code is going to sleep / block, always add a log call before that action in case 'the impossible' happens" 😄

Got it. Will rework.

dvoytik · 2017-08-17T09:06:29Z

@sboeuf, I've updated the patch. Could you please review it. Thanks.

sboeuf · 2017-08-17T16:01:21Z

@dvoytik sure, I'll do that tomorrow.

sboeuf

A few comments, but the PR looks globally good to me.

sboeuf · 2017-08-21T10:31:05Z

src/shim.c

+
+// Period between attempts to reconnect to proxy in milliseconds
+#ifndef RECONNECT_POLL_MS
+#define RECONNECT_POLL_MS 333


Where does this value come from ? I think we should go with something more simple: 100, 200, 300, ...

I'll set it to 100.

Sounds good to me.

sboeuf · 2017-08-21T12:12:21Z

src/shim.c

@@ -954,6 +966,51 @@ connect_to_proxy(struct cc_shim *shim)
 	return false;
 }

+inline void sleep_ms(int ms)
+{
+	struct timespec ts = { 0, ms * 1000000L };


Might worth changing 1000000L with 1e6 for more readability.

gcc 6.4.1 complains, it seems 1e6 is a double literal. I'll replace it with:

struct timespec ts = { 0, ms * 1000 * 1000 };

Oh okay, then that's fine, don't change it.

sboeuf · 2017-08-21T12:17:50Z

src/shim.c

+			shim->timeout);
+	int time = 0;
+
+try_again:


Better to avoid using goto when possible for more clarity in the code. Use a loop instead:

do { ... } while ()`

Ok. I'll rework this piece.

sboeuf · 2017-08-21T12:29:44Z

src/shim.c

@@ -971,6 +1028,7 @@ print_usage(void) {
        printf("  -c,  --container-id   Container id\n");
        printf("  -d,  --debug          Enable debug output\n");
        printf("  -t,  --token          Connection token passed by cc-proxy\n");
+        printf("  -r,  --rtimeout       Reconnection timeout to cc-proxy in seconds\n");


About the long option name, I think you can go with something longer like reconnect-timeout

sboeuf · 2017-08-21T12:31:13Z

src/shim.c

+try_again:
+	sleep_ms(RECONNECT_POLL_MS);
+	time += RECONNECT_POLL_MS;
+	if (time >= shim->timeout * 1000) {


1e3 instead of 1000 sounds clearer to me.

Interesting. For me it's completely opposite :) Nevertheless, I'm not sure it's a good idea to mix double/float literals with integer variables.

sboeuf · 2017-08-21T12:33:15Z

src/shim.c

@@ -983,6 +1041,7 @@ main(int argc, char **argv)
 		.container_id   =  NULL,
 		.proxy_sock_fd  = -1,
 		.token          =  NULL,
+		.timeout = RECONNECT_TIMEOUT_S,


Please align this with other assignments.

sboeuf

Two tiny comments.
@jodh-intel @amshinde please take a look.

sboeuf · 2017-08-21T16:57:51Z

src/shim.c

+		}
+
+		shim_debug("Sending connect command\n");
+		if (! send_connect_command(shim)) {


Since that's the second place in the code where we use both connect_to_proxy() with send_connect_command() right after (which is logical), I think we could factorize this through a new function.

@dvoytik you have updated the PR, but I don't think you addressed this comment. Am I missing something ?

@dvoytik ping ?

sboeuf · 2017-08-21T16:58:33Z

src/shim.c

@@ -971,6 +1031,7 @@ print_usage(void) {
        printf("  -c,  --container-id   Container id\n");
        printf("  -d,  --debug          Enable debug output\n");
        printf("  -t,  --token          Connection token passed by cc-proxy\n");
+        printf("  -r,  --reconnect-timeout Reconnection timeout to cc-proxy in seconds\n");


Please align this with the rest of the output.

jodh-intel · 2017-08-30T15:40:21Z

There are a couple of minor issues:

I'd add the new option in alphabetical order everywhere in the code.
Lots of strings hard-coding cc-proxy (I'd consider #define'ing it to avoid the duplications and make maintenance easier).

But these are nits, so:

lgtm

jodh-intel · 2017-08-30T15:40:46Z

@amshinde - could you re-review?

dvoytik · 2017-08-31T08:28:53Z

@jodh-intel, sure, no problem. I've updated the PR according to your comments. Thanks.

sboeuf · 2017-09-01T07:43:14Z

@dvoytik in case you missed that in the old comments, you've missed one comment I left two weeks ago:

Since that's the second place in the code where we use both connect_to_proxy() with send_connect_command() right after (which is logical), I think we could factorize this through a new function.

Does that make sense ?

dvoytik · 2017-09-01T10:35:27Z

@sboeuf, sorry, indeed I missed that comment. Thanks for reminding. I've updated the PR.

jodh-intel · 2017-10-02T08:36:26Z

Hi @dvoytik - are you still planning to work on this?

dvoytik · 2017-10-02T12:41:27Z

@jodh-intel, this PR was ready to merge one month ago in my opinion. I've rebased it again on the latest master. If all tests pass then it should be merged. This PR is needed by clearcontainers/proxy#107

grahamwhaley · 2017-10-02T12:49:06Z

@sboeuf pls re-review - we'll need your status ack :-)

sboeuf · 2017-10-02T13:05:49Z

Yes this is fine. Let's wait for the CI on this PR and we can merge it.
LGTM

jodh-intel · 2017-10-06T10:41:45Z

This still needs an ack from a project approver. @sameo, @grahamwhaley, @amshinde - could you take a look?

jodh-intel · 2017-10-09T10:49:22Z

pingety-ping! 😄

grahamwhaley

lgtm
I'd like to hear from @amshinde though for final ack, as primary original author/owner of the proxy iirc

grahamwhaley · 2017-10-09T13:06:29Z

src/shim.c

@@ -955,6 +968,71 @@ connect_to_proxy(struct cc_shim *shim)
 }

 /*


nitpick: @jodh-intel do these need to be \*! comment starts in order to get the marked up comment notionally processed?

Yep - good catch.

Aside: I just ran doxygen and found that there are a couple of errors that need correcting for master too [ #82]).

grahamwhaley · 2017-10-09T13:10:39Z

nice work btw @dvoytik :-)

Try to reconnect to proxy if connection is lost (e.g. proxy died). Timeout is defined either during configure phase (--with-timeout) or by passing --reconnect-timeout command line parameter. This patch is needed to implement high availability of cc-proxy. Fixes clearcontainers#53. Signed-off-by: Dmitry Voytik <dmitry.voytik@huawei.com>

dvoytik · 2017-10-09T22:41:25Z

@grahamwhaley, thank you! :)

jodh-intel · 2017-10-10T13:20:32Z

Hi @amshinde - please can you review?

jodh-intel · 2017-10-10T16:08:17Z

@amshinde - I've tested this PR in combination with clearcontainers/proxy#107 and it works a treat! ;-)

amshinde · 2017-10-10T16:19:45Z

@jodh-intel thanks!
lgtm

dvoytik mentioned this pull request Aug 1, 2017

Implement store/restore of cc-proxy's state to/from disk. clearcontainers/proxy#96

Open

sboeuf changed the title ~~connect: Try to recconect to proxy~~ connect: Try to re-connect to proxy Aug 1, 2017

sboeuf self-requested a review August 1, 2017 16:57

dvoytik force-pushed the try_to_reconnect branch from 9a2d432 to 595724c Compare August 2, 2017 08:16

sboeuf suggested changes Aug 4, 2017

View reviewed changes

jodh-intel reviewed Aug 7, 2017

View reviewed changes

dvoytik force-pushed the try_to_reconnect branch from 595724c to bd82046 Compare August 11, 2017 16:04

sboeuf mentioned this pull request Aug 18, 2017

implement (re)store of proxy's state clearcontainers/proxy#107

Open

sboeuf reviewed Aug 21, 2017

View reviewed changes

dvoytik force-pushed the try_to_reconnect branch 2 times, most recently from 586c4da to 18213cd Compare August 21, 2017 15:05

sboeuf reviewed Aug 21, 2017

View reviewed changes

dvoytik force-pushed the try_to_reconnect branch 2 times, most recently from 0666c12 to cf3da3d Compare August 22, 2017 09:02

clearcontainers deleted a comment from clearcontainersbot Aug 23, 2017

dvoytik force-pushed the try_to_reconnect branch from cf3da3d to 0df68ba Compare August 31, 2017 08:27

dvoytik force-pushed the try_to_reconnect branch from 0df68ba to df6d2ca Compare September 1, 2017 10:28

dvoytik force-pushed the try_to_reconnect branch from df6d2ca to b8325e8 Compare October 2, 2017 12:38

sboeuf approved these changes Oct 2, 2017

View reviewed changes

jodh-intel mentioned this pull request Oct 6, 2017

Create a high availability design proposal doc clearcontainers/runtime#683

Open

grahamwhaley reviewed Oct 9, 2017

View reviewed changes

jodh-intel mentioned this pull request Oct 9, 2017

doxygen is reporting a couple of errors #82

Closed

dvoytik force-pushed the try_to_reconnect branch from b8325e8 to ec00fcf Compare October 9, 2017 22:38

amshinde merged commit ff07765 into clearcontainers:master Oct 10, 2017

sboeuf mentioned this pull request Oct 13, 2017

cc-proxy/cc-shim high availability clearcontainers/proxy#4

Open

		@@ -955,6 +968,71 @@ connect_to_proxy(struct cc_shim *shim)
		}

		/*

connect: Try to re-connect to proxy #54

connect: Try to re-connect to proxy #54

Conversation

dvoytik commented Aug 1, 2017 • edited by sboeuf Loading

amshinde commented Aug 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytik Aug 10, 2017 • edited Loading

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

dvoytik commented Aug 17, 2017

sboeuf commented Aug 17, 2017

sboeuf 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

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

sboeuf 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

jodh-intel commented Aug 30, 2017 • edited by grahamwhaley Loading

jodh-intel commented Aug 30, 2017

dvoytik commented Aug 31, 2017

sboeuf commented Sep 1, 2017

dvoytik commented Sep 1, 2017

jodh-intel commented Oct 2, 2017

dvoytik commented Oct 2, 2017 • edited Loading

grahamwhaley commented Oct 2, 2017

sboeuf commented Oct 2, 2017

jodh-intel commented Oct 6, 2017

jodh-intel commented Oct 9, 2017

grahamwhaley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamwhaley commented Oct 9, 2017

dvoytik commented Oct 9, 2017

jodh-intel commented Oct 10, 2017

jodh-intel commented Oct 10, 2017

amshinde commented Oct 10, 2017 • edited by grahamwhaley Loading

dvoytik commented Aug 1, 2017 •

edited by sboeuf

Loading

dvoytik Aug 10, 2017 •

edited

Loading

jodh-intel commented Aug 30, 2017 •

edited by grahamwhaley

Loading

dvoytik commented Oct 2, 2017 •

edited

Loading

amshinde commented Oct 10, 2017 •

edited by grahamwhaley

Loading