Skip to content
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

vtctld: flag to proxy vttablet URLs #6058

Merged
merged 2 commits into from
Apr 13, 2020

Conversation

sougou
Copy link
Contributor

@sougou sougou commented Apr 13, 2020

This is a long awaited change. Adding a -proxy_tablets flag to vtctld which will make it act as a reverse proxy for all its vttablets.

This should ease a lot of pain from the kubernetes users.

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
@sougou
Copy link
Contributor Author

sougou commented Apr 13, 2020

I tested it manually. But was out of my element on an automated test. If someone wants to write a test, I'd be very happy to accept the work.

@sougou sougou requested a review from morgo April 13, 2020 00:40
Copy link
Contributor

@morgo morgo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@setassociative
Copy link
Contributor

setassociative commented Apr 13, 2020

Should this also allow proxying vtgate endpoints?

In the long run that would set up the VTCtld as a single destination for HTTP apis which feels good.

@sougou
Copy link
Contributor Author

sougou commented Apr 13, 2020

Should this also allow proxying vtgate endpoints?

In the long run that would set up the VTCtld as a single destination for HTTP apis which feels good.

VTGates haven't become discoverable yet. I think there's a feature request for it. But once they do, we'd naturally have to extend vtctld for them.

@sougou sougou merged commit 231cd51 into vitessio:master Apr 13, 2020
proxyMu.Lock()
defer proxyMu.Unlock()
if _, ok := remotes[tabletID]; ok {
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean the proxy will stop working for a given tablet after its address changes?

}
return nil
}
http.Handle(prefixPath, rp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more natural to respond to tablets coming/going and changing addresses if there were only one handler, which looks at a cached lookup table of tablet aliases to addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more natural to respond to tablets coming/going and changing addresses if there were only one handler, which looks at a cached lookup table of tablet aliases to addresses.

This is what I started out with. The problem with the unified approach is that there is no easy way to associate a response to a request, which is needed for rewriting URLs to match the tablet id. I looked at making the vttablets send an extra header identifying the tablet id. But the one-proxy-per-tablet felt much simpler.

What I'll instead do is replace the per-tablet proxy every time. I looked at the code. It's just metdata (no goroutines etc). So, it should be fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Couldn't you still have a single handler though? If reverse proxy objects are cheap to make, you can just make a new one for each request you handle and use it once. Then you don't have handlers slowly leaking in cases when tablet IDs go away.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the page that renders the tablets is the one that causes the map entry to be created.

Thinking about it again, there's no real need for that page to be controlling the redirects. We could directly GetTablet at the time of request, create the redirect and forward it. Let me update the other PR to reflect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants