From 6cf60e534473239e605141c1f542b7b4005e56f5 Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Thu, 5 Oct 2017 11:47:14 -0700 Subject: [PATCH 1/4] remove verbose/stderr glog flags Signed-off-by: Steve Kriss --- examples/azure/00-ark-deployment.yaml | 3 --- examples/common/10-deployment.yaml | 3 --- 2 files changed, 6 deletions(-) diff --git a/examples/azure/00-ark-deployment.yaml b/examples/azure/00-ark-deployment.yaml index 12ad84e89..efe7bbc18 100644 --- a/examples/azure/00-ark-deployment.yaml +++ b/examples/azure/00-ark-deployment.yaml @@ -34,9 +34,6 @@ spec: - /ark args: - server - - --logtostderr - - --v - - "4" envFrom: - secretRef: name: cloud-credentials diff --git a/examples/common/10-deployment.yaml b/examples/common/10-deployment.yaml index a9d8f8fea..e605157c5 100644 --- a/examples/common/10-deployment.yaml +++ b/examples/common/10-deployment.yaml @@ -34,9 +34,6 @@ spec: - /ark args: - server - - --logtostderr - - --v - - "4" volumeMounts: - name: cloud-credentials mountPath: /credentials From 9f3ce8ab1a4cd96734c1a324500076cee36f24a7 Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Thu, 5 Oct 2017 13:44:19 -0700 Subject: [PATCH 2/4] add log-level flag to server command Signed-off-by: Steve Kriss --- docs/cli-reference/ark_server.md | 3 +- pkg/cmd/server/server.go | 56 ++++++++++++++++++++++++++++++-- pkg/cmd/util/flag/enum.go | 15 +++++---- 3 files changed, 64 insertions(+), 10 deletions(-) diff --git a/docs/cli-reference/ark_server.md b/docs/cli-reference/ark_server.md index da18fe925..ef770c248 100644 --- a/docs/cli-reference/ark_server.md +++ b/docs/cli-reference/ark_server.md @@ -14,7 +14,8 @@ ark server [flags] ### Options ``` - -h, --help help for server + -h, --help help for server + --log-level the level at which to log. Valid values are debug, info, warning, error, fatal, panic. (default info) ``` ### Options inherited from parent commands diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 5692d6c04..6f67b6ea8 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -22,6 +22,8 @@ import ( "io/ioutil" "os" "reflect" + "sort" + "strings" "sync" "time" @@ -48,6 +50,7 @@ import ( "github.com/heptio/ark/pkg/cloudprovider/azure" "github.com/heptio/ark/pkg/cloudprovider/gcp" "github.com/heptio/ark/pkg/cmd" + "github.com/heptio/ark/pkg/cmd/util/flag" "github.com/heptio/ark/pkg/controller" arkdiscovery "github.com/heptio/ark/pkg/discovery" "github.com/heptio/ark/pkg/generated/clientset" @@ -60,15 +63,30 @@ import ( ) func NewCommand() *cobra.Command { - var kubeconfig string + var ( + kubeconfig string + sortedLogLevels = getSortedLogLevels() + logLevelFlag = flag.NewEnum(logrus.InfoLevel.String(), sortedLogLevels...) + ) var command = &cobra.Command{ Use: "server", Short: "Run the ark server", Long: "Run the ark server", Run: func(c *cobra.Command, args []string) { - logger := logrus.New() - logger.Hooks.Add(&logging.ErrorLocationHook{}) + logLevel := logrus.InfoLevel + + if parsed, err := logrus.ParseLevel(logLevelFlag.String()); err == nil { + logLevel = parsed + } else { + // This should theoretically never happen assuming the enum flag + // is constructed correctly because the enum flag will not allow + // an invalid value to be set. + logrus.Errorf("log-level flag has invalid value %s", strings.ToUpper(logLevelFlag.String())) + } + logrus.Infof("setting log-level to %s", strings.ToUpper(logLevel.String())) + + logger := newLogger(logLevel, &logging.ErrorLocationHook{}) s, err := newServer(kubeconfig, fmt.Sprintf("%s-%s", c.Parent().Name(), c.Name()), logger) @@ -78,11 +96,43 @@ func NewCommand() *cobra.Command { }, } + command.Flags().Var(logLevelFlag, "log-level", fmt.Sprintf("the level at which to log. Valid values are %s.", strings.Join(sortedLogLevels, ", "))) command.Flags().StringVar(&kubeconfig, "kubeconfig", "", "Path to the kubeconfig file to use to talk to the Kubernetes apiserver. If unset, try the environment variable KUBECONFIG, as well as in-cluster configuration") return command } +func newLogger(level logrus.Level, hooks ...logrus.Hook) *logrus.Logger { + logger := logrus.New() + logger.Level = level + + for _, hook := range hooks { + logger.Hooks.Add(hook) + } + + return logger +} + +// getSortedLogLevels returns a string slice containing all of the valid logrus +// log levels (based on logrus.AllLevels), sorted in ascending order of severity. +func getSortedLogLevels() []string { + var ( + sortedLogLevels = make([]logrus.Level, len(logrus.AllLevels)) + logLevelsStrings []string + ) + + copy(sortedLogLevels, logrus.AllLevels) + + // logrus.Panic has the lowest value, so the compare function uses ">" + sort.Slice(sortedLogLevels, func(i, j int) bool { return sortedLogLevels[i] > sortedLogLevels[j] }) + + for _, level := range sortedLogLevels { + logLevelsStrings = append(logLevelsStrings, level.String()) + } + + return logLevelsStrings +} + type server struct { kubeClient kubernetes.Interface arkClient clientset.Interface diff --git a/pkg/cmd/util/flag/enum.go b/pkg/cmd/util/flag/enum.go index 2c02ef248..dbe85c0f7 100644 --- a/pkg/cmd/util/flag/enum.go +++ b/pkg/cmd/util/flag/enum.go @@ -31,12 +31,12 @@ type Enum struct { } // NewEnum returns a new enum flag with the specified list -// of allowed values. The first value specified is used -// as the default. -func NewEnum(allowedValues ...string) Enum { - return Enum{ +// of allowed values, and the specified default value if +// none is set. +func NewEnum(defaultValue string, allowedValues ...string) *Enum { + return &Enum{ allowedValues: sets.NewString(allowedValues...), - value: allowedValues[0], + value: defaultValue, } } @@ -61,5 +61,8 @@ func (e *Enum) Set(s string) error { // Type returns a string representation of the // Enum type. func (e *Enum) Type() string { - return "enum" + // we don't want the help text to display anything regarding + // the type because the usage text for the flag should capture + // the possible options. + return "" } From e7c62b5f382dd1305197b7e01803f411dad7725f Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Thu, 5 Oct 2017 14:58:04 -0700 Subject: [PATCH 3/4] add file/line numbers to logs Signed-off-by: Steve Kriss --- pkg/cmd/server/server.go | 2 +- pkg/util/logging/log_location_hook.go | 68 +++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 pkg/util/logging/log_location_hook.go diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 6f67b6ea8..0e8bb08fa 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -86,7 +86,7 @@ func NewCommand() *cobra.Command { } logrus.Infof("setting log-level to %s", strings.ToUpper(logLevel.String())) - logger := newLogger(logLevel, &logging.ErrorLocationHook{}) + logger := newLogger(logLevel, &logging.ErrorLocationHook{}, &logging.LogLocationHook{}) s, err := newServer(kubeconfig, fmt.Sprintf("%s-%s", c.Parent().Name(), c.Name()), logger) diff --git a/pkg/util/logging/log_location_hook.go b/pkg/util/logging/log_location_hook.go new file mode 100644 index 000000000..784e3499b --- /dev/null +++ b/pkg/util/logging/log_location_hook.go @@ -0,0 +1,68 @@ +/* +Copyright 2017 Heptio Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package logging + +import ( + "fmt" + "runtime" + "strings" + + "github.com/sirupsen/logrus" +) + +const logLocationField = "logSource" + +// LogLocationHook is a logrus hook that attaches location information +// to log entries, i.e. the file and line number of the logrus log call. +type LogLocationHook struct { +} + +func (h *LogLocationHook) Levels() []logrus.Level { + return logrus.AllLevels +} + +func (h *LogLocationHook) Fire(entry *logrus.Entry) error { + pcs := make([]uintptr, 64) + + // skip 2 frames: + // runtime.Callers + // github.com/heptio/ark/pkg/util/logging/(*LogLocationHook).Fire + n := runtime.Callers(2, pcs) + + // re-slice pcs based on the number of entries written + frames := runtime.CallersFrames(pcs[:n]) + + // now traverse up the call stack looking for the first non-logrus + // func, which will be the logrus invoker + var ( + frame runtime.Frame + more = true + ) + + for more { + frame, more = frames.Next() + + if strings.Contains(frame.File, "github.com/sirupsen/logrus") { + continue + } + + entry.Data[logLocationField] = fmt.Sprintf("%s:%d", frame.File, frame.Line) + break + } + + return nil +} From 7c0d9dcfce56ade016c719351721664779f3601b Mon Sep 17 00:00:00 2001 From: Steve Kriss Date: Thu, 5 Oct 2017 14:58:41 -0700 Subject: [PATCH 4/4] add missing licenses Signed-off-by: Steve Kriss --- pkg/util/logging/error_location_hook.go | 18 +++++++++++++++++- pkg/util/logging/error_location_hook_test.go | 16 ++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/pkg/util/logging/error_location_hook.go b/pkg/util/logging/error_location_hook.go index c7bbd3d6c..12faa0fc1 100644 --- a/pkg/util/logging/error_location_hook.go +++ b/pkg/util/logging/error_location_hook.go @@ -1,3 +1,19 @@ +/* +Copyright 2017 Heptio Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package logging import ( @@ -14,7 +30,7 @@ const ( // ErrorLocationHook is a logrus hook that attaches error location information // to log entries if an error is being logged and it has stack-trace information -// (i.e. if it originates from or is wrapped by github.com/pkg/errors) +// (i.e. if it originates from or is wrapped by github.com/pkg/errors). type ErrorLocationHook struct { } diff --git a/pkg/util/logging/error_location_hook_test.go b/pkg/util/logging/error_location_hook_test.go index 3f99eb07a..607b1347b 100644 --- a/pkg/util/logging/error_location_hook_test.go +++ b/pkg/util/logging/error_location_hook_test.go @@ -1,3 +1,19 @@ +/* +Copyright 2017 Heptio Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package logging import (