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

[query] Remote storages can warn rather than error on failure #1938

Merged
merged 6 commits into from
Sep 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 19 additions & 6 deletions src/cmd/services/m3query/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -419,29 +419,37 @@ type ClusterManagementConfiguration struct {
Etcd etcdclient.Configuration `yaml:"etcd"`
}

// Remotes is a set of remote host configurations.
type Remotes []Remote
// RemoteConfigurations is a set of remote host configurations.
type RemoteConfigurations []RemoteConfiguration

// Remote is the configuration for a single remote host.
type Remote struct {
// RemoteConfiguration is the configuration for a single remote host.
Copy link
Contributor

@pavelnikolov pavelnikolov Sep 12, 2019

Choose a reason for hiding this comment

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

nit: Why do you need this? It's already in the config package and in the config.go file. So you end up with config.RemoteConfiguration vs config.Remote. I prefer the shorter version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mostly to avoid conflicts with the new more concrete Remote type; ideally we shouldn't be using RemoteConfiguration anywhere outside this package, but we have to make it public for yaml parsing

type RemoteConfiguration struct {
// Name is the name for the remote zone.
Name string `yaml:"name"`
// RemoteListenAddresses is the remote listen addresses to call for remote
// coordinator calls in the remote zone.
RemoteListenAddresses []string `yaml:"remoteListenAddresses"`
// ErrorBehavior overrides the default error behavior for this host.
//
// NB: defaults to warning on error.
ErrorBehavior *storage.ErrorBehavior `yaml:"errorBehavior"`
Copy link
Contributor

Choose a reason for hiding this comment

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

If ErrorBehavior is uint8 how do you serialize it to yaml? Shouldn't you have some string representation instead of numbers? Also, I saw that the constants are using iota assignment which means that if in future someone adds more values, these need to always be appended at the end. Otherwise, existing apps might break.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So we've defined a custom yaml unmarshall here, which allows us to use the plaintext version of the desired behavior in the yaml. The documentation is definitely missing at the moment, but you can look at some of the new tests added in remote_options_test.go to see how the options will look in yaml (in all honesty you'd probably be fine with just taking the default approach which sets all remotes to BehaviorWarn)

}

// RPCConfiguration is the RPC configuration for the coordinator for
// the GRPC server used for remote coordinator to coordinator calls.
type RPCConfiguration struct {
// Enabled determines if coordinator RPC is enabled for remote calls.
Enabled bool `yaml:"enabled"`
//
// NB: this is no longer necessary to set to true if RPC is desired; enabled
// status is inferred based on which other options are provided;
// this remains for back-compat, and for disabling any existing RPC options.
Enabled *bool `yaml:"enabled"`

// ListenAddress is the RPC server listen address.
ListenAddress string `yaml:"listenAddress"`

// Remotes are the configuration settings for remote coordinator zones.
Remotes Remotes `yaml:"remotes"`
Remotes RemoteConfigurations `yaml:"remotes"`

// RemoteListenAddresses is the remote listen addresses to call for
// remote coordinator calls.
Expand All @@ -450,6 +458,11 @@ type RPCConfiguration struct {
// RemoteListenAddresses will only allow for a single remote zone to be used.
RemoteListenAddresses []string `yaml:"remoteListenAddresses"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is RemoteListenAddresses deprecated in favour of Remotes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah; it used to limit to a single zone; with this PR, can technically do both but would recommend sticking to Remotes

At some point soon we'll update docs as well as our example configs to mirror this


// ErrorBehavior overrides the default error behavior for all rpc hosts.
//
// NB: defaults to warning on error.
ErrorBehavior *storage.ErrorBehavior `yaml:"errorBehavior"`

// ReflectionEnabled will enable reflection on the GRPC server, useful
// for testing connectivity with grpcurl, etc.
ReflectionEnabled bool `yaml:"reflectionEnabled"`
Expand Down
128 changes: 128 additions & 0 deletions src/cmd/services/m3query/config/remote_options.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
// Copyright (c) 2019 Uber Technologies, Inc.
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

package config

import (
"github.com/m3db/m3/src/query/storage"
)

// Remote is an option for remote storage.
type Remote struct {
Copy link
Contributor

@pavelnikolov pavelnikolov Sep 12, 2019

Choose a reason for hiding this comment

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

nit: Why do you have both Remote and RemoteConfiguration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So RemoteConfiguration is the parsed yaml, Remote is the parsed structure

// ErrorBehavior describes what this remote client should do on error.
ErrorBehavior storage.ErrorBehavior
// Name is the name for this remote client.
Name string
// Addresses are the remote addresses for this client.
Addresses []string
}

func makeRemote(
name string,
addresses []string,
global storage.ErrorBehavior,
override *storage.ErrorBehavior,
) Remote {
if override != nil {
global = *override
}
Copy link
Collaborator

@robskillington robskillington Sep 12, 2019

Choose a reason for hiding this comment

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

nit(ignore if you'd like): Any reason to do this in the constructor, than perhaps just in the for loop creating the remotes?

Had to follow to next method call to work out what the ... defaultBehavior, nil) meant (i.e. what is nil param used for).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah yeah missed responding to this; tried it both ways and this looked a little cleaner


return Remote{Name: name, Addresses: addresses, ErrorBehavior: global}
}

// RemoteOptions are the options for RPC configurations.
type RemoteOptions interface {
// ServeEnabled describes if this RPC should serve rpc requests.
ServeEnabled() bool
// ServeAddress describes the address this RPC server will listening on.
ServeAddress() string
// ListenEnabled describes if this RPC should connect to remote clients.
ListenEnabled() bool
// ReflectionEnabled describes if this RPC server should have reflection
// enabled.
ReflectionEnabled() bool
// Remotes is a list of remote clients.
Remotes() []Remote
}

type remoteOptions struct {
enabled bool
reflectionEnabled bool
address string
remotes []Remote
}

// RemoteOptionsFromConfig builds remote options given a set of configs.
func RemoteOptionsFromConfig(cfg *RPCConfiguration) RemoteOptions {
if cfg == nil {
return &remoteOptions{}
}

// NB: Remote storage enabled should be determined by which options are
// present unless enabled is explicitly set to false.
enabled := true
if cfg.Enabled != nil {
enabled = *cfg.Enabled
}

// NB: Remote storages warn on failure by default.
defaultBehavior := storage.BehaviorWarn
if cfg.ErrorBehavior != nil {
defaultBehavior = *cfg.ErrorBehavior
}

remotes := make([]Remote, 0, len(cfg.Remotes)+1)
if len(cfg.RemoteListenAddresses) > 0 {
remotes = append(remotes, makeRemote("default", cfg.RemoteListenAddresses,
defaultBehavior, nil))
}

for _, remote := range cfg.Remotes {
remotes = append(remotes, makeRemote(remote.Name,
remote.RemoteListenAddresses, defaultBehavior, remote.ErrorBehavior))
}

return &remoteOptions{
enabled: enabled,
reflectionEnabled: cfg.ReflectionEnabled,
address: cfg.ListenAddress,
remotes: remotes,
}
}

func (o *remoteOptions) ServeEnabled() bool {
return o.enabled && len(o.address) > 0
}

func (o *remoteOptions) ServeAddress() string {
return o.address
}

func (o *remoteOptions) ListenEnabled() bool {
return o.enabled && len(o.remotes) > 0
}

func (o *remoteOptions) ReflectionEnabled() bool {
return o.enabled && o.reflectionEnabled
}

func (o *remoteOptions) Remotes() []Remote {
return o.remotes
}
Loading