diff --git a/pkg/oc/admin/diagnostics/client.go b/pkg/oc/admin/diagnostics/client.go index cb6f10bffd35..4730a5ef7f60 100644 --- a/pkg/oc/admin/diagnostics/client.go +++ b/pkg/oc/admin/diagnostics/client.go @@ -19,14 +19,14 @@ func availableClientDiagnostics() types.DiagnosticList { } // buildClientDiagnostics builds client Diagnostic objects based on the rawConfig passed in. -// Returns the Diagnostics built, "ok" bool for whether to proceed or abort, and an error if any was encountered during the building of diagnostics.) { -func (o DiagnosticsOptions) buildClientDiagnostics(rawConfig *clientcmdapi.Config) ([]types.Diagnostic, bool, error) { +// Returns the Diagnostics built, and any fatal errors encountered during the building of diagnostics. +func (o DiagnosticsOptions) buildClientDiagnostics(rawConfig *clientcmdapi.Config) ([]types.Diagnostic, error) { available := availableClientDiagnostics().Names() networkClient, err := o.Factory.OpenshiftInternalNetworkClient() kubeClient, clientErr := o.Factory.ClientSet() if clientErr != nil || err != nil { - o.Logger.Notice("CED0001", "Could not configure a client, so client diagnostics are limited to testing configuration and connection") + o.Logger().Notice("CED0001", "Could not configure a client, so client diagnostics are limited to testing configuration and connection") available = sets.NewString(clientdiags.ConfigContextsName) } @@ -66,8 +66,8 @@ func (o DiagnosticsOptions) buildClientDiagnostics(rawConfig *clientcmdapi.Confi nd.PreventModification = o.PreventModification diagnostics = append(diagnostics, nd) default: - return nil, false, fmt.Errorf("unknown diagnostic: %v", diagnosticName) + return nil, fmt.Errorf("unknown diagnostic: %v", diagnosticName) } } - return diagnostics, true, clientErr + return diagnostics, clientErr } diff --git a/pkg/oc/admin/diagnostics/cluster.go b/pkg/oc/admin/diagnostics/cluster.go index 87c9c3a4ba38..8d5199e60d11 100644 --- a/pkg/oc/admin/diagnostics/cluster.go +++ b/pkg/oc/admin/diagnostics/cluster.go @@ -43,52 +43,50 @@ func availableClusterDiagnostics() types.DiagnosticList { } // buildClusterDiagnostics builds cluster Diagnostic objects if a cluster-admin client can be extracted from the rawConfig passed in. -// Returns the Diagnostics built, "ok" bool for whether to proceed or abort, and an error if any was encountered during the building of diagnostics.) { -func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Config) ([]types.Diagnostic, bool, error) { +// Returns the Diagnostics built and any fatal error encountered during the building of diagnostics. +func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Config) ([]types.Diagnostic, error) { requestedDiagnostics := availableClusterDiagnostics().Names().Intersection(sets.NewString(o.RequestedDiagnostics.List()...)).List() if len(requestedDiagnostics) == 0 { // no diagnostics to run here - return nil, true, nil // don't waste time on discovery + return nil, nil // don't waste time on discovery } - var ( - kclusterClient kclientset.Interface - ) + var kclusterClient kclientset.Interface - config, kclusterClient, found, serverUrl, err := o.findClusterClients(rawConfig) - if !found { - o.Logger.Notice("CED1002", "Could not configure a client with cluster-admin permissions for the current server, so cluster diagnostics will be skipped") - return nil, true, err + config, kclusterClient, serverUrl, err := o.findClusterClients(rawConfig) + if config == nil { + o.Logger().Notice("CED1002", "Could not configure a client with cluster-admin permissions for the current server, so cluster diagnostics will be skipped") + return nil, nil } if err != nil { - return nil, false, err + return nil, err } imageClient, err := imageclient.NewForConfig(config) if err != nil { - return nil, false, err + return nil, err } projectClient, err := projectclient.NewForConfig(config) if err != nil { - return nil, false, err + return nil, err } routeClient, err := routeclient.NewForConfig(config) if err != nil { - return nil, false, err + return nil, err } appsClient, err := appsclient.NewForConfig(config) if err != nil { - return nil, false, err + return nil, err } oauthClient, err := oauthclient.NewForConfig(config) if err != nil { - return nil, false, err + return nil, err } oauthorizationClient, err := oauthorizationclient.NewForConfig(config) if err != nil { - return nil, false, err + return nil, err } securityClient, err := securityclient.NewForConfig(config) if err != nil { - return nil, false, err + return nil, err } diagnostics := []types.Diagnostic{} @@ -116,64 +114,68 @@ func (o DiagnosticsOptions) buildClusterDiagnostics(rawConfig *clientcmdapi.Conf case clustdiags.RouteCertificateValidationName: d = &clustdiags.RouteCertificateValidation{SARClient: kclusterClient.Authorization(), RESTConfig: config} default: - return nil, false, fmt.Errorf("unknown diagnostic: %v", diagnosticName) + return nil, fmt.Errorf("unknown diagnostic: %v", diagnosticName) } diagnostics = append(diagnostics, d) } - return diagnostics, true, nil + return diagnostics, nil } // attempts to find which context in the config might be a cluster-admin for the server in the current context. -func (o DiagnosticsOptions) findClusterClients(rawConfig *clientcmdapi.Config) (*rest.Config, kclientset.Interface, bool, string, error) { +// returns config for the context chosen, kclusterClient for same, serverUrl of same, and any fatal error +func (o DiagnosticsOptions) findClusterClients(rawConfig *clientcmdapi.Config) (*rest.Config, kclientset.Interface, string, error) { if o.ClientClusterContext != "" { // user has specified cluster context to use - if context, exists := rawConfig.Contexts[o.ClientClusterContext]; !exists { + context, exists := rawConfig.Contexts[o.ClientClusterContext] + if !exists { configErr := fmt.Errorf("Specified '%s' as cluster-admin context, but it was not found in your client configuration.", o.ClientClusterContext) - o.Logger.Error("CED1003", configErr.Error()) - return nil, nil, false, "", configErr - } else if config, kube, found, serverUrl, err := o.makeClusterClients(rawConfig, o.ClientClusterContext, context); found { - return config, kube, true, serverUrl, err - } else { - return nil, nil, false, "", err + o.Logger().Error("CED1003", configErr.Error()) + return nil, nil, "", configErr } + config, kube, serverUrl, err := o.makeClusterClients(rawConfig, o.ClientClusterContext, context) + if err != nil || config == nil { + return nil, nil, "", err + } + return config, kube, serverUrl, nil } currentContext, exists := rawConfig.Contexts[rawConfig.CurrentContext] if !exists { // config specified cluster admin context that doesn't exist; complain and quit configErr := fmt.Errorf("Current context '%s' not found in client configuration; will not attempt cluster diagnostics.", rawConfig.CurrentContext) - o.Logger.Error("CED1004", configErr.Error()) - return nil, nil, false, "", configErr + o.Logger().Error("CED1004", configErr.Error()) + return nil, nil, "", configErr } // check if current context is already cluster admin - if config, kube, found, serverUrl, err := o.makeClusterClients(rawConfig, rawConfig.CurrentContext, currentContext); found { - return config, kube, true, serverUrl, err + config, kube, serverUrl, err := o.makeClusterClients(rawConfig, rawConfig.CurrentContext, currentContext) + if err == nil && config != nil { + return config, kube, serverUrl, nil } // otherwise, for convenience, search for a context with the same server but with the system:admin user for name, context := range rawConfig.Contexts { if context.Cluster == currentContext.Cluster && name != rawConfig.CurrentContext && strings.HasPrefix(context.AuthInfo, "system:admin/") { - if config, kube, found, serverUrl, err := o.makeClusterClients(rawConfig, name, context); found { - return config, kube, true, serverUrl, err - } else { - return nil, nil, false, "", err // don't try more than one such context, they'll probably fail the same + config, kube, serverUrl, err := o.makeClusterClients(rawConfig, name, context) + if err != nil || config == nil { + break // don't try more than one such context, they'll probably fail the same } + return config, kube, serverUrl, nil } } - return nil, nil, false, "", nil + return nil, nil, "", nil } // makes the client from the specified context and determines whether it is a cluster-admin. -func (o DiagnosticsOptions) makeClusterClients(rawConfig *clientcmdapi.Config, contextName string, context *clientcmdapi.Context) (*rest.Config, kclientset.Interface, bool, string, error) { +func (o DiagnosticsOptions) makeClusterClients(rawConfig *clientcmdapi.Config, contextName string, context *clientcmdapi.Context) (*rest.Config, kclientset.Interface, string, error) { overrides := &clientcmd.ConfigOverrides{Context: *context} clientConfig := clientcmd.NewDefaultClientConfig(*rawConfig, overrides) serverUrl := rawConfig.Clusters[context.Cluster].Server factory := osclientcmd.NewFactory(clientConfig) config, err := factory.ClientConfig() if err != nil { - o.Logger.Debug("CED1006", fmt.Sprintf("Error creating client for context '%s':\n%v", contextName, err)) - return nil, nil, false, "", nil + o.Logger().Debug("CED1006", fmt.Sprintf("Error creating client for context '%s':\n%v", contextName, err)) + return nil, nil, "", nil } - o.Logger.Debug("CED1005", fmt.Sprintf("Checking if context is cluster-admin: '%s'", contextName)) + o.Logger().Debug("CED1005", fmt.Sprintf("Checking if context is cluster-admin: '%s'", contextName)) if kubeClient, err := factory.ClientSet(); err != nil { - o.Logger.Debug("CED1006", fmt.Sprintf("Error creating client for context '%s':\n%v", contextName, err)) - return nil, nil, false, "", nil + o.Logger().Debug("CED1006", fmt.Sprintf("Error creating client for context '%s':\n%v", contextName, err)) + return nil, nil, "", nil } else { subjectAccessReview := &authorization.SelfSubjectAccessReview{ Spec: authorization.SelfSubjectAccessReviewSpec{ @@ -187,17 +189,17 @@ func (o DiagnosticsOptions) makeClusterClients(rawConfig *clientcmdapi.Config, c } if resp, err := kubeClient.Authorization().SelfSubjectAccessReviews().Create(subjectAccessReview); err != nil { if regexp.MustCompile(`User "[\w:]+" cannot create \w+ at the cluster scope`).MatchString(err.Error()) { - o.Logger.Debug("CED1007", fmt.Sprintf("Context '%s' does not have cluster-admin access:\n%v", contextName, err)) - return nil, nil, false, "", nil + o.Logger().Debug("CED1007", fmt.Sprintf("Context '%s' does not have cluster-admin access:\n%v", contextName, err)) + return nil, nil, "", nil } else { - o.Logger.Error("CED1008", fmt.Sprintf("Unknown error testing cluster-admin access for context '%s':\n%v", contextName, err)) - return nil, nil, false, "", err + o.Logger().Error("CED1008", fmt.Sprintf("Unknown error testing cluster-admin access for context '%s':\n%v", contextName, err)) + return nil, nil, "", err } } else if resp.Status.Allowed { - o.Logger.Info("CED1009", fmt.Sprintf("Using context for cluster-admin access: '%s'", contextName)) - return config, kubeClient, true, serverUrl, nil + o.Logger().Info("CED1009", fmt.Sprintf("Using context for cluster-admin access: '%s'", contextName)) + return config, kubeClient, serverUrl, nil } } - o.Logger.Debug("CED1010", fmt.Sprintf("Context does not have cluster-admin access: '%s'", contextName)) - return nil, nil, false, "", nil + o.Logger().Debug("CED1010", fmt.Sprintf("Context does not have cluster-admin access: '%s'", contextName)) + return nil, nil, "", nil } diff --git a/pkg/oc/admin/diagnostics/config.go b/pkg/oc/admin/diagnostics/config.go index 6c2d614711cf..786c0664f8db 100644 --- a/pkg/oc/admin/diagnostics/config.go +++ b/pkg/oc/admin/diagnostics/config.go @@ -2,28 +2,17 @@ package diagnostics import ( "errors" + "fmt" + "io/ioutil" + "os" + "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "github.com/openshift/origin/pkg/client/config" - clientdiagnostics "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/client" - "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/types" + "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/util" ) -// determine if we even have a client config -func (o DiagnosticsOptions) detectClientConfig() (bool, bool, []types.DiagnosticError, []types.DiagnosticError) { - if o.ClientFlags == nil { - return false, false, []types.DiagnosticError{}, []types.DiagnosticError{} - } - diagnostic := &clientdiagnostics.ConfigLoading{ConfFlagName: config.OpenShiftConfigFlagName, ClientFlags: o.ClientFlags} - o.Logger.Notice("CED2011", "Determining if client configuration exists for client/cluster diagnostics") - result := diagnostic.Check() - for _, entry := range result.Logs() { - o.Logger.LogEntry(entry) - } - return true, diagnostic.SuccessfulLoad(), result.Warnings(), result.Errors() -} - // use the base factory to return a raw config (not specific to a context) func (o DiagnosticsOptions) buildRawConfig() (*clientcmdapi.Config, error) { kubeConfig, configErr := o.Factory.OpenShiftClientConfig().RawConfig() @@ -35,3 +24,117 @@ func (o DiagnosticsOptions) buildRawConfig() (*clientcmdapi.Config, error) { } return &kubeConfig, nil } + +// determine if we even have a client config +func (o DiagnosticsOptions) detectClientConfig() (expected bool, detected bool) { + if o.ClientFlags == nil { + // options for client not provided, so it must not be expected. + return false, false + } + o.Logger().Notice("CED2011", "Determining if client configuration exists for client/cluster diagnostics") + confFlagName := config.OpenShiftConfigFlagName + confFlagValue := o.ClientFlags.Lookup(confFlagName).Value.String() + successfulLoad := false + + var foundPath string + rules := config.NewOpenShiftClientConfigLoadingRules() + paths := append([]string{confFlagValue}, rules.Precedence...) + for index, path := range paths { + errmsg := "" + switch index { + case 0: + errmsg = fmt.Sprintf("--%s specified that client config should be at %s\n", confFlagName, path) + case len(paths) - 1: // config in ~/.kube + // no error message indicated if it is not there... user didn't say it would be + default: // can be multiple paths from the env var in theory; all cases should go here + if len(os.Getenv(config.OpenShiftConfigPathEnvVar)) != 0 { + errmsg = fmt.Sprintf("Env var %s specified that client config could be at %s\n", config.OpenShiftConfigPathEnvVar, path) + } + } + + if o.canOpenConfigFile(path, errmsg) && foundPath == "" { + successfulLoad = true + foundPath = path + } + } + if foundPath != "" { + if confFlagValue != "" && confFlagValue != foundPath { + // found config but not where --config said + o.Logger().Error("DCli1001", fmt.Sprintf(` +The client configuration file was not found where the --%s flag indicated: + %s +A config file was found at the following location: + %s +If you wish to use this file for client configuration, you can specify it +with the --%[1]s flag, or just not specify the flag. + `, confFlagName, confFlagValue, foundPath)) + } + } else { // not found, check for master-generated ones to recommend + if confFlagValue != "" { + o.Logger().Error("DCli1002", fmt.Sprintf("Did not find config file where --%s=%s indicated", confFlagName, confFlagValue)) + } + adminWarningF := ` +No client config file was available; however, one exists at + %[2]s +which may have been generated automatically by the master. +If you want to use this config, you should copy it to the +standard location (%[3]s), +or you can set the environment variable %[1]s: + export %[1]s=%[2]s +If not, obtain a config file and place it in the standard +location for use by the client and diagnostics. +` + // look for it in auto-generated locations when not found properly + for _, path := range util.AdminKubeConfigPaths { + msg := fmt.Sprintf("Looking for a possible client config at %s\n", path) + if o.canOpenConfigFile(path, msg) { + o.Logger().Warn("DCli1003", fmt.Sprintf(adminWarningF, config.OpenShiftConfigPathEnvVar, path, config.RecommendedHomeFile)) + break + } + } + } + return true, successfulLoad +} + +// ---------------------------------------------------------- +// Attempt to open file at path as client config +// If there is a problem and errmsg is set, log an error +func (o DiagnosticsOptions) canOpenConfigFile(path string, errmsg string) bool { + var file *os.File + var err error + if path == "" { // empty param/envvar + return false + } else if file, err = os.Open(path); err == nil { + o.Logger().Debug("DCli1004", fmt.Sprintf("Reading client config at %s", path)) + } else if errmsg == "" { + o.Logger().Debug("DCli1005", fmt.Sprintf("Could not read client config at %s:\n%#v", path, err)) + } else if os.IsNotExist(err) { + o.Logger().Debug("DCli1006", errmsg+"but that file does not exist.") + } else if os.IsPermission(err) { + o.Logger().Error("DCli1007", errmsg+"but lack permission to read that file.") + } else { + o.Logger().Error("DCli1008", fmt.Sprintf("%sbut there was an error opening it:\n%#v", errmsg, err)) + } + if file != nil { // it is open for reading + defer file.Close() + if buffer, err := ioutil.ReadAll(file); err != nil { + o.Logger().Error("DCli1009", fmt.Sprintf("Unexpected error while reading client config file (%s): %v", path, err)) + } else if _, err := clientcmd.Load(buffer); err != nil { + o.Logger().Error("DCli1010", fmt.Sprintf(` +Error reading YAML from client config file (%s): + %v +This file may have been truncated or mis-edited. +Please fix, remove, or obtain a new client config`, file.Name(), err)) + } else { + o.Logger().Info("DCli1011", fmt.Sprintf("Successfully read a client config file at '%s'", path)) + /* Note, we're not going to use this config file directly. + * Instead, we'll defer to the openshift client code to assimilate + * flags, env vars, and the potential hierarchy of config files + * into an actual configuration that the client uses. + * However, for diagnostic purposes, record the files we find. + */ + return true + } + } + return false +} diff --git a/pkg/oc/admin/diagnostics/diagnostics.go b/pkg/oc/admin/diagnostics/diagnostics.go index 50dc2445f314..02a63fe8f4b3 100644 --- a/pkg/oc/admin/diagnostics/diagnostics.go +++ b/pkg/oc/admin/diagnostics/diagnostics.go @@ -20,6 +20,7 @@ import ( "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/log" "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/types" "github.com/openshift/origin/pkg/oc/admin/diagnostics/options" + "github.com/openshift/origin/pkg/oc/admin/diagnostics/util" osclientcmd "github.com/openshift/origin/pkg/oc/cli/util/clientcmd" ) @@ -49,8 +50,8 @@ type DiagnosticsOptions struct { ClientClusterContext string // LogOptions determine globally what the user wants to see and how. LogOptions *log.LoggerOptions - // The Logger is built with the options and should be used for all diagnostic output. - Logger *log.Logger + // The logger is built with the options and should be used for all diagnostic output. + logger *log.Logger } const ( @@ -104,7 +105,7 @@ var ( `) ) -// NewCmdDiagnostics is the base command for running any diagnostics. +// NewCmdDiagnostics is the base command for running a standard set of diagnostics with generic options only. func NewCmdDiagnostics(name string, fullName string, out io.Writer) *cobra.Command { available := availableDiagnostics() o := &DiagnosticsOptions{ @@ -117,18 +118,7 @@ func NewCmdDiagnostics(name string, fullName string, out io.Writer) *cobra.Comma Use: name, Short: "Diagnose common cluster problems", Long: fmt.Sprintf(longDescription, fullName), - Run: func(c *cobra.Command, args []string) { - kcmdutil.CheckErr(o.Complete(c, args)) - - failed, err, warnCount, errorCount := o.RunDiagnostics() - o.Logger.Summary(warnCount, errorCount) - - kcmdutil.CheckErr(err) - if failed { - os.Exit(1) - } - - }, + Run: commandRunFunc(o), } cmd.SetOutput(out) // for output re: usage / help o.bindCommonFlags(cmd.Flags()) @@ -157,18 +147,7 @@ func NewCmdDiagnosticsAll(name string, fullName string, out io.Writer, available Use: name, Short: "Diagnose common cluster problems", Long: fmt.Sprintf(longDescriptionAll, fullName), - Run: func(c *cobra.Command, args []string) { - kcmdutil.CheckErr(o.Complete(c, args)) - - failed, err, warnCount, errorCount := o.RunDiagnostics() - o.Logger.Summary(warnCount, errorCount) - - kcmdutil.CheckErr(err) - if failed { - os.Exit(1) - } - - }, + Run: commandRunFunc(o), } cmd.SetOutput(out) // for output re: usage / help o.bindCommonFlags(cmd.Flags()) @@ -178,7 +157,7 @@ func NewCmdDiagnosticsAll(name string, fullName string, out io.Writer, available return cmd } -// NewCmdDiagnosticsIndividual is a generic subcommand providing a single diagnostic and its flags. +// NewCmdDiagnosticsIndividual is a parameterized subcommand providing a single diagnostic and its flags. func NewCmdDiagnosticsIndividual(name string, fullName string, out io.Writer, diagnostic types.Diagnostic) *cobra.Command { o := &DiagnosticsOptions{ RequestedDiagnostics: sets.NewString(diagnostic.Name()), @@ -187,21 +166,10 @@ func NewCmdDiagnosticsIndividual(name string, fullName string, out io.Writer, di } cmd := &cobra.Command{ - Use: name, - Short: diagnostic.Description(), - Long: fmt.Sprintf(longDescriptionIndividual, diagnostic.Name(), diagnostic.Description()), - Run: func(c *cobra.Command, args []string) { - kcmdutil.CheckErr(o.Complete(c, args)) - - failed, err, warnCount, errorCount := o.RunDiagnostics() - o.Logger.Summary(warnCount, errorCount) - - kcmdutil.CheckErr(err) - if failed { - os.Exit(1) - } - - }, + Use: name, + Short: diagnostic.Description(), + Long: fmt.Sprintf(longDescriptionIndividual, diagnostic.Name(), diagnostic.Description()), + Run: commandRunFunc(o), Aliases: []string{diagnostic.Name()}, } cmd.SetOutput(out) // for output re: usage / help @@ -219,6 +187,32 @@ func NewCmdDiagnosticsIndividual(name string, fullName string, out io.Writer, di return cmd } +type optionsRunner interface { + Logger() *log.Logger + Complete(*cobra.Command, []string) error + BuildAndRunDiagnostics() error +} + +// returns a shared function that runs when one of these Commands actually executes +func commandRunFunc(o optionsRunner) func(c *cobra.Command, args []string) { + return func(c *cobra.Command, args []string) { + kcmdutil.CheckErr(o.Complete(c, args)) + + if err := o.BuildAndRunDiagnostics(); err != nil { + o.Logger().Error("CED3001", fmt.Sprintf("Encountered fatal error while building diagnostics: %s", err.Error())) + } + o.Logger().Summary() + if o.Logger().ErrorsSeen() { + os.Exit(1) + } + } +} + +// returns the logger built according to options (must be Complete()ed) +func (o *DiagnosticsOptions) Logger() *log.Logger { + return o.logger +} + // gather a list of all diagnostics that are available to be invoked by the main command func availableDiagnostics() types.DiagnosticList { available := availableClientDiagnostics() @@ -266,7 +260,7 @@ func (o *DiagnosticsOptions) bindRequestedIndividualFlags(flags *flag.FlagSet) { } } -// bind flags for parameters on a single diagnostic +// bind flags for parameters from a single diagnostic func bindIndividualFlags(diag types.ParameterizedDiagnostic, prefix string, flags *flag.FlagSet) { for _, param := range diag.AvailableParameters() { name := prefix + param.Name @@ -286,7 +280,7 @@ func bindIndividualFlags(diag types.ParameterizedDiagnostic, prefix string, flag // Complete fills in DiagnosticsConfig needed if the command is actually invoked. func (o *DiagnosticsOptions) Complete(c *cobra.Command, args []string) error { var err error - o.Logger, err = o.LogOptions.NewLogger() + o.logger, err = o.LogOptions.NewLogger() if err != nil { return err } @@ -312,130 +306,67 @@ func (o *DiagnosticsOptions) Complete(c *cobra.Command, args []string) error { return nil } -// RunDiagnostics builds diagnostics based on the options and executes them, returning a summary. Returns: -// failure ("true" meaning there was not a clean diagnostic result), -// error (raised during construction or execution of diagnostics; may be an aggregate error object), -// number of warnings encountered in diagnostics results, -// number of errors encountered (diagnostic results plus errors previously raised). -func (o DiagnosticsOptions) RunDiagnostics() (bool, error, int, int) { - failed := false - warnings := []error{} - errors := []error{} - diagnostics := []types.Diagnostic{} - - func() { // don't trust discovery/build of diagnostics; wrap panic nicely in case of developer error - defer func() { - if r := recover(); r != nil { - failed = true - stack := debug.Stack() - errors = append(errors, fmt.Errorf("While building the diagnostics, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%v\n%s", r, stack)) - } - }() +// BuildAndRunDiagnostics builds diagnostics based on the options and executes them. Returns: +// error (raised during construction of diagnostics; may be an aggregate error object), +func (o DiagnosticsOptions) BuildAndRunDiagnostics() error { + diagnostics, failure := o.buildDiagnostics() + if failure != nil { + return failure + } + return util.RunDiagnostics(o.Logger(), diagnostics) +} - // build client/cluster diags if there is a client config for them to use - expected, detected, detectWarnings, detectErrors := o.detectClientConfig() // may log and return problems - for _, warn := range detectWarnings { - warnings = append(warnings, warn) - } - for _, err := range detectErrors { - errors = append(errors, err) - } - if !expected { - // no diagnostic required a client config, nothing to do - } else if !detected { - // there just plain isn't any client config file available - o.Logger.Notice("CED3014", "No client configuration specified; skipping client and cluster diagnostics.") - } else if rawConfig, err := o.buildRawConfig(); err != nil { // client config is totally broken - won't parse etc (problems may have been detected and logged) - o.Logger.Error("CED3015", fmt.Sprintf("Client configuration failed to load; skipping client and cluster diagnostics due to error: %s", err.Error())) - errors = append(errors, err) - } else { - clientDiags, ok, err := o.buildClientDiagnostics(rawConfig) - failed = failed || !ok - if ok { - diagnostics = append(diagnostics, clientDiags...) - } - if err != nil { - errors = append(errors, err) - } +func (o DiagnosticsOptions) buildDiagnostics() (diags []types.Diagnostic, failure error) { + diagnostics := []types.Diagnostic{} - clusterDiags, ok, err := o.buildClusterDiagnostics(rawConfig) - failed = failed || !ok - if ok { - diagnostics = append(diagnostics, clusterDiags...) - } - if err != nil { - errors = append(errors, err) - } + // don't trust discovery/build of diagnostics; wrap panic nicely in case of developer error + defer func() { + if r := recover(); r != nil { + failure = fmt.Errorf("While building the diagnostics, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%v\n%s", r, debug.Stack()) } + }() - // build host diagnostics if config is available - hostDiags, err := o.buildHostDiagnostics() + // build client/cluster diags if there is a client config for them to use + expected, detected := o.detectClientConfig() // may log and return problems + if !expected { + // no diagnostic required a client config, nothing to do + } else if !detected { + // there just plain isn't any client config file available + o.Logger().Notice("CED3014", "No client configuration specified; skipping client and cluster diagnostics.") + } else if rawConfig, err := o.buildRawConfig(); err != nil { // client config is totally broken - won't parse etc (problems may have been detected and logged) + o.Logger().Error("CED3015", fmt.Sprintf("Client configuration failed to load; skipping client and cluster diagnostics due to error: %s", err.Error())) + } else { + clientDiags, err := o.buildClientDiagnostics(rawConfig) if err != nil { - errors = append(errors, err) - } else { - diagnostics = append(diagnostics, hostDiags...) + return diagnostics, err } + diagnostics = append(diagnostics, clientDiags...) - // complete any diagnostics that require it - for _, d := range diagnostics { - if toComplete, ok := d.(types.IncompleteDiagnostic); ok { - if err := toComplete.Complete(o.Logger); err != nil { - errors = append(errors, err) - failed = true - } - } + clusterDiags, err := o.buildClusterDiagnostics(rawConfig) + if err != nil { + return diagnostics, err } - }() - - if failed { - return failed, kutilerrors.NewAggregate(errors), len(warnings), len(errors) + diagnostics = append(diagnostics, clusterDiags...) } - failed, err, numWarnings, numErrors := o.Run(diagnostics) - numWarnings += len(warnings) - numErrors += len(errors) - return failed, err, numWarnings, numErrors -} - -// Run performs the actual execution of diagnostics once they're built. -func (o DiagnosticsOptions) Run(diagnostics []types.Diagnostic) (bool, error, int, int) { - warnCount := 0 - errorCount := 0 - runCount := 0 - for _, diagnostic := range diagnostics { - func() { // wrap diagnostic panic nicely in case of developer error - defer func() { - if r := recover(); r != nil { - errorCount += 1 - stack := debug.Stack() - o.Logger.Error("CED3017", - fmt.Sprintf("While running the %s diagnostic, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%s\n%s", - diagnostic.Name(), fmt.Sprintf("%v", r), stack)) - } - }() - - if canRun, reason := diagnostic.CanRun(); !canRun { - if reason == nil { - o.Logger.Notice("CED3018", fmt.Sprintf("Skipping diagnostic: %s\nDescription: %s", diagnostic.Name(), diagnostic.Description())) - } else { - o.Logger.Notice("CED3019", fmt.Sprintf("Skipping diagnostic: %s\nDescription: %s\nBecause: %s", diagnostic.Name(), diagnostic.Description(), reason.Error())) - } - return - } + // build host diagnostics if config is available + hostDiags, err := o.buildHostDiagnostics() + if err != nil { + return diagnostics, err + } + diagnostics = append(diagnostics, hostDiags...) - o.Logger.Notice("CED3020", fmt.Sprintf("Running diagnostic: %s\nDescription: %s", diagnostic.Name(), diagnostic.Description())) - r := diagnostic.Check() - for _, entry := range r.Logs() { - o.Logger.LogEntry(entry) + // complete any diagnostics that require it + errors := []error{} + for _, d := range diagnostics { + if toComplete, ok := d.(types.IncompleteDiagnostic); ok { + if err := toComplete.Complete(o.Logger()); err != nil { + errors = append(errors, err) } - warnCount += len(r.Warnings()) - errorCount += len(r.Errors()) - runCount += 1 - }() + } } - if runCount == 0 { - o.Logger.Error("CED3016", "Requested diagnostic(s) skipped; nothing to run. See --help and consider setting flags or providing config to enable running.") - return true, nil, 0, 1 + if len(errors) > 0 { + return diagnostics, kutilerrors.NewAggregate(errors) } - return errorCount > 0, nil, warnCount, errorCount + return diagnostics, nil } diff --git a/pkg/oc/admin/diagnostics/diagnostics/client/config_loading.go b/pkg/oc/admin/diagnostics/diagnostics/client/config_loading.go deleted file mode 100644 index 3ca1dcef5b13..000000000000 --- a/pkg/oc/admin/diagnostics/diagnostics/client/config_loading.go +++ /dev/null @@ -1,150 +0,0 @@ -package client - -import ( - "fmt" - "io/ioutil" - "os" - - flag "github.com/spf13/pflag" - "k8s.io/client-go/tools/clientcmd" - - "github.com/openshift/origin/pkg/client/config" - "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/types" - "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/util" -) - -// ConfigLoading is a little special in that it is run separately as a precondition -// in order to determine whether we can run other dependent diagnostics. -type ConfigLoading struct { - ConfFlagName string - ClientFlags *flag.FlagSet - successfulLoad bool // set if at least one file loaded -} - -// Name is part of the Diagnostic interface and just returns name. -func (d *ConfigLoading) Name() string { - return "ConfigLoading" -} - -// Description is part of the Diagnostic interface and provides a user-focused description of what the diagnostic does. -func (d *ConfigLoading) Description() string { - return "Try to load client config file(s) and report what happens" -} - -// CanRun is part of the Diagnostic interface; it determines if the conditions are right to run this diagnostic. -func (d *ConfigLoading) CanRun() (bool, error) { - return true, nil -} - -// SuccessfulLoad returns whether the client config was found -func (d *ConfigLoading) SuccessfulLoad() bool { - return d.successfulLoad -} - -// Check is part of the Diagnostic interface; it runs the actual diagnostic logic -func (d *ConfigLoading) Check() types.DiagnosticResult { - r := types.NewDiagnosticResult("ConfigLoading") - confFlagValue := d.ClientFlags.Lookup(d.ConfFlagName).Value.String() - - var foundPath string - rules := config.NewOpenShiftClientConfigLoadingRules() - paths := append([]string{confFlagValue}, rules.Precedence...) - for index, path := range paths { - errmsg := "" - switch index { - case 0: - errmsg = fmt.Sprintf("--%s specified that client config should be at %s\n", d.ConfFlagName, path) - case len(paths) - 1: // config in ~/.kube - // no error message indicated if it is not there... user didn't say it would be - default: // can be multiple paths from the env var in theory; all cases should go here - if len(os.Getenv(config.OpenShiftConfigPathEnvVar)) != 0 { - errmsg = fmt.Sprintf("Env var %s specified that client config could be at %s\n", config.OpenShiftConfigPathEnvVar, path) - } - } - - if d.canOpenConfigFile(path, errmsg, r) && foundPath == "" { - d.successfulLoad = true - foundPath = path - } - } - if foundPath != "" { - if confFlagValue != "" && confFlagValue != foundPath { - // found config but not where --config said - r.Error("DCli1001", nil, fmt.Sprintf(` -The client configuration file was not found where the --%s flag indicated: - %s -A config file was found at the following location: - %s -If you wish to use this file for client configuration, you can specify it -with the --%[1]s flag, or just not specify the flag. - `, d.ConfFlagName, confFlagValue, foundPath)) - } - } else { // not found, check for master-generated ones to recommend - if confFlagValue != "" { - r.Error("DCli1002", nil, fmt.Sprintf("Did not find config file where --%s=%s indicated", d.ConfFlagName, confFlagValue)) - } - adminWarningF := ` -No client config file was available; however, one exists at - %[2]s -which may have been generated automatically by the master. -If you want to use this config, you should copy it to the -standard location (%[3]s), -or you can set the environment variable %[1]s: - export %[1]s=%[2]s -If not, obtain a config file and place it in the standard -location for use by the client and diagnostics. -` - // look for it in auto-generated locations when not found properly - for _, path := range util.AdminKubeConfigPaths { - msg := fmt.Sprintf("Looking for a possible client config at %s\n", path) - if d.canOpenConfigFile(path, msg, r) { - r.Warn("DCli1003", nil, fmt.Sprintf(adminWarningF, config.OpenShiftConfigPathEnvVar, path, config.RecommendedHomeFile)) - break - } - } - } - return r -} - -// ---------------------------------------------------------- -// Attempt to open file at path as client config -// If there is a problem and errmsg is set, log an error -func (d ConfigLoading) canOpenConfigFile(path string, errmsg string, r types.DiagnosticResult) bool { - var file *os.File - var err error - if path == "" { // empty param/envvar - return false - } else if file, err = os.Open(path); err == nil { - r.Debug("DCli1004", fmt.Sprintf("Reading client config at %s", path)) - } else if errmsg == "" { - r.Debug("DCli1005", fmt.Sprintf("Could not read client config at %s:\n%#v", path, err)) - } else if os.IsNotExist(err) { - r.Debug("DCli1006", errmsg+"but that file does not exist.") - } else if os.IsPermission(err) { - r.Error("DCli1007", err, errmsg+"but lack permission to read that file.") - } else { - r.Error("DCli1008", err, fmt.Sprintf("%sbut there was an error opening it:\n%#v", errmsg, err)) - } - if file != nil { // it is open for reading - defer file.Close() - if buffer, err := ioutil.ReadAll(file); err != nil { - r.Error("DCli1009", err, fmt.Sprintf("Unexpected error while reading client config file (%s): %v", path, err)) - } else if _, err := clientcmd.Load(buffer); err != nil { - r.Error("DCli1010", err, fmt.Sprintf(` -Error reading YAML from client config file (%s): - %v -This file may have been truncated or mis-edited. -Please fix, remove, or obtain a new client config`, file.Name(), err)) - } else { - r.Info("DCli1011", fmt.Sprintf("Successfully read a client config file at '%s'", path)) - /* Note, we're not going to use this config file directly. - * Instead, we'll defer to the openshift client code to assimilate - * flags, env vars, and the potential hierarchy of config files - * into an actual configuration that the client uses. - * However, for diagnostic purposes, record the files we find. - */ - return true - } - } - return false -} diff --git a/pkg/oc/admin/diagnostics/diagnostics/log/log.go b/pkg/oc/admin/diagnostics/diagnostics/log/log.go index 034f6b0d1dbe..e794ef0457cd 100644 --- a/pkg/oc/admin/diagnostics/diagnostics/log/log.go +++ b/pkg/oc/admin/diagnostics/diagnostics/log/log.go @@ -93,23 +93,33 @@ var ( ) // Provide a summary at the end -func (l *Logger) Summary(warningsSeen int, errorsSeen int) { +func (l *Logger) Summary() { l.Notice("DL0001", fmt.Sprintf("Summary of diagnostics execution (version %v):\n", version.Get())) - if warningsSeen > 0 { - l.Notice("DL0002", fmt.Sprintf("Warnings seen: %d", warningsSeen)) + if l.warningsSeen > 0 { + l.Notice("DL0002", fmt.Sprintf("Warnings seen: %d", l.warningsSeen)) } - if errorsSeen > 0 { - l.Notice("DL0003", fmt.Sprintf("Errors seen: %d", errorsSeen)) + if l.errorsSeen > 0 { + l.Notice("DL0003", fmt.Sprintf("Errors seen: %d", l.errorsSeen)) } - if warningsSeen == 0 && errorsSeen == 0 { + if l.warningsSeen == 0 && l.errorsSeen == 0 { l.Notice("DL0004", "Completed with no errors or warnings seen.") } } +func (l *Logger) ErrorsSeen() bool { + return l.errorsSeen > 0 +} + func (l *Logger) LogEntry(entry Entry) { if l == nil { // if there's no logger, return silently return } + if entry.Level == ErrorLevel { + l.errorsSeen++ + } + if entry.Level == WarnLevel { + l.warningsSeen++ + } if entry.Level.Level < l.level.Level { // logging level says skip this entry return } diff --git a/pkg/oc/admin/diagnostics/host.go b/pkg/oc/admin/diagnostics/host.go index 4eaf1a4d7056..962961168f12 100644 --- a/pkg/oc/admin/diagnostics/host.go +++ b/pkg/oc/admin/diagnostics/host.go @@ -17,7 +17,7 @@ var ( ) ) -// availableHostDiagnostics contains the names of host diagnostics that can be executed +// availableHostDiagnostics returns host diagnostics that can be executed // during a single run of diagnostics. Add more diagnostics to the list as they are defined. func availableHostDiagnostics() types.DiagnosticList { return types.DiagnosticList{ @@ -30,7 +30,7 @@ func availableHostDiagnostics() types.DiagnosticList { } // buildHostDiagnostics builds host Diagnostic objects based on the host environment. -// Returns the Diagnostics built, and an error if any was encountered during the building of diagnostics.) { +// Returns the Diagnostics built, and an error if any was encountered during the building of diagnostics. func (o DiagnosticsOptions) buildHostDiagnostics() ([]types.Diagnostic, error) { requestedDiagnostics := availableHostDiagnostics().Names().Intersection(sets.NewString(o.RequestedDiagnostics.List()...)).List() if len(requestedDiagnostics) == 0 { // no diagnostics to run here @@ -47,7 +47,7 @@ func (o DiagnosticsOptions) buildHostDiagnostics() ([]types.Diagnostic, error) { } diagnostics := []types.Diagnostic{} - systemdUnits := systemddiags.GetSystemdUnits(o.Logger) + systemdUnits := systemddiags.GetSystemdUnits(o.Logger()) for _, diagnosticName := range requestedDiagnostics { var d types.Diagnostic switch diagnosticName { diff --git a/pkg/oc/admin/diagnostics/network_pod.go b/pkg/oc/admin/diagnostics/network_pod.go index 01df1f1345d1..2fd322fdbb6c 100644 --- a/pkg/oc/admin/diagnostics/network_pod.go +++ b/pkg/oc/admin/diagnostics/network_pod.go @@ -3,16 +3,13 @@ package diagnostics import ( "fmt" "io" - "os" "runtime/debug" "github.com/spf13/cobra" flag "github.com/spf13/pflag" - kutilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/kubernetes/pkg/kubectl/cmd/templates" - kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util" "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/log" networkdiag "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/networkpod" @@ -30,7 +27,7 @@ type NetworkPodDiagnosticsOptions struct { // LogOptions determine globally what the user wants to see and how. LogOptions *log.LoggerOptions // The Logger is built with the options and should be used for all diagnostic output. - Logger *log.Logger + logger *log.Logger } var longNetworkPodDiagDescription = templates.LongDesc(` @@ -55,18 +52,7 @@ func NewCommandNetworkPodDiagnostics(name string, out io.Writer) *cobra.Command Use: name, Short: "Within a privileged pod, run network diagnostics", Long: fmt.Sprintf(longNetworkPodDiagDescription), - Run: func(c *cobra.Command, args []string) { - kcmdutil.CheckErr(o.Complete(args)) - - failed, err, warnCount, errorCount := o.BuildAndRunDiagnostics() - o.Logger.Summary(warnCount, errorCount) - - kcmdutil.CheckErr(err) - if failed { - os.Exit(255) - } - - }, + Run: commandRunFunc(o), } cmd.SetOutput(out) // for output re: usage / help @@ -75,9 +61,14 @@ func NewCommandNetworkPodDiagnostics(name string, out io.Writer) *cobra.Command return cmd } +// Logger returns the logger built according to options (must be Complete()ed) +func (o *NetworkPodDiagnosticsOptions) Logger() *log.Logger { + return o.logger +} + // Complete fills in NetworkPodDiagnosticsOptions needed if the command is actually invoked. -func (o *NetworkPodDiagnosticsOptions) Complete(args []string) (err error) { - o.Logger, err = o.LogOptions.NewLogger() +func (o *NetworkPodDiagnosticsOptions) Complete(c *cobra.Command, args []string) (err error) { + o.logger, err = o.LogOptions.NewLogger() if err != nil { return err } @@ -91,44 +82,34 @@ func (o *NetworkPodDiagnosticsOptions) Complete(args []string) (err error) { } // BuildAndRunDiagnostics builds diagnostics based on the options and executes them, returning a summary. -func (o NetworkPodDiagnosticsOptions) BuildAndRunDiagnostics() (failed bool, err error, numWarnings, numErrors int) { - failed = false - errors := []error{} +func (o NetworkPodDiagnosticsOptions) BuildAndRunDiagnostics() error { + var fatal error diagnostics := []types.Diagnostic{} func() { // don't trust discovery/build of diagnostics; wrap panic nicely in case of developer error defer func() { if r := recover(); r != nil { - failed = true - stack := debug.Stack() - errors = append(errors, fmt.Errorf("While building the diagnostics, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%v\n%s", r, stack)) + fatal = fmt.Errorf("While building the diagnostics, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%v\n%s", r, debug.Stack()) } }() // deferred panic handler - networkPodDiags, ok, err := o.buildNetworkPodDiagnostics() - failed = failed || !ok - if ok { - diagnostics = append(diagnostics, networkPodDiags...) - } - if err != nil { - errors = append(errors, err...) - } + + diagnostics, fatal = o.buildNetworkPodDiagnostics() }() - if failed { - return failed, kutilerrors.NewAggregate(errors), 0, len(errors) + if fatal != nil { + return fatal } - failed, err, numWarnings, numErrors = util.RunDiagnostics(o.Logger, diagnostics, 0, len(errors)) - return failed, err, numWarnings, numErrors + return util.RunDiagnostics(o.Logger(), diagnostics) } // buildNetworkPodDiagnostics builds network Diagnostic objects based on the host environment. -// Returns the Diagnostics built, "ok" bool for whether to proceed or abort, and an error if any was encountered during the building of diagnostics. -func (o NetworkPodDiagnosticsOptions) buildNetworkPodDiagnostics() ([]types.Diagnostic, bool, []error) { +// Returns the Diagnostics built or any fatal error encountered during the building of diagnostics. +func (o NetworkPodDiagnosticsOptions) buildNetworkPodDiagnostics() ([]types.Diagnostic, error) { diagnostics := []types.Diagnostic{} - err, requestedDiagnostics := util.DetermineRequestedDiagnostics(availableNetworkPodDiagnostics.List(), o.RequestedDiagnostics, o.Logger) + err, requestedDiagnostics := util.DetermineRequestedDiagnostics(availableNetworkPodDiagnostics.List(), o.RequestedDiagnostics, o.Logger()) if err != nil { - return diagnostics, false, []error{err} // don't waste time on discovery + return nil, err // don't waste time on discovery } clientFlags := flag.NewFlagSet("client", flag.ContinueOnError) // hide the extensive set of client flags @@ -136,11 +117,11 @@ func (o NetworkPodDiagnosticsOptions) buildNetworkPodDiagnostics() ([]types.Diag kubeClient, clientErr := factory.ClientSet() if clientErr != nil { - return diagnostics, false, []error{clientErr} + return nil, clientErr } networkClient, err := factory.OpenshiftInternalNetworkClient() if err != nil { - return diagnostics, false, []error{err} + return nil, err } for _, diagnosticName := range requestedDiagnostics { @@ -174,9 +155,9 @@ func (o NetworkPodDiagnosticsOptions) buildNetworkPodDiagnostics() ([]types.Diag }) default: - return diagnostics, false, []error{fmt.Errorf("unknown diagnostic: %v", diagnosticName)} + return diagnostics, fmt.Errorf("unknown diagnostic: %v", diagnosticName) } } - return diagnostics, true, nil + return diagnostics, nil } diff --git a/pkg/oc/admin/diagnostics/pod.go b/pkg/oc/admin/diagnostics/pod.go index d6f0cd37f774..a106005779db 100644 --- a/pkg/oc/admin/diagnostics/pod.go +++ b/pkg/oc/admin/diagnostics/pod.go @@ -3,15 +3,12 @@ package diagnostics import ( "fmt" "io" - "os" "runtime/debug" "github.com/spf13/cobra" - kutilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/kubernetes/pkg/kubectl/cmd/templates" - kcmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util" "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/log" poddiag "github.com/openshift/origin/pkg/oc/admin/diagnostics/diagnostics/pod" @@ -28,7 +25,12 @@ type PodDiagnosticsOptions struct { // LogOptions determine globally what the user wants to see and how. LogOptions *log.LoggerOptions // The Logger is built with the options and should be used for all diagnostic output. - Logger *log.Logger + logger *log.Logger +} + +// returns the logger built according to options (must be Complete()ed) +func (o *PodDiagnosticsOptions) Logger() *log.Logger { + return o.logger } const ( @@ -53,18 +55,7 @@ func NewCommandPodDiagnostics(name string, out io.Writer) *cobra.Command { Use: name, Short: "Within a pod, run pod diagnostics", Long: fmt.Sprintf(longPodDiagDescription), - Run: func(c *cobra.Command, args []string) { - kcmdutil.CheckErr(o.Complete(args)) - - failed, err, warnCount, errorCount := o.BuildAndRunDiagnostics() - o.Logger.Summary(warnCount, errorCount) - - kcmdutil.CheckErr(err) - if failed { - os.Exit(255) - } - - }, + Run: commandRunFunc(o), } cmd.SetOutput(out) // for output re: usage / help @@ -74,9 +65,9 @@ func NewCommandPodDiagnostics(name string, out io.Writer) *cobra.Command { } // Complete fills in PodDiagnosticsOptions needed if the command is actually invoked. -func (o *PodDiagnosticsOptions) Complete(args []string) error { +func (o *PodDiagnosticsOptions) Complete(c *cobra.Command, args []string) error { var err error - o.Logger, err = o.LogOptions.NewLogger() + o.logger, err = o.LogOptions.NewLogger() if err != nil { return err } @@ -89,37 +80,26 @@ func (o *PodDiagnosticsOptions) Complete(args []string) error { return nil } -// BuildAndRunDiagnostics builds diagnostics based on the options and executes them, returning a summary. -func (o PodDiagnosticsOptions) BuildAndRunDiagnostics() (bool, error, int, int) { - failed := false - errors := []error{} - diagnostics := []types.Diagnostic{} +// BuildAndRunDiagnostics builds diagnostics based on the options and executes them, returning fatal error(s) only. +func (o PodDiagnosticsOptions) BuildAndRunDiagnostics() error { + var fatal error + var diagnostics []types.Diagnostic func() { // don't trust discovery/build of diagnostics; wrap panic nicely in case of developer error defer func() { if r := recover(); r != nil { - failed = true - stack := debug.Stack() - errors = append(errors, fmt.Errorf("While building the diagnostics, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%v\n%s", r, stack)) + fatal = fmt.Errorf("While building the diagnostics, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%v\n%s", r, debug.Stack()) } }() // deferred panic handler - podDiags, ok, err := o.buildPodDiagnostics() - failed = failed || !ok - if ok { - diagnostics = append(diagnostics, podDiags...) - } - if err != nil { - errors = append(errors, err...) - } + diagnostics, fatal = o.buildPodDiagnostics() }() - if failed { - return failed, kutilerrors.NewAggregate(errors), 0, len(errors) + if fatal != nil { + return fatal } - failed, err, numWarnings, numErrors := util.RunDiagnostics(o.Logger, diagnostics, 0, len(errors)) - return failed, err, numWarnings, numErrors + return util.RunDiagnostics(o.Logger(), diagnostics) } var ( @@ -129,12 +109,12 @@ var ( ) // buildPodDiagnostics builds host Diagnostic objects based on the host environment. -// Returns the Diagnostics built, "ok" bool for whether to proceed or abort, and an error if any was encountered during the building of diagnostics. -func (o PodDiagnosticsOptions) buildPodDiagnostics() ([]types.Diagnostic, bool, []error) { +// Returns the Diagnostics built, and any fatal error encountered during the building of diagnostics. +func (o PodDiagnosticsOptions) buildPodDiagnostics() ([]types.Diagnostic, error) { diagnostics := []types.Diagnostic{} - err, requestedDiagnostics := util.DetermineRequestedDiagnostics(availablePodDiagnostics.List(), o.RequestedDiagnostics, o.Logger) + err, requestedDiagnostics := util.DetermineRequestedDiagnostics(availablePodDiagnostics.List(), o.RequestedDiagnostics, o.Logger()) if err != nil { - return diagnostics, false, []error{err} // don't waste time on discovery + return diagnostics, err // don't waste time on discovery } // TODO: check we're actually in a container @@ -152,9 +132,9 @@ func (o PodDiagnosticsOptions) buildPodDiagnostics() ([]types.Diagnostic, bool, }) default: - return diagnostics, false, []error{fmt.Errorf("unknown diagnostic: %v", diagnosticName)} + return diagnostics, fmt.Errorf("unknown diagnostic: %v", diagnosticName) } } - return diagnostics, true, nil + return diagnostics, nil } diff --git a/pkg/oc/admin/diagnostics/util/util.go b/pkg/oc/admin/diagnostics/util/util.go index 183c8665c835..27a980dea986 100644 --- a/pkg/oc/admin/diagnostics/util/util.go +++ b/pkg/oc/admin/diagnostics/util/util.go @@ -31,16 +31,15 @@ func DetermineRequestedDiagnostics(available []string, requested []string, logge } // RunDiagnostics performs the actual execution of diagnostics once they're built. -func RunDiagnostics(logger *log.Logger, diagnostics []types.Diagnostic, warnCount int, errorCount int) (bool, error, int, int) { +func RunDiagnostics(logger *log.Logger, diagnostics []types.Diagnostic) error { + runCount := 0 for _, diagnostic := range diagnostics { func() { // wrap diagnostic panic nicely in case of developer error defer func() { if r := recover(); r != nil { - errorCount += 1 - stack := debug.Stack() logger.Error("CED7001", fmt.Sprintf("While running the %s diagnostic, a panic was encountered.\nThis is a bug in diagnostics. Error and stack trace follow: \n%s\n%s", - diagnostic.Name(), fmt.Sprintf("%v", r), stack)) + diagnostic.Name(), fmt.Sprintf("%v", r), debug.Stack())) } }() @@ -52,16 +51,18 @@ func RunDiagnostics(logger *log.Logger, diagnostics []types.Diagnostic, warnCoun } return } + runCount += 1 logger.Notice("CED7004", fmt.Sprintf("Running diagnostic: %s\nDescription: %s", diagnostic.Name(), diagnostic.Description())) r := diagnostic.Check() for _, entry := range r.Logs() { logger.LogEntry(entry) } - warnCount += len(r.Warnings()) - errorCount += len(r.Errors()) }() } - return errorCount > 0, nil, warnCount, errorCount + if runCount == 0 { + return fmt.Errorf("Requested diagnostic(s) skipped; nothing to run. See --help and consider setting flags or providing config to enable running.") + } + return nil }