Preserve single file result names and keep job/ds depth the same #785

johnSchnake · 2019-06-27T19:09:37Z

What this PR does / why we need it:
If there is a job returning results it currently goes into the
tarball at plugins/name/results/ but this is different
from daemonset results which have an extra directory stating
the node name (plugins/name/node/results/).

To prepare for consistent handling of results, it would be preferred
that these have the same layouts so we added a new level for jobs:
"global" so a job plugin results go to plugins/name/global/results.

In addition, if a single file was uploaded as results, it was handled
in a unique way. Instead of being put into the directories listed above,
the same path would refer, instead, to a file. The contents of which
were the result.

It would be much preferred to preserve the folder structure and the
filename being uploaded. This way automated processing is easier and
users can rationalize about what files they are uploading (when they
would reasonably expect them, by name, to appear in the tarball).

Which issue(s) this PR fixes
Fixes #741

Special notes for your reviewer:
TODO:

Add more tests (waiting to see coverage report but I know I'll need some more)
Will add details here how to test it and confirm different situations.
Fixup existing unit tests
Add new version of tarball structure

Release note:

Made the location of plugin results within the results tarball more consistent. Job type plugins will have their results placed into `plugins/<name>/global/results/<files>` to be at a consistent depth with daemonset plugins (which list their nodes in the path). In addition, when a plugin reports a single file (not an archive) of results, then that file is put into the same directory and with the same name, it had locally on the container.

johnSchnake · 2019-06-27T19:10:13Z

cmd/sonobuoy/app/worker.go

-		os.Exit(1)
-	}
-
-	client, err := getHTTPClient(cfg)


Fixed these repetitive sections while I was here. The only thing different between these methods were the URLs so I extracted that logic.

johnSchnake · 2019-06-27T19:10:49Z

pkg/plugin/aggregation/aggregator.go

@@ -285,24 +285,24 @@ func (a *Aggregator) handleResult(result *plugin.Result) error {
 	}

 	// Create the output directory for the result.  Will be of the
-	// form .../plugins/:results_type/:node.json (for DaemonSet plugins) or
+	// form .../plugins/:results_type/:node/filename.json (for DaemonSet plugins) or
 	// .../plugins/:results_type.json (for Job plugins)


Need to update this comment.

johnSchnake · 2019-06-27T19:11:19Z

