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

config: Stop server startup if an unrecognized option is found in a config file #9855

Merged
merged 17 commits into from
Apr 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ff42683
Stop server startup if unrecognized options are found in the config file
kolbe Mar 21, 2019
d5c8f8a
Added test for unrecognized options in a config file throwing an error
kolbe Mar 21, 2019
3be74b0
Merge branch 'master' into kolbe-undecoded-config
kolbe Mar 21, 2019
7f526b0
Merge branch 'master' of github.com:pingcap/tidb into kolbe-undecoded…
kolbe Mar 21, 2019
b4207f7
Merge branch 'kolbe-undecoded-config' of github.com:kolbe/tidb into k…
kolbe Mar 21, 2019
46f9037
Merge branch 'kolbe-undecoded-config' into kolbe-config-check
kolbe Mar 22, 2019
3238d1e
Added --config-check flag
kolbe Mar 22, 2019
6266fd8
Clarified behavior of new --config-check flag in --help output
kolbe Mar 22, 2019
344e3d7
Merge branch 'master' into kolbe-undecoded-config
jackysp Mar 25, 2019
1062aff
Merge branch 'master' into kolbe-undecoded-config
xiekeyi98 Mar 25, 2019
5de01e9
Merge branch 'master' into kolbe-undecoded-config
kolbe Mar 25, 2019
f65f452
Merge branch 'master' of github.com:pingcap/tidb into kolbe-undecoded…
kolbe Apr 2, 2019
eb2a90a
Added new error type and some kind of circuitous handling to save and…
kolbe Apr 2, 2019
1d8d5ec
Merge branch 'master' of github.com:pingcap/tidb into kolbe-undecoded…
kolbe Apr 2, 2019
bb0ec3d
Added handling for case of --config-check=true --config-strict=false.…
kolbe Apr 2, 2019
573f82a
Merge branch 'master' into kolbe-undecoded-config
winkyao Apr 3, 2019
2ec9439
Merge branch 'master' into kolbe-undecoded-config
xiekeyi98 Apr 8, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 28 additions & 2 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ package config
import (
"crypto/tls"
"crypto/x509"
"fmt"
"io/ioutil"
"strings"
"time"

"github.com/BurntSushi/toml"
Expand Down Expand Up @@ -108,6 +110,18 @@ type Security struct {
ClusterSSLKey string `toml:"cluster-ssl-key" json:"cluster-ssl-key"`
}

// The ErrConfigValidationFailed error is used so that external callers can do a type assertion
// to defer handling of this specific error when someone does not want strict type checking.
// This is needed only because logging hasn't been set up at the time we parse the config file.
// This should all be ripped out once strict config checking is made the default behavior.
type ErrConfigValidationFailed struct {
err string
}

func (e *ErrConfigValidationFailed) Error() string {
return e.err
}

// ToTLSConfig generates tls's config based on security section of the config.
func (s *Security) ToTLSConfig() (*tls.Config, error) {
var tlsConfig *tls.Config
Expand Down Expand Up @@ -373,11 +387,23 @@ func GetGlobalConfig() *Config {

// Load loads config options from a toml file.
func (c *Config) Load(confFile string) error {
Copy link
Member

Choose a reason for hiding this comment

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

When upgrade from old version tidb to a newer version, some old config item maybe is not used in the newer version tidb. The rolling upgrade may meet error in this case, then the rolling upgrade failed.

How about only check the config items when -config-check is provided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this suggestion. I think the behavior should be that tidb-server simply refuses to start if there are unknown configuration options, for the reasons I provided above.

A couple points:

  1. Now that there's a --config-check option, it can be a part of the recommended procedure for a rolling upgrade, so that the only people running into the issue you describe are those not following the recommended procedure.

  2. The benefit of a rolling upgrade is that you never have to take the entire cluster offline. When upgrading the first node, it will refuse to start with this error. That's a fine time to address the problem, apply the change to the config files, get that first node upgraded, and then proceed with the rolling upgrade.

@morgo, your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think because our own CI broke, we may need to ease this change in slowly. Here would be my suggestion:

  1. Keep --config-check as is, it is independently very useful.
  2. Change from refuse to start to print a WARN about each incorrect config item.
  3. Add a new flag for --config-strict, which defaults to FALSE. We can document it that enabling it is a best practice, and we may change the default to TRUE in the future. This will result in errors for invalid options.

It is not bulletproof of course, because the default log level is INFO it is possible that the warnings may not be seen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the very fact our own CI broke is evidence of the value of this behavior change. Even we weren't keeping track of whether our config files were well-formed!

  1. Your suggestion above is difficult, because the logging system isn't set up yet when the config file is parsed. Either we have to keep around the list of undecoded items and check them later after logging is possible, or we have to pass some other string/structure/flag around, or we have to simply print something to stderr, or ...? @coocood Morgan mentioned that you may have some thoughts on this?

  2. I dislike this. Adding this new flag means that people may start relying on it, and then we have to keep it around as a no-op if we later make that behavior the default.

Copy link
Contributor

@winkyao winkyao Mar 26, 2019

Choose a reason for hiding this comment

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

I do agree with Morgan's first and second suggestions. Our users may use old version ansible to upgrade, that doesn't pre-check the config using tidb-server --config-check, we need to keep the cluster stable when rolling upgrade.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, it sounds like there's a consensus. Can someone offer a suggestion of the best way to technically implement a "warning" before the logging system is set up?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kolbe I will create a pull request to your branch later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@winkyao I added some comments on your PR. I re-wrote my own interpretation of how I see this working and pushed it to my branch of my fork, please take a look.

Basically I added a kind of unsettling amount of additional stuff to create a custom error in config.go, then use that in the case of failed config validation. That enables checking for the specific error type in main.go, and if configStrict is not enabled, it'll get the string from the warning and keep that until ater logging has been set up.

This is kind of a lot of extra work and adds some ugly things, but the whole idea behind strict config checking (and the --config-strict option) is that this is a temporary situation until we make --config-strict behavior the default (and hopefully only!) behavior of TiDB Server in the future. I think at that point we'd simply remove all this extra stuff with the custom error and strange handling in the server.

Thoughts?

_, err := toml.DecodeFile(confFile, c)
metaData, err := toml.DecodeFile(confFile, c)
if c.TokenLimit <= 0 {
c.TokenLimit = 1000
}
return errors.Trace(err)

// If any items in confFile file are not mapped into the Config struct, issue
// an error and stop the server from starting.
undecoded := metaData.Undecoded()
if len(undecoded) > 0 && err == nil {
var undecodedItems []string
for _, item := range undecoded {
undecodedItems = append(undecodedItems, item.String())
}
err = &ErrConfigValidationFailed{fmt.Sprintf("config file %s contained unknown configuration options: %s", confFile, strings.Join(undecodedItems, ", "))}
}

return err
}

// ToLogConfig converts *Log to *logutil.LogConfig.
Expand Down
20 changes: 17 additions & 3 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,27 @@ func (s *testConfigSuite) TestConfig(c *C) {

f, err := os.Create(configFile)
c.Assert(err, IsNil)
_, err = f.WriteString(`[performance]

// Make sure the server refuses to start if there's an unrecognized configuration option
_, err = f.WriteString(`
unrecognized-option-test = true
`)
c.Assert(err, IsNil)
c.Assert(f.Sync(), IsNil)

c.Assert(conf.Load(configFile), ErrorMatches, "(?:.|\n)*unknown configuration option(?:.|\n)*")

f.Truncate(0)
f.Seek(0, 0)

_, err = f.WriteString(`
token-limit = 0
[performance]
[tikv-client]
commit-timeout="41s"
max-batch-size=128

token-limit = -1
`)

c.Assert(err, IsNil)
c.Assert(f.Sync(), IsNil)

Expand Down
30 changes: 26 additions & 4 deletions tidb-server/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ import (
const (
nmVersion = "V"
nmConfig = "config"
nmConfigCheck = "config-check"
nmConfigStrict = "config-strict"
nmStore = "store"
nmStorePath = "path"
nmHost = "host"
Expand Down Expand Up @@ -90,8 +92,10 @@ const (
)

var (
version = flagBoolean(nmVersion, false, "print version information and exit")
configPath = flag.String(nmConfig, "", "config file path")
version = flagBoolean(nmVersion, false, "print version information and exit")
configPath = flag.String(nmConfig, "", "config file path")
configCheck = flagBoolean(nmConfigCheck, false, "check config file validity and exit")
configStrict = flagBoolean(nmConfigStrict, false, "enforce config file validity")

// Base
store = flag.String(nmStore, "mocktikv", "registered store name, [tikv, mocktikv]")
Expand Down Expand Up @@ -141,11 +145,21 @@ func main() {
}
registerStores()
registerMetrics()
loadConfig()
configWarning := loadConfig()
overrideConfig()
validateConfig()
if *configCheck {
kolbe marked this conversation as resolved.
Show resolved Hide resolved
fmt.Println("config check successful")
os.Exit(0)
}
setGlobalVars()
setupLog()
// If configStrict had been specified, and there had been an error, the server would already
// have exited by now. If configWarning is not an empty string, write it to the log now that
// it's been properly set up.
if configWarning != "" {
log.Warn(configWarning)
}
setupTracing() // Should before createServer and after setup config.
printInfo()
setupBinlogClient()
Expand Down Expand Up @@ -287,12 +301,20 @@ func flagBoolean(name string, defaultVal bool, usage string) *bool {
return flag.Bool(name, defaultVal, usage)
}

func loadConfig() {
func loadConfig() string {
cfg = config.GetGlobalConfig()
if *configPath != "" {
err := cfg.Load(*configPath)
// This block is to accommodate an interim situation where strict config checking
// is not the default behavior of TiDB. The warning message must be deferred until
// logging has been set up. After strict config checking is the default behavior,
// This should all be removed.
if _, ok := err.(*config.ErrConfigValidationFailed); ok && !*configCheck && !*configStrict {
return err.Error()
}
terror.MustNil(err)
}
return ""
}

func overrideConfig() {
Expand Down