helm: Allow non-leader Orchestrator instances to accept requests #3665

derekperkins · 2018-02-20T00:07:07Z

Orchestrator 3.0.7 added a proxy that forwards master only requests, so we don’t have to workaround that by having perpetually unready pods via the /api/leader-check endpoint

cc @shlomi-noach @enisoc

derekperkins · 2018-02-20T00:11:11Z

I haven't been able to test this yet, AKS broke my cluster and won't let me create a new 1.9.x cluster yet.

derekperkins · 2018-02-20T00:12:04Z

Related to issue openark/orchestrator#245 and PR openark/orchestrator#408

shlomi-noach

LGTM

enisoc · 2018-02-20T22:09:52Z

helm/vitess/templates/_orchestrator.tpl

@@ -24,34 +24,6 @@ spec:

 ---

-###################################


We should also remove the serviceName from the StatefulSet.

Actually it looks like we have to set it to empty since it's required.

I actually changed it to the main orchestrator service, since that is replacing the old headless service. I also wonder if that will solve your port problem. I'd test, but AKS is still not working.

enisoc · 2018-02-20T22:15:11Z

helm/vitess/values.yaml

 # Default values for orchestrator resources
 orchestrator:
  enabled: false
-  image: "vitess/orchestrator:3.0.6"
+  image: "vitess/orchestrator:3.0.7"


Can you update the k8s/orchestrator Dockerfile to build 3.0.7?

3.0.8 is coming up very shortly with bugfixes to 3.0.7.

This should change to 3.0.9 now?

enisoc · 2018-02-20T22:32:19Z

I think I got it working, but I had to add port 3000 to the per-pod orchestrator services, since that's the port that the reverse proxy is using. Is there an orc config option to change the port that followers assume the leader is serving the API on? Or maybe we should just tell orchestrator to directly serve on 80?

shlomi-noach · 2018-02-21T03:35:30Z

Is there an orc config option to change the port that followers assume the leader is serving the API on?

The leader advertises to the followers on which port it is listening. If configured to "ListenAddress": ":12345" then that's the port the followers will use.

shlomi-noach · 2018-02-21T03:38:52Z

This computation:

https://github.com/github/orchestrator/blob/b092c26961a6d623ea75b8a923e38eb7361172ab/go/raft/raft.go#L84-L97

Is done independently on each node, once, upon startup, and then advertised to the followers any time the node turns to be the leader.

enisoc · 2018-02-21T21:07:12Z

From the code that @shlomi-noach linked, it looks like you can specify advertised host separately from the listen host, but you can't specify advertised port separately from the listen port. Because we're using k8s Services to route port 80 (on the Service) to port 3000 (on the Pod), our advertised and listen ports are different.

We could fix this by:

Asking orc to let us specify advertised port separately from listen port.
Changing our config to use the same port on the Service as the listen port (either 80 for both, or 3000 for both).
Adding port 3000 to the Service in addition to port 80, so both Service ports redirect to port 3000 on the Pod.

For testing, I did (3) since it's the quickest, but (2) is probably better in the long run for reducing confusion and complexity in our config. Although (1) sounds the most flexible, I'd rather do (2) even if orc adds that feature, since it simplifies our setup.

shlomi-noach · 2018-02-22T05:05:23Z

it looks like you can specify advertised host separately from the listen host

hat's for the raft endpoint, whereas:

but you can't specify advertised port separately from the listen port

that's about the HTTP endpoint.

You are correct in your analysis. I'm merely pointing out that the HTTP endpoint doesn't have a "listen vs. advertised" config in the first place.

enisoc · 2018-02-22T18:59:10Z

@shlomi-noach wrote:

I'm merely pointing out that the HTTP endpoint doesn't have a "listen vs. advertised" config in the first place.

If the RaftAdvertise host wasn't intended to apply to the HTTP endpoint, then isn't it technically incorrect for the reverse proxy to use that host when hitting the HTTP port? If RaftAdvertise won't play double duty, it seems necessary to either auto-detect the local address (in case ListenAddress leaves the host empty) or provide a separate HttpAdvertise setting.

That would actually be better for us, since HTTP traffic could bypass the per-Pod Service (which is a workaround to make Raft think our IPs are static) and go directly Pod-to-Pod. Of course, this assumes that followers will be advised of the latest Leader URI if an orc node has been restarted (in a replacement Pod) with the same RaftAdvertise host, but a different HttpAdvertise host.

shlomi-noach · 2018-02-24T19:15:36Z

