Implicitly extract tar.gz files from plugins that upload them #47

kensimon · 2017-08-10T01:16:23Z

This makes it so plugins upload to a path like:

/api/v1/results/global/e2e.tar.gz

And it will show up in:

/results/plugins/e2e/results/<contents>

(For single-node plugins, this would map to:)

/results/plugins/systemd_logs/node1/<contents>

This needs to be documented along with the snapshot documentation that
is already under review.

Since the upload path is now expected to have a file extension in it, In
case for some reason the sonobuoy master launches with an old version
of the sonobuoy worker (or for plugins not using sonobuoy worker that
upload their own contents), if the path has no file extension, it is
guessed to be .json or .tar.gz, same as before.

Signed-off-by: Ken Simon ninkendo@gmail.com

kensimon · 2017-08-10T01:18:02Z

Fixes #28

timothysc

Comments, namely around us being explicit on the results coming from the plugins.

timothysc · 2017-08-10T15:21:16Z

pkg/plugin/aggregation/aggregator.go

+	if result.Extension != "" {
+		extension = result.Extension
+	} else {
+		extension = result.GuessExtension()


Guess? Aren't we always going to be explicit w/defaults?

We only know the extension if one was provided in the upload URI (ie. /api/v1/results/global/foo.json), I'm not introducing any concept of per-plugin defaults or anything.

In case a plugin doesn't include a . in the upload URI, we're using the old behavior of guessing json unless it's the e2e plugin. This may be a bit over-defensive though. Any suggestions on a better approach?

timothysc · 2017-08-10T15:22:23Z

pkg/plugin/aggregation/aggregator.go

@@ -217,5 +225,20 @@ func (a *Aggregator) handleResult(result *plugin.Result) error {
 	io.Copy(f, result.Body)
 	glog.Infof("wrote results to %v\n", resultsFile)

+	// If it's a tarball, extract it
+	if extension == ".tar.gz" {
+		f.Close()


shouldn't we close above and remove the defer?

Trying to be defensive in case later code introduces things that may panic... Close() is safe to call repeatedly on a filehandle in go, at least from what I've been able to dig up.

I'll make another pass at this and see if I can make it a bit cleaner, I'm not all that happy with it yet.

timothysc · 2017-08-10T15:23:38Z

pkg/plugin/aggregation/aggregator.go

+		err = tarx.Extract(resultsFile, path.Join(a.OutputDir, result.Path()), &tarx.ExtractOptions{})
+		if err != nil {
+			glog.Errorf("Could not extract tar file %v: %v", resultsFile, err)
+			return err


I'd have to pop up, but what happens if there is an error in the aggregation? Is it just reported and tar.gz the whole results?

Good point, currently errors are channeled from (*Driver).Monitor() as Result objects and get stored the same way, but nothing's specifying that their extension should be json. I'll add Extension: ".json" to the MakeErrorResult function here, and make sure we have tests for this.

timothysc · 2017-08-10T15:26:44Z

pkg/plugin/aggregation/aggregator_test.go

+	})
+}
+
+func TestAggregation_guessExtension(t *testing.T) {


same comment as above.

timothysc · 2017-08-10T15:28:58Z

pkg/plugin/aggregation/server.go

@@ -212,3 +215,16 @@ func (s *Server) globalResultsHandler(w http.ResponseWriter, r *http.Request) {
 	s.ResultsCallback(result, w)
 	r.Body.Close()
 }
+
+// given an uploaded filename, parse it into its base name and extension.  If


Could use https://golang.org/pkg/path/filepath/

TIL. But looks like filepath.Ext grabs the last dot, I want the first dot (eg. I want ".tar.gz" from "e2e.tar.gz", not ".gz".)

This makes it so plugins upload to a path like: ``` /api/v1/results/global/e2e.tar.gz ``` And it will show up in: ``` /results/plugins/e2e/results/<contents> ``` (For single-node plugins, this would map to:) ``` /results/plugins/systemd_logs/node1/<contents> ``` This needs to be documented along with the snapshot documentation that is already under review. Since the upload path is now expected to have a file extension in it, In case for some reason the sonobuoy master launches with an old version of the sonobuoy worker (or for plugins not using `sonobuoy worker` that upload their own contents), if the path has no file extension, it is guessed to be `.json` or `.tar.gz`, same as before. Signed-off-by: Ken Simon <ninkendo@gmail.com>

kensimon · 2017-08-10T17:55:17Z

@timothysc PTAL

Made some changes per review:

If there's no file extension in the upload URI, we just save the file without an extension instead of guessing one (so no untarring will happen in that case)
Plugin errors channeled from driver.Monitor() now carry a ".json" extension to make things consistent, and there's a test for this now
Removed duplicate Close() calls by putting the file write in a closure and putting the defer f.Close() there

timothysc

Minor nit but /lgtm - dealers choice on if you want to change now or later.

timothysc · 2017-08-10T19:32:24Z

pkg/plugin/aggregation/aggregator.go

-	// Copy the request body into the file
-	io.Copy(f, result.Body)
-	glog.Infof("wrote results to %v\n", resultsFile)
+	// If it's a tarball, extract it


could switch case this as we'll likely have other file options to add.

I'm thinking we can cross that bridge when we get there... it's possible we'll want to something more intelligent like some full post-processing logic that lives somewhere else, etc.

Implicitly extract tar.gz files from plugins that upload them Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com

Implicitly extract tar.gz files from plugins that upload them Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com Signed-off-by: Jesse Hamilton jesse.hamilton@heptio.com

kensimon force-pushed the extract-tar-within-tar branch from 5ef704b to 263664a Compare August 10, 2017 01:34

timothysc suggested changes Aug 10, 2017

View reviewed changes

kensimon force-pushed the extract-tar-within-tar branch from 263664a to 5082fc2 Compare August 10, 2017 17:52

timothysc approved these changes Aug 10, 2017

View reviewed changes

kensimon merged commit 7aac273 into vmware-tanzu:master Aug 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implicitly extract tar.gz files from plugins that upload them #47

Implicitly extract tar.gz files from plugins that upload them #47

kensimon commented Aug 10, 2017

kensimon commented Aug 10, 2017

timothysc left a comment

timothysc Aug 10, 2017

kensimon Aug 10, 2017

timothysc Aug 10, 2017

kensimon Aug 10, 2017

timothysc Aug 10, 2017

kensimon Aug 10, 2017

timothysc Aug 10, 2017

timothysc Aug 10, 2017

kensimon Aug 10, 2017

kensimon commented Aug 10, 2017

timothysc left a comment

timothysc Aug 10, 2017

kensimon Aug 10, 2017

Implicitly extract tar.gz files from plugins that upload them #47

Implicitly extract tar.gz files from plugins that upload them #47

Conversation

kensimon commented Aug 10, 2017

kensimon commented Aug 10, 2017

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kensimon commented Aug 10, 2017

timothysc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment