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

Throttler: fix nil pointer dereference error #15180

Merged

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented Feb 8, 2024

Description

Fixes #15179 by removing excessive and ineffective line of code.

This needs to be backported to release 19

The nil pointer dereference error could happen when a v19 primary runs with a v18 replica.

Related Issue(s)

#15179

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Copy link
Contributor

vitess-bot bot commented Feb 8, 2024

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Feb 8, 2024
@github-actions github-actions bot added this to the v20.0.0 milestone Feb 8, 2024
@deepthi deepthi removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Feb 8, 2024
@shlomi-noach
Copy link
Contributor Author

The current unit test failure is unrelated to this change:

2024-02-08T16:33:42.6950434Z === RUN   TestMtlsAuthUnauthorizedFails
2024-02-08T16:33:42.6950904Z I0208 16:30:59.991164   25468 tlstest.go:167] Creating test root CA in /tmp/TestMtlsAuthUnauthorizedFails1273706000/001
2024-02-08T16:33:42.6951238Z I0208 16:30:59.991604   25468 tlstest.go:236] Creating signed cert and key vtctld.example.com
2024-02-08T16:33:42.6951537Z I0208 16:30:59.992025   25468 tlstest.go:236] Creating signed cert and key AnotherApp
2024-02-08T16:33:42.6951755Z I0208 16:30:59.992812   25468 main.go:297] Starting local cluster...
2024-02-08T16:33:42.6955027Z I0208 16:30:59.992830   25468 main.go:298] config: vttest.Config{Topology:(*vttest.VTTestTopology)(0xc00085e960), Seed:(*vttest.SeedConfig)(nil), SchemaDir:"data/schema", DefaultSchemaDir:"", DataDir:"", Charset:"utf8mb4", PlannerVersion:"", ExtraMyCnf:[]string(nil), OnlyMySQL:false, PersistentMode:false, VtComboBindAddress:"localhost", MySQLBindHost:"localhost", SnapshotFile:"", EnableSystemSettings:true, TransactionMode:"MULTI", TransactionTimeout:0, TabletHostName:"localhost", InitWorkflowManager:false, VSchemaDDLAuthorizedUsers:"%", ForeignKeyMode:"allow", EnableOnlineDDL:true, EnableDirectDDL:true, ExternalTopoImplementation:"", ExternalTopoGlobalServerAddress:"", ExternalTopoGlobalRoot:"", VtgateTabletRefreshInterval:10000000000}
2024-02-08T16:33:42.6955423Z I0208 16:30:59.992865   25468 local_cluster.go:351] No environment in cluster settings. Creating default...
2024-02-08T16:33:42.6956795Z I0208 16:30:59.992963   25468 local_cluster.go:358] LocalCluster environment: &{BasePort:24616 TmpPath:/tmp/vttest1461989537 DefaultMyCnf:[/home/runner/work/vitess/vitess/config/mycnf/test-suite.cnf] Env:[VTDATAROOT=/tmp/vttest1461989537 VTTEST=endtoend] EnableToxiproxy:false}
2024-02-08T16:33:42.6957159Z I0208 16:30:59.992995   25468 local_cluster.go:381] Initializing MySQL Manager (*vttest.Mysqlctl)...
2024-02-08T16:33:42.6957481Z E0208 16:31:59.994745   25468 local_cluster.go:383] Mysqlctl failed to start: signal: killed
2024-02-08T16:33:42.6959559Z E0208 16:31:59.994788   25468 local_cluster.go:385] stderr: W0208 16:31:00.001579   36028 log.go:39] Failed to read in config : Config File "vtconfig" Not Found in "[/home/runner/work/vitess/vitess/go/cmd/vttestserver/cli]". This is optional, and can be ignored if you are not using config files. For a detailed explanation, see https://github.com/vitessio/vitess/blob/main/doc/viper/viper.md#config-files.
2024-02-08T16:33:42.6959802Z I0208 16:31:00.003594   36028 dbconfigs.go:372] DBConfigs: allprivs:
2024-02-08T16:33:42.6959937Z   password: '****'
2024-02-08T16:33:42.6960025Z app:
2024-02-08T16:33:42.6960158Z   password: '****'
2024-02-08T16:33:42.6960248Z appdebug:
2024-02-08T16:33:42.6960379Z   password: '****'
2024-02-08T16:33:42.6960473Z charset: utf8mb4
2024-02-08T16:33:42.6960557Z dba:
2024-02-08T16:33:42.6960678Z   password: '****'
2024-02-08T16:33:42.6960776Z   useSsl: true
2024-02-08T16:33:42.6960880Z   user: vt_dba
2024-02-08T16:33:42.6960968Z filtered:
2024-02-08T16:33:42.6961098Z   password: '****'
2024-02-08T16:33:42.6961180Z repl:
2024-02-08T16:33:42.6961302Z   password: '****'
2024-02-08T16:33:42.6961313Z 
2024-02-08T16:33:42.6961604Z I0208 16:31:00.011934   36028 mysqld.go:195] Using flavor: mysql, version: {5 7 42}
2024-02-08T16:33:42.6962217Z I0208 16:31:00.011996   36028 mysqld.go:690] mysqlctl.Init running with contents previously embedded from /home/runner/work/vitess/vitess/config/init_db.sql
2024-02-08T16:33:42.6962430Z I0208 16:31:00.012007   36028 mysqld.go:672] mysqlctl.InitConfig
2024-02-08T16:33:42.6962778Z I0208 16:31:00.012013   36028 mysqld.go:984] creating directory /tmp/vttest1461989537/vt_0000000001
2024-02-08T16:33:42.6963158Z I0208 16:31:00.012152   36028 mysqld.go:1018] creating directory /tmp/vttest1461989537/vt_0000000001/data
2024-02-08T16:33:42.6963532Z I0208 16:31:00.012219   36028 mysqld.go:1018] creating directory /tmp/vttest1461989537/vt_0000000001/innodb
2024-02-08T16:33:42.6964021Z I0208 16:31:00.012278   36028 mysqld.go:1018] creating directory /tmp/vttest1461989537/vt_0000000001/relay-logs
2024-02-08T16:33:42.6964487Z I0208 16:31:00.012332   36028 mysqld.go:1018] creating directory /tmp/vttest1461989537/vt_0000000001/bin-logs
2024-02-08T16:33:42.6964862Z I0208 16:31:00.012376   36028 mysqld.go:994] creating directory /tmp/vttest1461989537/vt_0000000001/data
2024-02-08T16:33:42.6965256Z I0208 16:31:00.012390   36028 mysqld.go:994] creating directory /tmp/vttest1461989537/vt_0000000001/innodb/data
2024-02-08T16:33:42.6965650Z I0208 16:31:00.012431   36028 mysqld.go:994] creating directory /tmp/vttest1461989537/vt_0000000001/innodb/logs
2024-02-08T16:33:42.6966150Z I0208 16:31:00.012476   36028 mysqld.go:994] creating directory /tmp/vttest1461989537/vt_0000000001/tmp
2024-02-08T16:33:42.6966627Z I0208 16:31:00.012523   36028 mysqld.go:994] creating directory /tmp/vttest1461989537/vt_0000000001/relay-logs
2024-02-08T16:33:42.6967089Z I0208 16:31:00.012536   36028 mysqld.go:994] creating directory /tmp/vttest1461989537/vt_0000000001/bin-logs
2024-02-08T16:33:42.6967513Z I0208 16:31:00.012554   36028 mysqld.go:830] make_mycnf hook doesn't exist, reading template files
2024-02-08T16:33:42.6967929Z I0208 16:31:00.012879   36028 mysqld.go:786] Installing data dir with mysqld --initialize-insecure
2024-02-08T16:33:42.6968370Z I0208 16:31:01.572765   36028 mysqld.go:352] Mysqld.Start(1707409861): No mysqld_start hook, running mysqld_safe directly
2024-02-08T16:33:42.6972072Z I0208 16:31:01.573000   36028 mysqld.go:392] Mysqld.Start(1707409861) &exec.Cmd{Path:"/usr/bin/mysqld_safe", Args:[]string{"/usr/bin/mysqld_safe", "--defaults-file=/tmp/vttest1461989537/vt_0000000001/my.cnf", "--basedir=/usr"}, Env:[]string{"LD_LIBRARY_PATH=/usr/lib/mysql", "LD_PRELOAD=libeatmydata.so"}, Dir:"/usr", Stdin:io.Reader(nil), Stdout:io.Writer(nil), Stderr:io.Writer(nil), ExtraFiles:[]*os.File(nil), SysProcAttr:(*syscall.SysProcAttr)(nil), Process:(*os.Process)(nil), ProcessState:(*os.ProcessState)(nil), ctx:context.Context(nil), Err:error(nil), Cancel:(func() error)(nil), WaitDelay:0, childIOFiles:[]io.Closer(nil), parentIOPipes:[]io.Closer(nil), goroutine:[]func() error(nil), goroutineErr:(<-chan error)(nil), ctxResult:(<-chan exec.ctxResult)(nil), createdByStack:[]uint8(nil), lookPathErr:error(nil)}
2024-02-08T16:33:42.6972705Z I0208 16:31:01.573453   36028 mysqld.go:501] Waiting for mysqld socket file (/tmp/vttest1461989537/vt_0000000001/mysql.sock) to be ready...
2024-02-08T16:33:42.6973500Z I0208 16:31:01.931998   36028 mysqld.go:410] Mysqld.Start(1707409861) stdout: 2024-02-08T16:31:01.931699Z mysqld_safe Logging to '/tmp/vttest1461989537/vt_0000000001/error.log'.
2024-02-08T16:33:42.6974409Z I0208 16:31:01.954839   36028 mysqld.go:410] Mysqld.Start(1707409861) stdout: 2024-02-08T16:31:01.954544Z mysqld_safe Starting mysqld daemon with databases from /tmp/vttest1461989537/vt_0000000001/data
2024-02-08T16:33:42.6975263Z I0208 16:31:03.586006   36028 mysqld.go:410] Mysqld.Start(1707409861) stdout: 2024-02-08T16:31:03.585652Z mysqld_safe mysqld from pid file /tmp/vttest1461989537/vt_0000000001/mysql.pid ended
2024-02-08T16:33:42.6975538Z I0208 16:31:03.586945   36028 mysqld.go:423] Mysqld.Start(1707409861) exit: <nil>
2024-02-08T16:33:42.6975688Z     main_test.go:306: 
2024-02-08T16:33:42.6976218Z         	Error Trace:	/home/runner/work/vitess/vitess/go/cmd/vttestserver/cli/main_test.go:306
2024-02-08T16:33:42.6976822Z         	Error:      	"signal: killed" does not contain "code = Unauthenticated desc = client certificate not authorized"
2024-02-08T16:33:42.6977058Z         	Test:       	TestMtlsAuthUnauthorizedFails
2024-02-08T16:33:42.6977277Z --- FAIL: TestMtlsAuthUnauthorizedFails (60.05s)
2024-02-08T16:33:42.6977361Z FAIL

Copy link

codecov bot commented Feb 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3a98b4c) 67.28% compared to head (0d28546) 67.28%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15180   +/-   ##
=======================================
  Coverage   67.28%   67.28%           
=======================================
  Files        1559     1559           
  Lines      192056   192061    +5     
=======================================
+ Hits       129219   129225    +6     
+ Misses      62837    62836    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shlomi-noach shlomi-noach added Component: General Changes throughout the code base Type: Bug and removed Component: General Changes throughout the code base Type: Bug labels Feb 8, 2024
@frouioui frouioui added Backport This is a backport and removed Backport This is a backport labels Feb 8, 2024
@shlomi-noach shlomi-noach merged commit d3b2593 into vitessio:main Feb 8, 2024
107 of 111 checks passed
@shlomi-noach shlomi-noach deleted the throttler-nil-reference-fix branch February 8, 2024 17:09
vitess-bot pushed a commit that referenced this pull request Feb 8, 2024
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
shlomi-noach added a commit that referenced this pull request Feb 8, 2024
…15181)

Signed-off-by: Florent Poinsard <florent.poinsard@outlook.fr>
Signed-off-by: Vicent Marti <vmg@strn.cat>
Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
Co-authored-by: Florent Poinsard <florent.poinsard@outlook.fr>
Co-authored-by: Vicent Marti <vmg@strn.cat>
Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug Report: Throttler: nil pointer dereference
4 participants