From 6bd59dfcbaca06e8530cf60e41afdac7fb722024 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Tue, 4 Dec 2018 21:02:56 -0800 Subject: [PATCH] cmd/openshift-install: RunE -> logrus.Fatal Before this commit, we had a number of callers with the following pattern: func doSomething(args) error { cleanup, err := setupFileHook(rootOpts.dir) if err != nil { return errors.Wrap(err, "failed to setup logging hook") } defer cleanup() err := someHelper() if err != nil { return err } return nil } The problem with that approach is than any errors returned by doSomething will not be logged to .openshift_install.log, and we definitely want those errors logged to the file. With this commit, I've shuffled things around to ensure we call logrus.Fatal(err) before the deferred cleanup() fires. --- cmd/openshift-install/create.go | 56 ++++++++++++++++++-------------- cmd/openshift-install/destroy.go | 32 +++++++++++------- cmd/openshift-install/log.go | 8 ++--- 3 files changed, 55 insertions(+), 41 deletions(-) diff --git a/cmd/openshift-install/create.go b/cmd/openshift-install/create.go index 4749f99f29..aca9d1edcc 100644 --- a/cmd/openshift-install/create.go +++ b/cmd/openshift-install/create.go @@ -91,23 +91,31 @@ var ( Short: "Create an OpenShift cluster", // FIXME: add longer descriptions for our commands with examples for better UX. // Long: "", - PostRunE: func(_ *cobra.Command, _ []string) error { + PostRun: func(_ *cobra.Command, _ []string) { ctx := context.Background() + + cleanup := setupFileHook(rootOpts.dir) + defer cleanup() + config, err := clientcmd.BuildConfigFromFlags("", filepath.Join(rootOpts.dir, "auth", "kubeconfig")) if err != nil { - return errors.Wrap(err, "loading kubeconfig") + logrus.Fatal(errors.Wrap(err, "loading kubeconfig")) } err = destroyBootstrap(ctx, config, rootOpts.dir) if err != nil { - return err - } - consoleURL, err := waitForConsole(ctx, config, rootOpts.dir) - if err != nil { - return err + logrus.Fatal(err) } - return logComplete(rootOpts.dir, consoleURL) + consoleURL, err := waitForConsole(ctx, config, rootOpts.dir) + if err != nil { + logrus.Fatal(err) + } + + err = logComplete(rootOpts.dir, consoleURL) + if err != nil { + logrus.Fatal(err) + } }, }, assets: []asset.WritableAsset{&cluster.TerraformVariables{}, &kubeconfig.Admin{}, &cluster.Cluster{}}, @@ -126,22 +134,16 @@ func newCreateCmd() *cobra.Command { } for _, t := range targets { - t.command.RunE = runTargetCmd(t.assets...) + t.command.Run = runTargetCmd(t.assets...) cmd.AddCommand(t.command) } return cmd } -func runTargetCmd(targets ...asset.WritableAsset) func(cmd *cobra.Command, args []string) error { - return func(cmd *cobra.Command, args []string) error { - cleanup, err := setupFileHook(rootOpts.dir) - if err != nil { - return errors.Wrap(err, "failed to setup logging hook") - } - defer cleanup() - - assetStore, err := asset.NewStore(rootOpts.dir) +func runTargetCmd(targets ...asset.WritableAsset) func(cmd *cobra.Command, args []string) { + runner := func(directory string) error { + assetStore, err := asset.NewStore(directory) if err != nil { return errors.Wrapf(err, "failed to create asset store") } @@ -155,7 +157,7 @@ func runTargetCmd(targets ...asset.WritableAsset) func(cmd *cobra.Command, args err = errors.Wrapf(err, "failed to fetch %s", a.Name()) } - if err2 := asset.PersistToFile(a, rootOpts.dir); err2 != nil { + if err2 := asset.PersistToFile(a, directory); err2 != nil { err2 = errors.Wrapf(err2, "failed to write asset (%s) to disk", a.Name()) if err != nil { logrus.Error(err2) @@ -170,17 +172,21 @@ func runTargetCmd(targets ...asset.WritableAsset) func(cmd *cobra.Command, args } return nil } + + return func(cmd *cobra.Command, args []string) { + cleanup := setupFileHook(rootOpts.dir) + defer cleanup() + + err := runner(rootOpts.dir) + if err != nil { + logrus.Fatal(err) + } + } } // FIXME: pulling the kubeconfig and metadata out of the root // directory is a bit cludgy when we already have them in memory. func destroyBootstrap(ctx context.Context, config *rest.Config, directory string) (err error) { - cleanup, err := setupFileHook(rootOpts.dir) - if err != nil { - return errors.Wrap(err, "failed to setup logging hook") - } - defer cleanup() - client, err := kubernetes.NewForConfig(config) if err != nil { return errors.Wrap(err, "creating a Kubernetes client") diff --git a/cmd/openshift-install/destroy.go b/cmd/openshift-install/destroy.go index 5da614b704..b07d05282a 100644 --- a/cmd/openshift-install/destroy.go +++ b/cmd/openshift-install/destroy.go @@ -30,18 +30,20 @@ func newDestroyClusterCmd() *cobra.Command { return &cobra.Command{ Use: "cluster", Short: "Destroy an OpenShift cluster", - RunE: runDestroyCmd, + Run: func(_ *cobra.Command, _ []string) { + cleanup := setupFileHook(rootOpts.dir) + defer cleanup() + + err := runDestroyCmd(rootOpts.dir) + if err != nil { + logrus.Fatal(err) + } + }, } } -func runDestroyCmd(cmd *cobra.Command, args []string) error { - cleanup, err := setupFileHook(rootOpts.dir) - if err != nil { - return errors.Wrap(err, "failed to setup logging hook") - } - defer cleanup() - - destroyer, err := destroy.New(logrus.StandardLogger(), rootOpts.dir) +func runDestroyCmd(directory string) error { + destroyer, err := destroy.New(logrus.StandardLogger(), directory) if err != nil { return errors.Wrap(err, "Failed while preparing to destroy cluster") } @@ -49,7 +51,7 @@ func runDestroyCmd(cmd *cobra.Command, args []string) error { return errors.Wrap(err, "Failed to destroy cluster") } - store, err := asset.NewStore(rootOpts.dir) + store, err := asset.NewStore(directory) if err != nil { return errors.Wrapf(err, "failed to create asset store") } @@ -65,8 +67,14 @@ func newDestroyBootstrapCmd() *cobra.Command { return &cobra.Command{ Use: "bootstrap", Short: "Destroy the bootstrap resources", - RunE: func(cmd *cobra.Command, args []string) error { - return bootstrap.Destroy(rootOpts.dir) + Run: func(cmd *cobra.Command, args []string) { + cleanup := setupFileHook(rootOpts.dir) + defer cleanup() + + err := bootstrap.Destroy(rootOpts.dir) + if err != nil { + logrus.Fatal(err) + } }, } } diff --git a/cmd/openshift-install/log.go b/cmd/openshift-install/log.go index ca1275c551..44fe11c489 100644 --- a/cmd/openshift-install/log.go +++ b/cmd/openshift-install/log.go @@ -44,14 +44,14 @@ func (h *fileHook) Fire(entry *logrus.Entry) error { return err } -func setupFileHook(baseDir string) (func(), error) { +func setupFileHook(baseDir string) func() { if err := os.MkdirAll(baseDir, 0755); err != nil { - return nil, errors.Wrap(err, "failed to create base directory for logs") + logrus.Fatal(errors.Wrap(err, "failed to create base directory for logs")) } logfile, err := os.OpenFile(filepath.Join(baseDir, ".openshift_install.log"), os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0666) if err != nil { - return nil, errors.Wrap(err, "failed to open log file") + logrus.Fatal(errors.Wrap(err, "failed to open log file")) } originalHooks := logrus.LevelHooks{} @@ -68,5 +68,5 @@ func setupFileHook(baseDir string) (func(), error) { return func() { logfile.Close() logrus.StandardLogger().ReplaceHooks(originalHooks) - }, nil + } }