From 3d46d239f3d8fa2ebe8af17d75c0fb131f9c8017 Mon Sep 17 00:00:00 2001 From: Liz <33235082+liztio@users.noreply.github.com> Date: Mon, 9 Jul 2018 15:02:41 -0400 Subject: [PATCH] Retrieve error handling (#453) * Add errgroup dependency Signed-off-by: liz * client.Retrieve now returns an error channel instead of error sonobuoy retrieve now has better, more user-friendly error handling closes #407 Signed-off-by: liz --- Gopkg.lock | 8 ++- Gopkg.toml | 4 ++ cmd/sonobuoy/app/retrieve.go | 19 ++++-- pkg/client/interfaces.go | 2 +- pkg/client/retrieve.go | 37 +++++----- vendor/golang.org/x/sync/AUTHORS | 3 + vendor/golang.org/x/sync/CONTRIBUTORS | 3 + vendor/golang.org/x/sync/LICENSE | 27 ++++++++ vendor/golang.org/x/sync/PATENTS | 22 ++++++ vendor/golang.org/x/sync/errgroup/errgroup.go | 67 +++++++++++++++++++ 10 files changed, 169 insertions(+), 23 deletions(-) create mode 100644 vendor/golang.org/x/sync/AUTHORS create mode 100644 vendor/golang.org/x/sync/CONTRIBUTORS create mode 100644 vendor/golang.org/x/sync/LICENSE create mode 100644 vendor/golang.org/x/sync/PATENTS create mode 100644 vendor/golang.org/x/sync/errgroup/errgroup.go diff --git a/Gopkg.lock b/Gopkg.lock index 3dc80efe3..b61e1fa7e 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -337,6 +337,12 @@ ] revision = "ef147856a6ddbb60760db74283d2424e98c87bff" +[[projects]] + branch = "master" + name = "golang.org/x/sync" + packages = ["errgroup"] + revision = "1d60e4601c6fd243af51cc01ddf169918a5407ca" + [[projects]] branch = "master" name = "golang.org/x/sys" @@ -570,6 +576,6 @@ [solve-meta] analyzer-name = "dep" analyzer-version = 1 - inputs-digest = "ec6552ce0ec3103ac2c15ea9641355861fa9ac040e77b85ce2cdacf2a8dad3ef" + inputs-digest = "b442c341c1dcea876a7d1b400c37ef187fe3580075751fa18b317f3db8038530" solver-name = "gps-cdcl" solver-version = 1 diff --git a/Gopkg.toml b/Gopkg.toml index 3589789a7..f2c63c8d5 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -80,3 +80,7 @@ [[constraint]] branch = "release-8.0" name = "k8s.io/client-go" + +[[constraint]] + branch = "master" + name = "golang.org/x/sync" diff --git a/cmd/sonobuoy/app/retrieve.go b/cmd/sonobuoy/app/retrieve.go index 026283cfd..afd411c39 100644 --- a/cmd/sonobuoy/app/retrieve.go +++ b/cmd/sonobuoy/app/retrieve.go @@ -17,6 +17,7 @@ limitations under the License. package app import ( + "fmt" "os" "path/filepath" @@ -24,6 +25,8 @@ import ( "github.com/heptio/sonobuoy/pkg/errlog" "github.com/pkg/errors" "github.com/spf13/cobra" + "golang.org/x/sync/errgroup" + "k8s.io/client-go/util/exec" ) var ( @@ -70,15 +73,23 @@ func retrieveResults(cmd *cobra.Command, args []string) { } // Get a reader that contains the tar output of the results directory. - reader, err := sbc.RetrieveResults(&client.RetrieveConfig{Namespace: rcvFlags.namespace}) + reader, ec := sbc.RetrieveResults(&client.RetrieveConfig{Namespace: rcvFlags.namespace}) if err != nil { errlog.LogError(err) os.Exit(1) } - // Extract the tar output into a local directory under the prefix. - err = client.UntarAll(reader, outDir, prefix) - if err != nil { + eg := &errgroup.Group{} + eg.Go(func() error { return <-ec }) + eg.Go(func() error { return client.UntarAll(reader, outDir, prefix) }) + + err = eg.Wait() + if _, ok := err.(exec.CodeExitError); ok { + fmt.Fprintln(os.Stderr, "Results not ready yet. Check `sonobuoy status` for status.") os.Exit(1) + + } else if err != nil { + fmt.Fprintf(os.Stderr, "error retrieving results: %v\n", err) + os.Exit(2) } } diff --git a/pkg/client/interfaces.go b/pkg/client/interfaces.go index b50a9ab81..34627b108 100644 --- a/pkg/client/interfaces.go +++ b/pkg/client/interfaces.go @@ -128,7 +128,7 @@ type Interface interface { // GenerateManifest fills in a template with a Sonobuoy config GenerateManifest(cfg *GenConfig) ([]byte, error) // RetrieveResults copies results from a sonobuoy run into a Reader in tar format. - RetrieveResults(cfg *RetrieveConfig) (io.Reader, error) + RetrieveResults(cfg *RetrieveConfig) (io.Reader, <-chan error) // GetStatus determines the status of the sonobuoy run in order to assist the user. GetStatus(namespace string) (*aggregation.Status, error) // LogReader returns a reader that contains a merged stream of sonobuoy logs. diff --git a/pkg/client/retrieve.go b/pkg/client/retrieve.go index c284aec8d..507ecd9f0 100644 --- a/pkg/client/retrieve.go +++ b/pkg/client/retrieve.go @@ -24,8 +24,6 @@ import ( "path" "path/filepath" - "github.com/sirupsen/logrus" - "github.com/heptio/sonobuoy/pkg/config" "github.com/pkg/errors" @@ -34,10 +32,19 @@ import ( "k8s.io/client-go/tools/remotecommand" ) -func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, error) { +var tarCommand = []string{ + "/usr/bin/env", + "bash", + "-c", + fmt.Sprintf("tar cf - %s/*.tar.gz", config.MasterResultsPath), +} + +func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, <-chan error) { + ec := make(chan error, 1) client, err := c.Client() if err != nil { - return nil, err + ec <- err + return nil, ec } restClient := client.CoreV1().RESTClient() req := restClient.Post(). @@ -48,34 +55,30 @@ func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, error) Param("container", config.MasterContainerName) req.VersionedParams(&corev1.PodExecOptions{ Container: config.MasterContainerName, - Command: []string{"tar", "cf", "-", config.MasterResultsPath}, + Command: tarCommand, Stdin: false, Stdout: true, - Stderr: true, + Stderr: false, }, scheme.ParameterCodec) executor, err := remotecommand.NewSPDYExecutor(c.RestConfig, "POST", req.URL()) if err != nil { - return nil, err + ec <- err + return nil, ec } reader, writer := io.Pipe() - go func(writer *io.PipeWriter) { + go func(writer *io.PipeWriter, ec chan error) { defer writer.Close() + defer close(ec) err = executor.Stream(remotecommand.StreamOptions{ Stdout: writer, - Stderr: os.Stderr, Tty: false, }) if err != nil { - // Since this function returns an io.Reader to the consumer and does - // not buffer the entire (potentially large) output, RetrieveResults - // has to return the reader first to be read from. This means we - // either lose this error (easy) or provide a significantly more - // complex error mechanism for the consumer (hard). - logrus.Error(err) + ec <- err } - }(writer) + }(writer, ec) - return reader, nil + return reader, ec } /** Everything below this marker has been copy/pasta'd from k8s/k8s. The only modification is exporting UntarAll **/ diff --git a/vendor/golang.org/x/sync/AUTHORS b/vendor/golang.org/x/sync/AUTHORS new file mode 100644 index 000000000..15167cd74 --- /dev/null +++ b/vendor/golang.org/x/sync/AUTHORS @@ -0,0 +1,3 @@ +# This source code refers to The Go Authors for copyright purposes. +# The master list of authors is in the main Go distribution, +# visible at http://tip.golang.org/AUTHORS. diff --git a/vendor/golang.org/x/sync/CONTRIBUTORS b/vendor/golang.org/x/sync/CONTRIBUTORS new file mode 100644 index 000000000..1c4577e96 --- /dev/null +++ b/vendor/golang.org/x/sync/CONTRIBUTORS @@ -0,0 +1,3 @@ +# This source code was written by the Go contributors. +# The master list of contributors is in the main Go distribution, +# visible at http://tip.golang.org/CONTRIBUTORS. diff --git a/vendor/golang.org/x/sync/LICENSE b/vendor/golang.org/x/sync/LICENSE new file mode 100644 index 000000000..6a66aea5e --- /dev/null +++ b/vendor/golang.org/x/sync/LICENSE @@ -0,0 +1,27 @@ +Copyright (c) 2009 The Go Authors. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/vendor/golang.org/x/sync/PATENTS b/vendor/golang.org/x/sync/PATENTS new file mode 100644 index 000000000..733099041 --- /dev/null +++ b/vendor/golang.org/x/sync/PATENTS @@ -0,0 +1,22 @@ +Additional IP Rights Grant (Patents) + +"This implementation" means the copyrightable works distributed by +Google as part of the Go project. + +Google hereby grants to You a perpetual, worldwide, non-exclusive, +no-charge, royalty-free, irrevocable (except as stated in this section) +patent license to make, have made, use, offer to sell, sell, import, +transfer and otherwise run, modify and propagate the contents of this +implementation of Go, where such license applies only to those patent +claims, both currently owned or controlled by Google and acquired in +the future, licensable by Google that are necessarily infringed by this +implementation of Go. This grant does not include claims that would be +infringed only as a consequence of further modification of this +implementation. If you or your agent or exclusive licensee institute or +order or agree to the institution of patent litigation against any +entity (including a cross-claim or counterclaim in a lawsuit) alleging +that this implementation of Go or any code incorporated within this +implementation of Go constitutes direct or contributory patent +infringement, or inducement of patent infringement, then any patent +rights granted to you under this License for this implementation of Go +shall terminate as of the date such litigation is filed. diff --git a/vendor/golang.org/x/sync/errgroup/errgroup.go b/vendor/golang.org/x/sync/errgroup/errgroup.go new file mode 100644 index 000000000..533438d91 --- /dev/null +++ b/vendor/golang.org/x/sync/errgroup/errgroup.go @@ -0,0 +1,67 @@ +// Copyright 2016 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Package errgroup provides synchronization, error propagation, and Context +// cancelation for groups of goroutines working on subtasks of a common task. +package errgroup + +import ( + "sync" + + "golang.org/x/net/context" +) + +// A Group is a collection of goroutines working on subtasks that are part of +// the same overall task. +// +// A zero Group is valid and does not cancel on error. +type Group struct { + cancel func() + + wg sync.WaitGroup + + errOnce sync.Once + err error +} + +// WithContext returns a new Group and an associated Context derived from ctx. +// +// The derived Context is canceled the first time a function passed to Go +// returns a non-nil error or the first time Wait returns, whichever occurs +// first. +func WithContext(ctx context.Context) (*Group, context.Context) { + ctx, cancel := context.WithCancel(ctx) + return &Group{cancel: cancel}, ctx +} + +// Wait blocks until all function calls from the Go method have returned, then +// returns the first non-nil error (if any) from them. +func (g *Group) Wait() error { + g.wg.Wait() + if g.cancel != nil { + g.cancel() + } + return g.err +} + +// Go calls the given function in a new goroutine. +// +// The first call to return a non-nil error cancels the group; its error will be +// returned by Wait. +func (g *Group) Go(f func() error) { + g.wg.Add(1) + + go func() { + defer g.wg.Done() + + if err := f(); err != nil { + g.errOnce.Do(func() { + g.err = err + if g.cancel != nil { + g.cancel() + } + }) + } + }() +}