Fix bug which caused daemonset status to not be updated with results #1045

johnSchnake · 2019-12-12T17:52:10Z

What this PR does / why we need it:
The bug would cause only one of the plugin/node values in the
sonobuoy status --json output to be updated and it would include
all the results for all nodes.

Instead, we want each plugin/node value in that list to report its
own values for passed/failed.

Which issue(s) this PR fixes
Fixes #1001

Special notes for your reviewer:

Release note:

Fixed bug which caused daemonset plugins' status to not be updated with results properly.

johnSchnake · 2019-12-12T17:54:19Z

pkg/discovery/discovery.go

-		if podStatus.Plugins[i].Plugin == pluginType {
-			podStatus.Plugins[i].ResultStatus = pluginResultStatus
-			podStatus.Plugins[i].ResultStatusCounts = pluginResultCounts
-			break


This was the main idea that was broken before. This was looping over the plugins and updating one value (the first that matched by name) and breaking the loop.

Instead you need to try and update every plugin in the list that matches on name. It is just a slight misnomer since podStatus.Plugins is really meaning podStatus.PluginNodePairs

This meant I had to add the code (which existed elsewhere) to just snip the item at the right spot to get the results for just a particular node.

johnSchnake · 2019-12-12T17:55:23Z

pkg/discovery/discovery.go

@@ -397,3 +413,24 @@ func setPodStatusAnnotation(client kubernetes.Interface, namespace string, statu
 	_, err = client.CoreV1().Pods(namespace).Patch(podName, types.MergePatchType, patchBytes)
 	return err
 }
+
+func getItemInTree(i *results.Item, root string) *results.Item {


This was copied from elsewhere; I considered putting it into results and exporting it; maybe even as method on the Item type. Let me know if you'd prefer that.

👍 I think having just one copy would be good.

johnSchnake · 2019-12-12T17:57:37Z

pkg/discovery/discovery.go

+		if podStatus.Plugins[i].Node == plugin.GlobalResult {
+			itemForNode = item
+		} else {
+			itemForNode = getItemInTree(item, podStatus.Plugins[i].Node)


Here I just traverse and choose the item/branch that matches the node name. It works and I think is sufficient, but I also considered adding some sort of metadata field so that we know that not only does name=NodeName but it has some type=hostname or something like that. Maybe not really necessary though.

I think it could help for readability, as I find myself getting lost in what some of these objects are and what they represent. I think just a comment would be enough though to inform readers of the code for next time that the Name field for a DS result item will be the node name.

Created a new issue; adding it to just nodes doesn't make as much sense as adding it to a few different things (overall summary, file summaries, etc) and so the change is a little more than I want to smush into this PR.

zubron

Thanks! I'm happy with this but have just left a couple of suggestions.

zubron · 2019-12-12T18:59:07Z

pkg/discovery/discovery.go

@@ -357,20 +355,38 @@ func updateStatus(client kubernetes.Interface, namespace string, status string,
 	return setPodStatusAnnotation(client, namespace, podStatus)
 }

-func updatePluginStatus(client kubernetes.Interface, namespace string, pluginType string, pluginResultStatus string, pluginResultCounts map[string]int) error {
+func updatePluginStatus(client kubernetes.Interface, namespace string, pluginType string, item results.Item) error {


Can we rename pluginType in the parameters to pluginName as that's what the value is here.

zubron · 2019-12-12T19:00:49Z

pkg/discovery/discovery.go

+	return setPodStatusAnnotation(client, namespace, podStatus)
+}
+
+func integrateResultsIntoStatus(podStatus *pluginaggregation.Status, pluginName string, item *results.Item) {
 	for i := range podStatus.Plugins {


Unrelated to this change, but the second value from range could be used here rather than needing to index into the slice with i.

Only for the value lookups; the range loop makes a local copy and so sometimes you think a value is editable but it isn't necessary depending on if its a pointer/value/whatever. Most times if I'm editing the value I just err on the side of caution and use the index.

Testing this out, it is one of the cases where the value doesnt get edited when you try and set the status/counts a few lines below.

Ah, I didn't know that. TIL!

zubron · 2019-12-12T19:28:07Z

pkg/discovery/discovery.go

+		if podStatus.Plugins[i].Node == plugin.GlobalResult {
+			itemForNode = item
+		} else {
+			itemForNode = getItemInTree(item, podStatus.Plugins[i].Node)


I think it could help for readability, as I find myself getting lost in what some of these objects are and what they represent. I think just a comment would be enough though to inform readers of the code for next time that the Name field for a DS result item will be the node name.

zubron · 2019-12-12T19:30:15Z

pkg/discovery/discovery.go

@@ -397,3 +413,24 @@ func setPodStatusAnnotation(client kubernetes.Interface, namespace string, statu
 	_, err = client.CoreV1().Pods(namespace).Patch(podName, types.MergePatchType, patchBytes)
 	return err
 }
+
+func getItemInTree(i *results.Item, root string) *results.Item {


👍 I think having just one copy would be good.

The bug would cause only one of the plugin/node values in the `sonobuoy status --json` output to be updated and it would include all the results for all nodes. Instead, we want each plugin/node value in that list to report its own values for passed/failed. Fixes #1001 Signed-off-by: John Schnake <jschnake@vmware.com>

johnSchnake · 2019-12-12T22:37:02Z

Waiting to go green; expect it to do so though.

As of now this is the output for the various status views:

$ sonobuoy status --json | jq
{
  "plugins": [
    {
      "plugin": "e2e",
      "node": "global",
      "status": "complete",
      "result-status": "passed",
      "result-counts": {
        "passed": 1,
        "skipped": 4896
      }
    },
    {
      "plugin": "systemd-logs",
      "node": "kind-control-plane",
      "status": "complete",
      "result-status": "passed",
      "result-counts": {
        "passed": 1
      }
    },
    {
      "plugin": "systemd-logs",
      "node": "kind-worker",
      "status": "complete",
      "result-status": "passed",
      "result-counts": {
        "passed": 1
      }
    }
  ],
  "status": "complete",
  "tar-info": {
    "name": "201912122232_sonobuoy_93015c64-5e06-446a-81b0-9262b3c78ff0.tar.gz",
    "created": "2019-12-12T22:32:40.2666856Z",
    "sha256": "d9cc3080c6e85da6f76e70028d52e7541d21c330c37594e8230b57cd7bd2d4bd",
    "size": 233587
  }
}

$ sonobuoy status
         PLUGIN     STATUS   RESULT   COUNT
            e2e   complete   passed       1
   systemd-logs   complete   passed       2

Sonobuoy has completed. Use `sonobuoy retrieve` to get results.

$ sonobuoy status --show-all
         PLUGIN                 NODE     STATUS   RESULT
            e2e               global   complete   passed
   systemd-logs   kind-control-plane   complete   passed
   systemd-logs          kind-worker   complete   passed

johnSchnake requested a review from zubron December 12, 2019 17:52

johnSchnake commented Dec 12, 2019

View reviewed changes

zubron approved these changes Dec 12, 2019

View reviewed changes

johnSchnake mentioned this pull request Dec 12, 2019

Add some metadata to result.Item to describe the type of entry #1047

Closed

johnSchnake merged commit 121ba8d into vmware-tanzu:master Dec 13, 2019

johnSchnake deleted the dsPluginResultsInStatus branch December 13, 2019 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug which caused daemonset status to not be updated with results #1045

Fix bug which caused daemonset status to not be updated with results #1045

johnSchnake commented Dec 12, 2019

johnSchnake Dec 12, 2019

johnSchnake Dec 12, 2019

zubron Dec 12, 2019

johnSchnake Dec 12, 2019

zubron Dec 12, 2019

johnSchnake Dec 12, 2019

zubron left a comment

zubron Dec 12, 2019

zubron Dec 12, 2019

johnSchnake Dec 12, 2019

zubron Dec 12, 2019 •

edited

Loading

zubron Dec 12, 2019

zubron Dec 12, 2019

johnSchnake commented Dec 12, 2019

Fix bug which caused daemonset status to not be updated with results #1045

Fix bug which caused daemonset status to not be updated with results #1045

Conversation

johnSchnake commented Dec 12, 2019

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

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

zubron Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnSchnake commented Dec 12, 2019

zubron Dec 12, 2019 •

edited

Loading