pkg/plugin/aggregation/aggregator.go


 	if err := os.MkdirAll(resultsDir, 0755); err != nil {
-		errors.Wrapf(err, "couldn't create directory %v", resultsDir)
+		errors.Wrapf(err, "couldn't create directory %q", resultsDir)


Errors when iterating made it clear that quotes here would be easier due to confusion when the error string ends up there.

johnSchnake · 2019-06-27T19:11:47Z

pkg/plugin/aggregation/aggregator.go

@@ -311,7 +311,7 @@ func (a *Aggregator) handleResult(result *plugin.Result) error {
 }

 func (a *Aggregator) handleArchiveResult(result *plugin.Result) error {
-	resultsDir := path.Join(a.OutputDir, result.Path())
+	resultsDir := filepath.Join(a.OutputDir, result.Path())


Somewhat preemptively changed these to filepath; it will help when we start ensuring compatibility on windows nodes. No change otherwise.

pkg/plugin/aggregation/handler.go

johnSchnake · 2019-06-27T19:15:17Z

pkg/plugin/driver/job/job.go

@@ -72,7 +72,7 @@ func NewPlugin(dfn plugin.Definition, namespace, sonobuoyImage, imagePullPolicy,
 // a Job only launches one pod, only one result type is expected.
 func (p *Plugin) ExpectedResults(nodes []v1.Node) []plugin.ExpectedResult {
 	return []plugin.ExpectedResult{
-		{ResultType: p.GetResultType()},
+		{ResultType: p.GetResultType(), NodeName: plugin.GlobalResult},


Theoretically, if you have a node named "global" then it becomes ambiguous (when looking at the tarball by itself) if a job or daemonset was run.

However, that seems to be a fair tradeoff since by doing this we get rid of another ambiguity (if a job uploaded results with directories named with the nodes).

Both are unlikely and this is preferred since it helps make the two structures consistent.

johnSchnake · 2019-06-27T19:16:27Z

pkg/worker/worker.go

 		outfile, err = os.Open(resultFile)
-		return outfile, mimeType, errors.WithStack(err)
+		return outfile, filepath.Base(resultFile), mimeType, errors.WithStack(err)


This is the logic that kicks off informing the rest of the code what the filename is.

Then the handler will add it to the headers and the aggregator will read the header to name the file on its side.

johnSchnake · 2019-06-27T19:32:52Z

Will need to update the logic for getting e2e results as well;

CI shows: results/plugins/e2e/results/global/junit_01.xml but fails because ERRO[0000] could not get tests from archive: failed to find results file "plugins/e2e/results/junit_01.xml" in archive

johnSchnake · 2019-06-27T20:04:57Z

e2e tests and command passing; now it is just all the unit tests that need fixed up. Will be a bit of a pain because there are test tarballs that get tested and I'll have to deal with copying/versioning them so it will be a bit non-trivial.

Would love a review at this point though before I get into the weeds with that stuff.

zubron · 2019-06-27T20:10:01Z

I'm reviewing it now! :)

johnSchnake · 2019-06-27T21:13:27Z

pkg/client/results/e2e/consts.go

@@ -19,6 +19,6 @@ package e2e

 const (
 	// ResultsDirectory is the directory where the results will be found
-	ResultsSubdirectory = "e2e/results/"
+	ResultsSubdirectory = "e2e/results/global/"


This fixes the e2e command for now, but unlike the rest of the results stuff, it wasn't versioned.

I could modify this to look in the appropriate location based on the version. One of the frustrating things about this is that the config readers are not rewindable so you only get one read-per open. That would be great to be able to change that.

Sorry, I'm missing something here. Where and how is the rest of the results stuff versioned?

So even I learned something going through this code. I originally thought it was versioned independently of Sonobuoy because I saw things like this. However, the version it uses (to compare those to) is actually the version stored in the config file; i.e. the Sonobuoy version.

Those are just versioning it based on the major/minor number and not patch releases which is why I (in another snippet of code) moved to using go-version which will let me do more complex checks with versions that look like semver (the existing checks in the Reader just want to know if the version has that prefix, e.g. has the same major/minor version)

zubron

I think this is looking good so far. I know it's in your PR description about adding more details about how to test this, but I tried to run it locally and couldn't see the updated results structure. I'm guessing I should wait and get the extra details from you :)

pkg/plugin/aggregation/handler.go

pkg/plugin/aggregation/handler_test.go

zubron · 2019-06-27T21:30:34Z

pkg/client/results/e2e/consts.go

@@ -19,6 +19,6 @@ package e2e

 const (
 	// ResultsDirectory is the directory where the results will be found
-	ResultsSubdirectory = "e2e/results/"
+	ResultsSubdirectory = "e2e/results/global/"


Sorry, I'm missing something here. Where and how is the rest of the results stuff versioned?

codecov-io · 2019-07-03T15:20:48Z

Codecov Report

Merging #785 into master will increase coverage by 0.09%.
The diff coverage is 66.19%.

@@            Coverage Diff             @@
##           master     #785      +/-   ##
==========================================
+ Coverage   42.84%   42.93%   +0.09%     
==========================================
  Files          71       71              
  Lines        4232     4220      -12     
==========================================
- Hits         1813     1812       -1     
+ Misses       2311     2299      -12     
- Partials      108      109       +1

Impacted Files	Coverage Δ
pkg/plugin/driver/utils/utils.go	`30.76% <0%> (-0.81%)`	⬇️
pkg/client/e2e.go	`75% <100%> (+0.64%)`	⬆️
pkg/worker/worker.go	`100% <100%> (ø)`	⬆️
pkg/plugin/aggregation/handler.go	`86.2% <100%> (+3.98%)`	⬆️
pkg/client/results/reader.go	`73.59% <58.82%> (-2.51%)`	⬇️
pkg/worker/request.go	`58.33% <66.66%> (-0.76%)`	⬇️
pkg/plugin/driver/job/job.go	`56.17% <66.66%> (ø)`	⬆️
pkg/plugin/aggregation/aggregator.go	`79.2% <77.77%> (+0.33%)`	⬆️
cmd/sonobuoy/app/worker.go	`10.63% <8.33%> (+0.63%)`	⬆️
pkg/discovery/discovery.go	`0% <0%> (-5.09%)`	⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3e7514...4caf2bc. Read the comment docs.

johnSchnake · 2019-07-03T18:00:53Z

pkg/client/e2e.go

@@ -44,9 +45,9 @@ func (*SonobuoyClient) GetTests(reader io.Reader, show string) ([]reporters.JUni
 			}
 			// TODO(chuckha) consider reusing this function for any generic e2e-esque plugin results.
 			// TODO(chuckha) consider using path.Join()
-			if path == e2eJunitPath {
+			if path == e2eJunitPath || path == legacye2eJunitPath {


Since the old path is at a parent directory from the new one (and so a user can't actually write data to that file that could be misleading) it is fine to just search for either of the paths.

johnSchnake · 2019-07-03T18:02:34Z

pkg/client/e2e.go

 				found = true
-				return results.ExtractFileIntoStruct(e2eJunitPath, path, info, &junitResults)
+				return results.ExtractFileIntoStruct(path, path, info, &junitResults)


All of these Extract... functions are kind of hard to read IMO; I think in some cases we're misusing them and we don't need to be first filtering for the filepath. The only reason to do so here is to (1) mark that we actually found the file and (2) here we want to extract either file into the struct instead of a hardcoded one.

johnSchnake · 2019-07-03T18:06:46Z

pkg/plugin/aggregation/aggregator_test.go

@@ -94,7 +94,7 @@ func TestAggregation_noExtension(t *testing.T) {

 func TestAggregation_tarfile(t *testing.T) {
 	expected := []plugin.ExpectedResult{
-		{ResultType: "e2e"},
+		{ResultType: "e2e", NodeName: "global"},


I've done the "nodename=global" work but it does have the tiny problem of: what if someone actually has a node with that name? Does it make a problem?

So it seems like it might but we actually err out if two plugins have the same ResultType so there isn't a risk of path conflics.

The only concern is when parsing the results will we interpret e2e/results/global as a job with results or a daemonset with results from a single node named global.

Since those are really just problems with reading folder names it doesn't seem like we can get away from some ambiguity so it seems fine to me. I just wanted to call it out so another brain can at least consider it.

Hmm, it doesn't seem that relying on the directory structure is the best way to determine the type of test that was run anyway. Do we have any other metadata to help distinguish between these two cases?

We are dumping all the plugin info into a config map with a known name; we can also read that in and iterate over the data. You're correct that is the most accurate thing to automate; on the next PR I may do that as well but I do think that it helps to have the path also provide implicit context so that you don't have to fully parse a separate file at a known location to get that information.

johnSchnake · 2019-07-03T18:07:31Z

pkg/plugin/aggregation/aggregator_test.go

@@ -125,6 +125,9 @@ func TestAggregation_tarfile(t *testing.T) {
 			}
 		} else {
 			t.Errorf("AggregationServer didn't record a result for e2e tests. Got: %+v", agg.Results)
+			for k, v := range agg.Results {
+				t.Logf("Result %q: %+v\n", k, v)


The set of results is a map so you get just the key: <memory address> when printing the map above. This makes it clear what each result is if you get an error (which I did while making these changes)

johnSchnake · 2019-07-03T18:08:39Z

pkg/plugin/aggregation/aggregator_test.go

-		} else {
-			r = strings.NewReader("foo")
-		}
+		t.Run(tc.desc, func(t *testing.T) {


This was a table-driven test but was probably implemented before subtests in golang.

I've just indented everything; I originally had an error here and couldn't tell why because it was running all test cases in one go.

johnSchnake · 2019-07-03T18:09:49Z

pkg/plugin/aggregation/aggregator_test.go

+			// Add buffer to avoid raciness due to processing time.
+			diffTime := waitTime - tc.expectExtraWait
+			if diffTime > testBufferDuration || diffTime < -1*testBufferDuration {
+				t.Errorf("Expected Wait() to wait the duration %v (+/- %v), instead waited %v", tc.expectExtraWait, testBufferDuration, waitTime)


Cleaned up the error message; it was confusing in cases

where we weren't causing errors

where the wait time > expectedWaitTime

what precision we expected

johnSchnake · 2019-07-03T18:11:26Z

pkg/plugin/aggregation/aggregator_test.go

-		if result, ok := agg.Results["e2e"]; ok {
-			bytes, err := ioutil.ReadFile(path.Join(agg.OutputDir, result.Path()))
+		if result, ok := agg.Results["e2e/global"]; ok {
+			bytes, err := ioutil.ReadFile(path.Join(agg.OutputDir, result.Path(), "error.json"))


Again, the error, being a single file, used to be named the same as the expected path. This would be counterintuitive to users who are not expecting users or even single files: when running e2e for instance you assume the directory will exist and you can cd to it and print files. In a CI script, that would fail if you didn't have the directory you expected.

johnSchnake · 2019-07-08T19:04:59Z

Rebased; I think this is done and ready for final review though.

To review the real output I used this command (which uses some images I made available on my dockerhub):

sonobuoy run --sonobuoy-image=schnake/sonobuoy:keepFilenames --plugin=./singleFile.plugin --wait --image-pull-policy=Always --plugin=./singleFileDS.plugin --plugin ./tarresults.plugin --plugin ./tarresultsds.plugin

It uses 4 plugins defined locally which investigate the combination of {single file vs tarfile} & {job vs daemonset}

The single file plugin def is:

sonobuoy-config:
  driver: Job
  plugin-name: singlefile
  result-type: singlefile
spec:
  command:
  - ./run.sh
  image: schnake/singlefileresults:v0.1
  name: plugin
  resources: {}
  volumeMounts:
  - mountPath: /tmp/results
    name: results

and the tar file one is:

sonobuoy-config:
  driver: Job
  plugin-name: tar
  result-type: tar
spec:
  args:
  - echo hello world
  - echo hello world pt 2
  command:
  - ./run.sh
  image: schnake/easy-sonobuoy-cmds:v0.1
  name: plugin
  resources: {}
  volumeMounts:
  - mountPath: /tmp/results
    name: results

To make the other 2, just copy the files modify job->daemonset and modify the name so it is unique for every plugin.

zubron

Thanks, @johnSchnake! I've left a number of comments but nothing major. It's mostly small cleanup related things or questions. I'm happy for you to merge this once you've addressed those.

zubron · 2019-07-18T15:50:11Z

pkg/client/results/reader.go

 	"io"
 	"os"
 	"path/filepath"
 	"strings"

+	version "github.com/hashicorp/go-version"


version is used as a variable name a number of times in this file. To prevent shadowing of this alias, I would use a different name here, or change the variable names.

zubron · 2019-07-18T15:52:29Z

pkg/client/results/reader.go

@@ -111,18 +118,30 @@ func DiscoverVersion(reader io.Reader) (string, error) {
 	if err != nil {
 		return "", errors.Wrap(err, "error extracting config")
 	}
-	var version string
+
+	parsedVersion, err := validateVersion(conf.Version)


Minor thing, but is the separate validateVersion function needed? It doesn't seem to provide any additional value other than wrapping the error which is already happening in the err check below. It seems like that could be combined.

I'll take a look; I originally just copied the code snippet from another location and I think the validation may have done another task there which I removed, so it may not be necessary now.

zubron · 2019-07-18T17:24:21Z

pkg/plugin/aggregation/aggregator_test.go

@@ -94,7 +94,7 @@ func TestAggregation_noExtension(t *testing.T) {

 func TestAggregation_tarfile(t *testing.T) {
 	expected := []plugin.ExpectedResult{
-		{ResultType: "e2e"},
+		{ResultType: "e2e", NodeName: "global"},


Hmm, it doesn't seem that relying on the directory structure is the best way to determine the type of test that was run anyway. Do we have any other metadata to help distinguish between these two cases?

zubron · 2019-07-18T17:26:31Z

pkg/plugin/driver/job/job.go

@@ -160,15 +160,15 @@ func (p *Plugin) monitorOnce(kubeclient kubernetes.Interface, _ []v1.Node) (done
 	// Make sure there's a pod
 	pod, err := p.findPod(kubeclient)
 	if err != nil {
-		return true, utils.MakeErrorResult(p.GetResultType(), map[string]interface{}{"error": err.Error()}, "")
+		return true, utils.MakeErrorResult(p.GetResultType(), map[string]interface{}{"error": err.Error()}, "global")


Can we make use of plugin.GlobalResult here rather than a literal?

zubron · 2019-07-18T17:26:38Z

pkg/plugin/driver/job/job.go

 	}

 	// Make sure the pod isn't failing
 	if isFailing, reason := utils.IsPodFailing(pod); isFailing {
 		return true, utils.MakeErrorResult(p.GetResultType(), map[string]interface{}{
 			"error": reason,
 			"pod":   pod,
-		}, "")
+		}, "global")


Same as above.

zubron · 2019-07-18T17:40:53Z

pkg/plugin/aggregation/aggregator.go

-	resultsFile := path.Join(a.OutputDir, result.Path())
-	resultsDir := path.Dir(resultsFile)
+	// Create the output directory for the result. Will be of the
+	// form .../plugins/:results_type/[:node|global]/results/filename.json


The order of node|global/results is incorrect in this comment. In the implementation of Path, the order is results_type/results/node_name.

zubron · 2019-07-18T17:42:13Z

pkg/plugin/aggregation/aggregator.go

 	if err != nil {
-		return errors.Wrapf(err, "couldn't create results file %v", resultsFile)
+		return errors.Wrapf(err, "couldn't create results file %q", result.Filename)


I think it might be more useful to include the full path here rather than just the filename.

zubron · 2019-07-18T17:59:47Z

pkg/worker/request.go

@@ -90,6 +90,11 @@ func DoRequest(url string, client *http.Client, callback func() (io.Reader, stri
 		return errors.Wrapf(err, "error constructing master request to %v", url)
 	}
 	req.Header.Add("content-type", mimeType)
+	if len(filename) > 0 {


This change is missing a test to check that the content-disposition header is set correctly based on the result of the callback. The true case is being hit as part of other tests but it's not being explicitly checked.

zubron · 2019-07-18T18:10:40Z

pkg/plugin/interface.go

@@ -27,6 +27,13 @@ import (
 	"k8s.io/client-go/kubernetes"
 )

+var (


Can this be const instead?

If there is a job returning results it currently goes into the tarball at plugins/name/results/<files> but this is different from daemonset results which have an extra directory stating the node name (plugins/name/results/node/<files>). To prepare for consistent handling of results, it would be preferred that these have the same layouts so we added a new level for jobs: "global" so a job plugin results go to plugins/name/results/global. In addition, if a single file was uploaded as results, it was handled in a unique way. Instead of being put into the directories listed above, the same path would refer, instead, to a file. The contents of which were the result. It would be much preferred to preserve the folder structure and the filename being uploaded. This way automated processing is easier and users can rationalize about what files they are uploading (when they would reasonably expect them, by name, to appear in the tarball). Fixes #741 Signed-off-by: John Schnake <jschnake@vmware.com>

johnSchnake · 2019-07-19T18:45:23Z

Only change this doesn't have from master is the regexp stuff for e2e --rerun; no problem squashing it.

Since the last change I just tweaked the code based on the comments:

added another test for the request headers
made the global string a constant and used it in a few places instead of restating the literal
made the path listed in an error the full path instead of just the base name of the file.

Merging.

johnSchnake requested a review from zubron June 27, 2019 19:09

johnSchnake commented Jun 27, 2019

View reviewed changes

pkg/plugin/aggregation/handler.go Show resolved Hide resolved

johnSchnake commented Jun 27, 2019

View reviewed changes

pkg/plugin/aggregation/handler.go Show resolved Hide resolved

johnSchnake commented Jun 27, 2019

View reviewed changes

zubron reviewed Jun 28, 2019

View reviewed changes

johnSchnake commented Jul 3, 2019

View reviewed changes

zubron approved these changes Jul 18, 2019

View reviewed changes

johnSchnake merged commit 6240935 into vmware-tanzu:master Jul 19, 2019

Preserve single file result names and keep job/ds depth the same #785

Preserve single file result names and keep job/ds depth the same #785

Conversation

johnSchnake commented Jun 27, 2019 • edited Loading

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

johnSchnake commented Jun 27, 2019

johnSchnake commented Jun 27, 2019

zubron commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zubron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 3, 2019 • edited Loading

Codecov Report

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

johnSchnake commented Jul 8, 2019

zubron 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

Choose a reason for hiding this comment

johnSchnake commented Jul 19, 2019

johnSchnake commented Jun 27, 2019 •

edited

Loading

codecov-io commented Jul 3, 2019 •

edited

Loading