Remove use of global sonobuoy pod name

Previously the sonobuoy pod name was stored in a global variable and was updated before it was used. Now, instead of updating the global variable the function that determines the pod name returns it instead. Signed-off-by: Bridget McErlean <[email protected]>
vmware-tanzu · Jul 24, 2019 · 9971fbb · 9971fbb
1 parent 020a7e4
commit 9971fbb
Show file tree

Hide file tree

Showing 5 changed files with 114 additions and 24 deletions.
diff --git a/pkg/client/retrieve.go b/pkg/client/retrieve.go
@@ -60,12 +60,15 @@ func (c *SonobuoyClient) RetrieveResults(cfg *RetrieveConfig) (io.Reader, <-chan
 	}
 
 	// Determine sonobuoy pod name
-	pluginaggregation.SetStatusPodName(client, cfg.Namespace)
+	podName, err := pluginaggregation.GetStatusPodName(client, cfg.Namespace)
+	if err != nil {
+		return nil, nil, errors.Wrap(err, "failed to get the name of the aggregator pod to fetch results from")
+	}
 
 	restClient := client.CoreV1().RESTClient()
 	req := restClient.Post().
 		Resource("pods").
-		Name(pluginaggregation.StatusPodName).
+		Name(podName).
 		Namespace(cfg.Namespace).
 		SubResource("exec").
 		Param("container", config.MasterContainerName)

diff --git a/pkg/discovery/discovery.go b/pkg/discovery/discovery.go
@@ -96,9 +96,6 @@ func Run(restConf *rest.Config, cfg *config.Config) (errCount int) {
 		}
 	}
 
-	// Determine sonobuoy pod name
-	pluginaggregation.SetStatusPodName(kubeClient, cfg.Namespace)
-
 	// Set initial annotation stating the pod is running. Ensures the annotation
 	// exists sooner for user/polling consumption and prevents issues were we try
 	// to patch a non-existant status later.
@@ -110,8 +107,8 @@ func Run(restConf *rest.Config, cfg *config.Config) (errCount int) {
 	)
 
 	// 2. Get the list of namespaces and apply the regex filter on the namespace
-	logrus.Infof("Filtering namespaces based on the following regex:%s",  cfg.Filters.Namespaces)
-	nslist, err := FilterNamespaces(kubeClient,  cfg.Filters.Namespaces)
+	logrus.Infof("Filtering namespaces based on the following regex:%s", cfg.Filters.Namespaces)
+	nslist, err := FilterNamespaces(kubeClient, cfg.Filters.Namespaces)
 	if err != nil {
 		errlog.LogError(errors.Wrap(err, "could not filter namespaces"))
 		return errCount + 1
@@ -243,6 +240,12 @@ func setStatus(client kubernetes.Interface, namespace string, status *pluginaggr
 		return errors.Wrap(err, "failed to marshal the patch")
 	}
 
-	_, err = client.CoreV1().Pods(namespace).Patch(pluginaggregation.StatusPodName, types.MergePatchType, patchBytes)
+	// Determine sonobuoy pod name
+	podName, err := pluginaggregation.GetStatusPodName(client, namespace)
+	if err != nil {
+		return errors.Wrap(err, "failed to get the name of the aggregator pod to set the status on")
+	}
+
+	_, err = client.CoreV1().Pods(namespace).Patch(podName, types.MergePatchType, patchBytes)
 	return err
 }
diff --git a/pkg/plugin/aggregation/status.go b/pkg/plugin/aggregation/status.go
@@ -83,9 +83,12 @@ func GetStatus(client kubernetes.Interface, namespace string) (*Status, error) {
 	}
 
 	// Determine sonobuoy pod name
-	SetStatusPodName(client, namespace)
+	podName, err := GetStatusPodName(client, namespace)
+	if err != nil {
+		return nil, errors.Wrap(err, "failed to get the name of the aggregator pod to get the status from")
+	}
 
-	pod, err := client.CoreV1().Pods(namespace).Get(StatusPodName, metav1.GetOptions{})
+	pod, err := client.CoreV1().Pods(namespace).Get(podName, metav1.GetOptions{})
 	if err != nil {
 		return nil, errors.New("could not retrieve sonobuoy pod")
 	}

diff --git a/pkg/plugin/aggregation/update.go b/pkg/plugin/aggregation/update.go
@@ -33,10 +33,7 @@ import (
 const (
 	StatusAnnotationName = "sonobuoy.hept.io/status"
 	StatusPodLabel       = "run=sonobuoy-master"
-)
-
-var (
-	StatusPodName = "sonobuoy"
+	DefaultStatusPodName = "sonobuoy"
 )
 
 // node and name uniquely identify a single plugin result
@@ -120,7 +117,13 @@ func (u *updater) Annotate(results map[string]*plugin.Result) error {
 		return errors.Wrap(err, "couldn't encode patch")
 	}
 
-	_, err = u.client.CoreV1().Pods(u.namespace).Patch(StatusPodName, types.MergePatchType, bytes)
+	// Determine sonobuoy pod name
+	podName, err := GetStatusPodName(u.client, u.namespace)
+	if err != nil {
+		return errors.Wrap(err, "failed to get name of the aggregator pod to annotate")
+	}
+
+	_, err = u.client.CoreV1().Pods(u.namespace).Patch(podName, types.MergePatchType, bytes)
 	return errors.Wrap(err, "couldn't patch pod annotation")
 }
 
@@ -163,26 +166,25 @@ func GetPatch(annotation string) map[string]interface{} {
 	}
 }
 
-// SetStatusPodName sets the sonobuoy master pod name based on it's label.
-func SetStatusPodName(client kubernetes.Interface, namespace string) {
-
+// GetStatusPodName gets the sonobuoy master pod name based on its label.
+func GetStatusPodName(client kubernetes.Interface, namespace string) (string, error) {
 	listOptions := metav1.ListOptions{
 		LabelSelector: StatusPodLabel,
 	}
 
 	podList, err := client.CoreV1().Pods(namespace).List(listOptions)
 	if err != nil {
-		logrus.Errorf("Error listing pods with label '%s': %s", StatusPodLabel, err)
-		return
+		return "", errors.Wrap(err, "unable to list pods with label %q")
 	}
 
 	switch {
 	case len(podList.Items) == 0:
-		logrus.Errorf("No pods found with label '%s' in namespace %s, using default pod name 'sonobuoy'", StatusPodLabel, namespace)
+		logrus.Warningf("No pods found with label %q in namespace %s, using default pod name %q", StatusPodLabel, namespace, DefaultStatusPodName)
+		return DefaultStatusPodName, nil
 	case len(podList.Items) > 1:
-		logrus.Warningf("Found more than one pod with label '%s'. Using '%s'", StatusPodLabel, podList.Items[0].GetName())
-		StatusPodName = podList.Items[0].GetName()
+		logrus.Warningf("Found more than one pod with label %q. Using %q", StatusPodLabel, podList.Items[0].GetName())
+		return podList.Items[0].GetName(), nil
 	default:
-		StatusPodName = podList.Items[0].GetName()
+		return podList.Items[0].GetName(), nil
 	}
 }
diff --git a/pkg/plugin/aggregation/update_test.go b/pkg/plugin/aggregation/update_test.go
@@ -20,6 +20,13 @@ import (
 	"testing"
 
 	"github.com/heptio/sonobuoy/pkg/plugin"
+	"github.com/pkg/errors"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kuberuntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/fake"
+	k8stesting "k8s.io/client-go/testing"
 )
 
 func TestCreateUpdater(t *testing.T) {
@@ -47,3 +54,75 @@ func TestCreateUpdater(t *testing.T) {
 		t.Errorf("expected status to be failed, got %v", updater.status.Status)
 	}
 }
+
+func TestGetStatusPodName(t *testing.T) {
+	createPodWithRunLabel := func(name string) corev1.Pod {
+		return corev1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   name,
+				Labels: map[string]string{"run": "sonobuoy-master"},
+			},
+		}
+	}
+
+	testCases := []struct {
+		desc            string
+		podsOnServer    corev1.PodList
+		errFromServer   error
+		expectedPodName string
+	}{
+		{
+			desc:            "Error retrieving pods from server results in no pod name and an error being returned",
+			podsOnServer:    corev1.PodList{},
+			errFromServer:   errors.New("could not retrieve pods"),
+			expectedPodName: "",
+		},
+		{
+			desc:            "No pods results in default pod name being used",
+			podsOnServer:    corev1.PodList{},
+			errFromServer:   nil,
+			expectedPodName: "sonobuoy",
+		},
+		{
+			desc: "Only one pod results in that pod name being used",
+			podsOnServer: corev1.PodList{
+				Items: []corev1.Pod{
+					createPodWithRunLabel("sonobuoy-run-pod"),
+				},
+			},
+			errFromServer:   nil,
+			expectedPodName: "sonobuoy-run-pod",
+		},
+		{
+			desc: "More that one pod results in the first pod name being used",
+			podsOnServer: corev1.PodList{
+				Items: []corev1.Pod{
+					createPodWithRunLabel("sonobuoy-run-pod-1"),
+					createPodWithRunLabel("sonobuoy-run-pod-2"),
+				},
+			},
+			errFromServer:   nil,
+			expectedPodName: "sonobuoy-run-pod-1",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			fclient := fake.NewSimpleClientset()
+			fclient.PrependReactor("*", "*", func(action k8stesting.Action) (handled bool, ret kuberuntime.Object, err error) {
+				return true, &tc.podsOnServer, tc.errFromServer
+			})
+
+			podName, err := GetStatusPodName(fclient, "sonobuoy")
+			if tc.errFromServer == nil && err != nil {
+				t.Errorf("Unexpected error returned, expected nil but got %q", err)
+			}
+			if tc.errFromServer != nil && err == nil {
+				t.Errorf("Error not returned, expected %q but was nil", tc.errFromServer)
+			}
+			if podName != tc.expectedPodName {
+				t.Errorf("Incorrect pod name returned, expected %q but got %q", tc.expectedPodName, podName)
+			}
+		})
+	}
+}