I'm a bit confused about the question, so I'm going to answer what I did understand and how and why whatever works works the way it does. And hope fully you can point me back on track?

If the RaftAdvertise host wasn't intended to apply to the HTTP endpoint

Historically, when it was created, there was no reverse proxy, so "intended" or "not intended" are not the right words to use.

then isn't it technically incorrect for the reverse proxy to use that host when hitting the HTTP port?

Why it is incorrect? Say the host is 22.33.44.55 and RaftAdvertised = 77.88.66.55. The raft members will seek raft communication on 77.88.66.55:10008. The reverse proxy mechanism will seek the HTTP server on 77.88.66.55:XXXX, (assuming XXXX is the port in ListenAddr, and typically 3000).
Do you think reverse proxy should use 22.33.44.55:3000? I think not; if RaftAdvertised is specified, it's specified because the host is only visible to outsiders via some proxy/IP, and that IP (77.88.66.55 in our example) would hold true for :10008 as well as :3000. Is this not the case?

If RaftAdvertise won't play double duty

Sorry, I'm not a native English speaker, and such phrases always leave me uncertain of their meaning. What does it mean for RaftAdvertise to not play double duty?

it seems necessary to either auto-detect the local address

But the local address would not necessarily be visible to outsiders, right? So whatever the host self-resolves is irrelevant to remote spectators.

That would actually be better for us, since HTTP traffic could bypass the per-Pod Service (which is a workaround to make Raft think our IPs are static) and go directly Pod-to-Pod.

Apologies, I'm not sure what that it means to bypass the per-pod service, or to go directly pod-to-pod.

enisoc · 2018-02-26T19:30:57Z

if RaftAdvertised is specified, it's specified because the host is only visible to outsiders via some proxy/IP, and that IP (77.88.66.55 in our example) would hold true for :10008 as well as :3000. Is this not the case?

It's not the case for us currently, but I see now how that's unexpected. I shouldn't have used the phrase "technically incorrect" since we're the ones who are doing weird stuff. :)

If we were using RaftAdvertised because of a machine with multiple addresses, it would be appropriate to assume you could use the same address for any port. However, we are actually using RaftAdvertised because of reverse proxies (that's basically what a k8s Service is).

An example might look something like this:

The host address is 22.33.44.55 and the host only has that one IP.
We use a reverse proxy at 77.88.66.55 to forward the raft port (10008) and only the raft port.
We set RaftAdvertised to 77.88.66.55.
Trying to access port 3000 on the RaftAdvertised address fails because that port is not forwarded.

What does it mean for RaftAdvertise to not play double duty?

Sorry for the regional idioms :)

What I meant by "not play double duty" was the idea that RaftAdvertise applies only to Raft, and not to HTTP or any other port/protocol. If that's the case, I was proposing an equivalent setting for HTTP so we can set the advertised address explicitly.

But the local address would not necessarily be visible to outsiders, right? So whatever the host self-resolves is irrelevant to remote spectators.

Agreed. That's why I would prefer the explicit HttpAdvertise setting over trying to detect it automatically.

I'm not sure what that it means to bypass the per-pod service, or to go directly pod-to-pod.

Basically I'm saying it would be nice if HTTP traffic could bypass the reverse proxy we set up for Raft, because it's not necessary for HTTP. We only needed the reverse proxy for Raft in order to make Raft think our IPs are static.

shlomi-noach · 2018-02-27T08:46:34Z

OK, to me this reads like an orchestrator feature request to support an optional HttpAdvertise.

Assuming HttpAdvertise is configured by the user:

Does it makes sense for it to not have a hostname (can it be :8080)?
Does it makes sense for it to not have a port (can it be my-host-12345.com)?

enisoc · 2018-02-27T17:05:04Z

Does it makes sense for it to not have a hostname (can it be :8080)?

I guess it could make sense if the user only cares about changing the port, but where would you get the hostname from in that case? If the alternative to making hostname required is to take it from RaftAdvertise, I would prefer that it's required. Part of my confusion above was that I didn't expect a variable called RaftAdvertise to apply to the HTTP port.

Does it makes sense for it to not have a port (can it be my-host-12345.com)?

I think that would make sense.

shlomi-noach · 2018-03-06T09:51:06Z

Please see openark/orchestrator#430 for a HTTPAdvertise offering.

shlomi-noach · 2018-03-12T06:53:03Z

@enisoc @derekperkins HTTPAdvertise release in v3.0.9, https://github.com/github/orchestrator/releases/latest

derekperkins · 2018-03-13T11:44:21Z

@enisoc I updated Orchestrator to 3.0.9 and pushed the vitess/orchestrator:3.0.9 image.

