From 8059482abd6d21cae074347e2029ea3b44ce6aa7 Mon Sep 17 00:00:00 2001
From: Luke Meyer <lmeyer@redhat.com>
Date: Sun, 17 Dec 2017 10:19:05 -0500
Subject: [PATCH] diagnostics: refactor build-and-run for clarity

Improve the legibility of the code that builds and runs diagnostics.

The main confusion was the need to track and report the number of
diagnostic errors and warnings versus problems that halt execution
prematurely and the need to return a correct status code at completion.
In the end it seemed simplest to just have the logger report how many
diagnostic errors and warnings were seen, leaving function signatures to
return only build/run errors.

As a side effect, I looked at the ConfigLoading code that does an early
check to see if there is a client config, and concluded it was confusing
and unnecessary for it to be a diagnostic, so I refactored it away.

Main diagnostics as well as pod diagnostics are now implemented more uniformly.
---
 pkg/oc/admin/diagnostics/client.go            |  10 +-
 pkg/oc/admin/diagnostics/cluster.go           | 104 ++++----
 pkg/oc/admin/diagnostics/config.go            | 140 ++++++++--
 pkg/oc/admin/diagnostics/diagnostics.go       | 245 +++++++-----------
 .../diagnostics/client/config_loading.go      | 150 -----------
 .../admin/diagnostics/diagnostics/log/log.go  |  22 +-
 pkg/oc/admin/diagnostics/host.go              |   6 +-
 pkg/oc/admin/diagnostics/network_pod.go       |  71 ++---
 pkg/oc/admin/diagnostics/pod.go               |  68 ++---
 pkg/oc/admin/diagnostics/util/util.go         |  15 +-
 10 files changed, 347 insertions(+), 484 deletions(-)
 delete mode 100644 pkg/oc/admin/diagnostics/diagnostics/client/config_loading.go

diff --git a/pkg/oc/admin/diagnostics/client.go b/pkg/oc/admin/diagnostics/client.go
index 6c477edc81c7..922713de3608 100644
--- a/pkg/oc/admin/diagnostics/client.go
+++ b/pkg/oc/admin/diagnostics/client.go
@@ -18,14 +18,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)
 	}
 
@@ -64,8 +64,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..f156a728d722 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,122 @@ 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) && len(foundPath) == 0 {
+			successfulLoad = true
+			foundPath = path
+		}
+	}
+	if len(foundPath) > 0 {
+		if len(confFlagValue) > 0 && 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 len(confFlagValue) > 0 {
+			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
+		err  error
+	)
+	if len(path) == 0 { // 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 len(errmsg) == 0 {
+		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 {
+		return false
+	}
+
+	// file 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..e1b3fa22e698 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
+	RunDiagnostics() 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.RunDiagnostics(); 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))
-			}
-		}()
+// RunDiagnostics 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) RunDiagnostics() 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..0fa2cd3eed2b 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
 	}
@@ -90,45 +81,35 @@ func (o *NetworkPodDiagnosticsOptions) Complete(args []string) (err error) {
 	return nil
 }
 
-// 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{}
+// RunDiagnostics builds diagnostics based on the options and executes them, returning a summary.
+func (o NetworkPodDiagnosticsOptions) RunDiagnostics() 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..7b8ed5e8de66 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{}
+// RunDiagnostics builds diagnostics based on the options and executes them, returning fatal error(s) only.
+func (o PodDiagnosticsOptions) RunDiagnostics() 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
 }