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

Ensure vttablet does not crash if not given all parameters it expects #3195

Closed

Conversation

sjmudd
Copy link
Contributor

@sjmudd sjmudd commented Sep 18, 2017

Specifically I managed to crash it when mycnf was empty and it tried to reference the socket name to talk to mysqld on. Even if that setting is not present the binary should not fail and given it's possible via the appropraite dbconfig-XXXX settings to configure access to a remote mysqld then the requirement for the socket should not be there.

If you see a better fix then feel free to implement this.

Specifically I managed to crash it when mycnf was empty and it tried to
reference the socket name to talk to mysqld on. Even if that setting
is not present the binary should not fail and given it's possible via
the appropraite dbconfig-XXXX settings to configure access to a remote
mysqld then the requirement for the socket should not be there.

If you see a better fix then feel free to implement this.
@sjmudd
Copy link
Contributor Author

sjmudd commented Sep 18, 2017

Example (simplified) usage where this happened:

[user@somehost ~/dev/bin]$ export VTDATAROOT=$HOME/dev/vtdataroot ./vttablet -tablet-path cell1-0000000100

Would trigger the breakage.

@sjmudd
Copy link
Contributor Author

sjmudd commented Sep 18, 2017

This "fix" only causes problems later on in github.com/youtube/vitess/go/vt/mysqlctl/mysqld.go:91 where I get a separate segmentation violation.

Basically vttablet which shouldn't need much in the way of "mysqld information" seems to pull off several settings from a my.cnf file in the $VTDATAROOT/tablet-path/my.cnf file. However, it shouldn't really need them in most cases and especially if the mysqld is not on the same server.

What's somewhat troublesome is if you're trying to not use vttablet to manage the underlying mysqld is to know exactly which settings are expected as the error messages do not tell you this.

So perhaps this specific PR should actually give an error message: socket not specified in /path/to/tablet-path/my.cnf. Please provide access credentials to mysqld using a suitably populated my.cnf file, <some_socket_parameter> or <some_other_credentials_which_say_how_to_access_mysqld>

Maybe too line 91 of mysqld.go above could catch the missing settings and give a good message too?

I can change this PR to something better but am not sure what you think is acceptable or best, but from my point of view clarifying the required configs needed to reach vttablet (managed or not) would be useful as then we'd have a clearer target to improve our configuration and launch scripts.

So feedback on what you think about this and the best way if I bump into things like this again to handle them would be most welcome.

@sjmudd
Copy link
Contributor Author

sjmudd commented Sep 19, 2017

On second thoughts maybe for now it's better to not try to fix each problem but to catch and report the situation and let the user deal with this?

Longer term simplification of command line parameters may be better but avoiding a crash and indicating which expected variable is nil at least points at the technical cause even if it does lead the user to have to investigate further in the code.

sougou added a commit to sougou/vitess that referenced this pull request Jan 21, 2018
This bug has confused many people because it just crashes vttablet
with a mysterious SIGSEGV. The problem was a bug in the error
propagation of mycnf which was using old panic style error handling.

This also addresses what vitessio#3195 was trying to fix.
@sougou
Copy link
Contributor

sougou commented Jan 22, 2018

Fixed via #3568

@sougou sougou closed this Jan 22, 2018
frouioui pushed a commit to planetscale/vitess that referenced this pull request Nov 21, 2023
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
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.

3 participants