As for the HTTPAdvertise feature, I'm not totally sure what you're wanting that configuration to look like. I added a commit that is likely using it incorrectly, but I figure that will make it easier to discuss the right config. I also wasn't sure if you wanted to open up a new port on the Orchestrator service. Once we figure that out, I'll overwrite that last commit and this should be good to merge.

derekperkins · 2018-03-13T21:14:07Z

Here is my Orchestrator setting:"HTTPAdvertise": "orchestrator-headless.vitess:80"
Resulting in this error: FATAL If specified, HTTPAdvertise must include host name

shlomi-noach

Found a spot to change orchestrator version to 3.0.9

shlomi-noach · 2018-03-14T04:35:55Z

helm/vitess/values.yaml

 # Default values for orchestrator resources
 orchestrator:
  enabled: false
-  image: "vitess/orchestrator:3.0.6"
+  image: "vitess/orchestrator:3.0.7"


This should change to 3.0.9 now?

sougou · 2018-05-06T14:07:42Z

Should this PR be merged or abandoned in favor of the operator?

derekperkins · 2018-05-10T00:40:31Z

@sougou It's worth figuring out regardless. I'm just not familiar enough with the reasons @shlomi-noach and @enisoc architected it to take this any further. It should be relatively simple to get merged once I know what to put into the HTTPAdvertise flag.

derekperkins · 2018-05-10T01:18:30Z

I just rebased on master, updated to Orchestrator 3.0.10 + pmm-client 1.10.0 and pushed the corresponding docker images.

shlomi-noach · 2018-05-10T04:55:30Z

I'm leaving HTTPAdvertise to @enisoc . I don't have the insights he does on k8s deployments.

enisoc · 2018-05-21T23:56:02Z

helm/vitess/templates/_orchestrator.tpl

@@ -60,7 +55,7 @@ kind: StatefulSet
 metadata:
  name: orchestrator
 spec:
-  serviceName: orchestrator-headless
+  serviceName: orchestrator


I think this needs to stay orchestrator-headless since (I believe) per-Pod DNS only works with headless services.

enisoc · 2018-05-21T23:58:17Z

helm/vitess/templates/_orchestrator-conf.tpl

@@ -46,6 +46,7 @@ data:
    "HostnameResolveMethod": "none",
    "HTTPAuthPassword": "",
    "HTTPAuthUser": "",
+    "HTTPAdvertise": "orchestrator-headless.{{ $namespace }}:80",


My goal was to set HTTPAdvertise to bypass any Service, and talk directly Pod-to-Pod. I think we can do that by referring here to the per-Pod DNS entries created in the headless service, so it should look something like:

POD_NAME.orchestrator-headless.{{ $namespace }}:3000

That makes more sense to me now.

I just tried it out and got this error
FATAL If specified, HTTPAdvertise must include host name

It didn't like not having http://. I'm waiting for my cluster to cycle down so I can test it again.
https://play.golang.org/p/fxedGpvILSf

https://github.com/github/orchestrator/blob/677a004d0374e03e78e87bca417e67f673927fa1/go/config/config.go#L566

Adding http seems to have done the trick

derekperkins · 2018-05-22T01:53:05Z

This is working perfectly for me now. Once I get a LGTM from you @enisoc, I'll rebase my changes and fix the DCO.

derekperkins · 2018-05-22T01:54:54Z

Other than fixing HTTPAdvertise, I was able to eliminate all the per-StatefulSet services in lieu of using the headless service DNS entries. I think we just added the services to get a persistent DNS record in the event that the pod moved around, but this should have the same effect.

enisoc · 2018-05-22T16:52:53Z

I thought the original reason for doing Service-per-Pod was that Orchestrator's Raft library required static IPs, not just static DNS. Is that still the case?

shlomi-noach · 2018-05-22T17:15:10Z

I thought the original reason for doing Service-per-Pod was that Orchestrator's Raft library required static IPs, not just static DNS. Is that still the case?

Unsure what static DNS is?

orchestrator's raft library does indeed require static IPs, at least externally facing (which is why RaftAdvertise exists), and nothing has changed in the past few months.

derekperkins · 2018-05-22T17:36:57Z

I forgot about that, my bad. I tested deleting a pod and it failed. I'll roll those changes back.
openark/orchestrator#253

derekperkins · 2018-05-22T19:45:41Z

@enisoc I just launched a test cluster and all is well. Is there anything else we need to do before merging this aside from me rebasing?

enisoc

LGTM after squash.

