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

make: build vitess as static binaries by default #7795

Merged
merged 5 commits into from
Apr 7, 2021

Conversation

vmg
Copy link
Collaborator

@vmg vmg commented Apr 6, 2021

Description

We're trying to solve two related issues here:

  1. Since the introduction of vtorc, all the different binaries have ballooned up in size and dependencies, because the CGo dependencies of orchestrator (namely, SQLite) were being propagated through the build.
  2. Since newer versions of Go have been released, dynamically linked Go binaries depend on newer versions of libc that are not available on older OSSes.

Let's try to fix the two issues at once: first, we stop importing the go-sqlite package on orchestrator's sqlutils package, because it's being used in other parts of the codebase. We can add the dependency at the root of the binary itself. Secondly, we update our Makefile so that CGo is disabled by default for all binaries except vtorc.

Building Vitess with CGo disabled by default has a major implication: vitess will use Go's native DNS resolver always, and this could potentially be a breaking change. Usually, the Go runtime chooses the DNS resolver dynamically at runtime, between its native resolver and the one on glibc based on some heuristics. The native resolver is always selected unless the runtime detects configuration options in resolv.conf or /etc/gai.conf that it doesn't know how to handle (you can see the full heuristics here). By disabling CGo, the native resolver will always be used, and particular deployments that customize their DNS resolution in unsupported ways will error out with a helpful message.

How breaking is this change? I'm not sure, that's why I left a target in the Makefile to keep building versions of Vitess that depend dynamically on libc. I think that the sensible choice by far would be to ship Vitess as static binaries by default, because it greatly simplifies deployment in a large amount of (older) OS versions that we couldn't support otherwise -- an use case much more frequent than deploying in modern OSses with env-defined local resolver configurations.

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

vmg added 2 commits April 6, 2021 13:21
Signed-off-by: Vicent Marti <vmg@strn.cat>
Signed-off-by: Vicent Marti <vmg@strn.cat>
Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

👍 to the change in sqlutils

testing the Makefile changes at your discretion

vmg added 2 commits April 6, 2021 18:43
Signed-off-by: Vicent Marti <vmg@strn.cat>
Signed-off-by: Vicent Marti <vmg@strn.cat>
@deepthi
Copy link
Member

deepthi commented Apr 6, 2021

Building Vitess with CGo disabled by default has a major implication: vitess will use Go's native DNS resolver always, and this could potentially be a breaking change.

We have gone back and forth with CGo. Vitess used to have a CGo dependency that was removed around 2018, and it was added back when we brought in orchestrator in 2020. I don't believe this will be a big deal. Nobody reported issues during either transition.

Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
Copy link
Collaborator Author

vmg commented Apr 7, 2021

@deepthi: Good to know. I've switched to static build by default then; all tests are green. Good to merge?

@deepthi deepthi requested review from dkhenry and sougou April 7, 2021 15:51
@deepthi
Copy link
Member

deepthi commented Apr 7, 2021

Let's have either @sougou or @dkhenry review as well. Once we get an OK from one of them, it will be good to merge.

Copy link
Contributor

@dkhenry dkhenry left a comment

Choose a reason for hiding this comment

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

I agree with @deepthi when we made this change last time we didn't see any impact. It would still be good form to list this in the v10 release notes as a potential breaking change.

@deepthi
Copy link
Member

deepthi commented Apr 7, 2021

I agree with @deepthi when we made this change last time we didn't see any impact. It would still be good form to list this in the v10 release notes as a potential breaking change.

@askdba please make a note for release notes (assuming we are porting this to the release-10.0 branch).

@deepthi deepthi merged commit bbfe9c6 into vitessio:master Apr 7, 2021
@deepthi
Copy link
Member

deepthi commented Apr 7, 2021

@vmg can you create a cherry-pick PR on the release-10.0 branch so that we can get this into v10?

@vmg
Copy link
Collaborator Author

vmg commented Apr 7, 2021

👍 I'll do it first thing tomorrow.

@vmg
Copy link
Collaborator Author

vmg commented Apr 8, 2021

Here: #7808

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

Successfully merging this pull request may close these issues.

7 participants