3.0.7 added a proxy that forwards master only requests, so we don’t have to workaround that by having perpetually unready pods via the /api/leader-check endpoint 3.0.9 added HTTPAdvertise, which lets us eliminate the open raft port Signed-off-by: Derek Perkins <derek@derekperkins.com>

Signed-off-by: Derek Perkins <derek@derekperkins.com>

derekperkins · 2018-05-22T21:12:15Z

squashed and ready for merge

shlomi-noach · 2018-05-23T03:55:12Z

WOOHOO!

derekperkins · 2018-05-23T05:16:01Z

Thanks for the help with the core Orchestrator bits @shlomi-noach! This really makes it much nicer.

shlomi-noach · 2018-05-23T05:21:13Z

Hey I actually have no idea what I've signed up for! 😆

googlebot added the cla: yes label Feb 20, 2018

shlomi-noach approved these changes Feb 20, 2018

View reviewed changes

enisoc reviewed Feb 20, 2018

View reviewed changes

derekperkins force-pushed the orchestrator-update branch from e66efac to 860e41d Compare February 21, 2018 05:41

shlomi-noach mentioned this pull request Mar 1, 2018

Feature request: add HttpAdvertise openark/orchestrator#422

Closed

derekperkins force-pushed the orchestrator-update branch from 4948aca to f12d5ca Compare March 13, 2018 11:31

shlomi-noach requested changes Mar 14, 2018

View reviewed changes

derekperkins force-pushed the orchestrator-update branch 2 times, most recently from 385edbd to 2d32a99 Compare May 10, 2018 01:17

enisoc reviewed May 21, 2018

View reviewed changes

derekperkins force-pushed the orchestrator-update branch from c90d952 to eef85db Compare May 22, 2018 01:06

enisoc approved these changes May 22, 2018

View reviewed changes

derekperkins added 2 commits May 22, 2018 15:10

helm: update pmm to 1.10.0

a20c255

Signed-off-by: Derek Perkins <derek@derekperkins.com>

derekperkins force-pushed the orchestrator-update branch from 56ad760 to a20c255 Compare May 22, 2018 21:11

enisoc changed the title ~~helm: remove headless Orchestrator service~~ helm: Allow non-leader Orchestrator instances to accept requests May 22, 2018

enisoc merged commit c8e9070 into vitessio:master May 22, 2018

shlomi-noach mentioned this pull request Oct 2, 2018

Node unable to rejoin after failure bitpoke/mysql-operator#107

Closed

derekperkins deleted the orchestrator-update branch October 23, 2018 11:32

		@@ -24,34 +24,6 @@ spec:

		---

		###################################

helm: Allow non-leader Orchestrator instances to accept requests #3665

helm: Allow non-leader Orchestrator instances to accept requests #3665

Conversation

derekperkins commented Feb 20, 2018

derekperkins commented Feb 20, 2018

derekperkins commented Feb 20, 2018 • edited Loading

shlomi-noach 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

enisoc commented Feb 20, 2018

shlomi-noach commented Feb 21, 2018

shlomi-noach commented Feb 21, 2018

enisoc commented Feb 21, 2018

shlomi-noach commented Feb 22, 2018

enisoc commented Feb 22, 2018

shlomi-noach commented Feb 24, 2018

enisoc commented Feb 26, 2018

shlomi-noach commented Feb 27, 2018

enisoc commented Feb 27, 2018

shlomi-noach commented Mar 6, 2018 • edited Loading

shlomi-noach commented Mar 12, 2018

derekperkins commented Mar 13, 2018

derekperkins commented Mar 13, 2018

shlomi-noach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sougou commented May 6, 2018

derekperkins commented May 10, 2018

derekperkins commented May 10, 2018 • edited Loading

shlomi-noach commented May 10, 2018

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

derekperkins May 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekperkins commented May 22, 2018

derekperkins commented May 22, 2018

enisoc commented May 22, 2018

shlomi-noach commented May 22, 2018

derekperkins commented May 22, 2018

derekperkins commented May 22, 2018 • edited Loading

enisoc left a comment

Choose a reason for hiding this comment

derekperkins commented May 22, 2018

shlomi-noach commented May 23, 2018

derekperkins commented May 23, 2018

shlomi-noach commented May 23, 2018

derekperkins commented Feb 20, 2018 •

edited

Loading

shlomi-noach commented Mar 6, 2018 •

edited

Loading

derekperkins commented May 10, 2018 •

edited

Loading

derekperkins May 22, 2018 •

edited

Loading

derekperkins commented May 22, 2018 •

edited

Loading