Repository: linkerd/linkerd2
Pull request #6212 (2021-06-01 22:27:46)

Only check proxy version of running pods
This changes the data plane and control plane checks to only consider pods that
are in a running state.

This is currently an issue in scenarios when upgrading and an old pod is still
terminating while the upgraded pod is already running. Running the `check`
command will report that the old pod has an unexpected proxy version even though
it is terminating; it should not warn in this case.

Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com>


kleimkuhler: Unfortunately this check did not end up working because the issue was not with terminating pods being checked, but that pods that were mid rollout can end up in a state where both the old and new replicas are running.

This can occur if an old replica is running (say Prometheus with 2/2 containers), and it's new replica is 1/2 containers. These both report as running to Kubernetes, so they both end up in the `check` command output.

While it's still worth keeping the running check, ultimately we just have to wait for the linkerd-viz deployment rollouts like we do for the core install.

`WaitRollout` now takes a `DeploySpec` map and waits for each deployment in that map. I've added a new `LinkerdVizDeployReplicas` which contains the deployments we want to wait for after a linkerd-viz installation.

Patch:
diff --git a/bin/image-load b/bin/image-load
index aabf41d21..909c5c918 100755
--- a/bin/image-load
+++ b/bin/image-load
@@ -4,31 +4,51 @@ set -eo pipefail
 
 kind=""
 k3d=""
+cluster=""
 archive=""
+images_default=(proxy controller web metrics-api grafana debug cni-plugin jaeger-webhook tap)
+images=()
+
+usage() {
+  printf "Load into KinD/k3d the referred Linkerd images. If no images are specified all of them are loaded (%s)\n" "${images_default[*]}"
+  echo ""
+  echo "Usage:"
+  echo "    bin/image-load [--kind] [--k3d] [--cluster name] [--archive] [image] [image]..."
+  echo ""
+  echo "Examples:"
+  echo ""
+  echo "    # Load all the images from the local docker instance into KinD"
+  echo "    bin/image-load"
+  echo ""
+  echo "    # Load only the proxy and controller images into k3d"
+  echo "    bin/image-load --k3d proxy controller"
+  echo ""
+  echo "    # Load all the images from tar files located under the 'image-archives' directory into k3d"
+  echo "    bin/image-load --k3d --archive"
+  echo ""
+  echo "Available Commands:"
+  echo "    --cluster: target cluster name (defaults to 'k3s-default' under k3d, and 'kind' under KinD)."
+  echo "    --kind: use a KinD cluster (default)."
+  echo "    --k3d: use a k3d cluster."
+  echo "    --archive: load the images from local .tar files in the current directory."
+}
 
 while :
 do
   case $1 in
     -h|--help)
-      echo "Load into KinD/k3d the images for Linkerd's proxy, controller, metrics-api, web, grafana, cli-bin, tap, debug and cni-plugin."
-      echo ""
-      echo "Usage:"
-      echo "    bin/image-load [--kind] [--k3d] [--archive]"
-      echo ""
-      echo "Examples:"
-      echo ""
-      echo "    # Load images from the local docker instance into KinD"
-      echo "    bin/image-load"
-      echo ""
-      echo "    # Load images from tar files located under the 'image-archives' directory into k3d"
-      echo "    bin/image-load --k3d --archive"
-      echo ""
-      echo "Available Commands:"
-      echo "    --kind: use a KinD cluster (default)."
-      echo "    --k3d: use a k3d cluster."
-      echo "    --archive: load the images from local .tar files in the current directory."
+      usage
       exit 0
       ;;
+    --cluster)
+      cluster=$2
+      if [ -z "$cluster" ]; then
+        echo 'Error: the argument for --cluster was not specified' >&2
+        usage
+        exit 1
+      fi
+      shift
+      ;;
     --kind)
       kind=1
       ;;
@@ -39,26 +59,43 @@ do
       archive=1
       ;;
     *)
-      break
+      if [ -z "$1" ]; then
+        break
+      fi
+      if echo "$1" | grep -q '^-.*' ; then
+        echo "Unexpected flag: $1" >&2
+        usage
+        exit 1
+      fi
+      images+=("$1")
   esac
   shift
 done
 
+if [ ${#images[@]} -eq 0 ]; then
+  images=("${images_default[@]}")
+fi
+
 bindir=$( cd "${0%/*}" && pwd )
 
 if [ -n "$k3d" ]; then
   if [ -n "$kind" ]; then
     echo $k3d
     echo "Error: --kind and --k3d can't be used simultaneously" >&2
+    usage
     exit 1
   fi
-  cluster=${1:-"k3s-default"}
+  if [ -z "$cluster" ]; then
+    cluster="k3s-default"
+  fi
   bin="$bindir"/k3d
   image_sub_cmd=(image import -c "$cluster")
 else
   kind=1
-  cluster=${1:-"kind"}
   bin="$bindir"/kind
+  if [ -z "$cluster" ]; then
+    cluster="kind"
+  fi
   if [ $archive ]; then
     image_sub_cmd=(load image-archive --name "$cluster")
   else
@@ -79,23 +116,30 @@ fi
 "$bin" version
 
 rm -f load_fail
-for img in proxy controller web metrics-api grafana debug cni-plugin jaeger-webhook tap; do
-  printf 'Importing %s...\n' $img
+for i in "${!images[@]}"; do
   if [ $archive ]; then
-    param="image-archives/$img.tar"
+    param="image-archives/${images[$i]}.tar"
   else
-    param="$DOCKER_REGISTRY/$img:$TAG"
+    param="$DOCKER_REGISTRY/${images[$i]}:$TAG"
   fi
 
   if [ -n "$kind" ]; then
+    printf 'Importing %s...\n' "${images[$i]}"
     "$bin" "${image_sub_cmd[@]}" "${param[@]}" || touch load_fail &
   else
     # "k3d image import" commands don't behave well when parallelized
-    "$bin" "${image_sub_cmd[@]}" "${param[@]}"
+    # but all the images can be loaded in a single invocation
+    images[$i]=$param
   fi
 done
 
-wait < <(jobs -p)
+if [ -n "$kind" ]; then
+  wait < <(jobs -p)
+else
+  printf 'Importing %s...\n' "${images[@]}"
+  "$bin" "${image_sub_cmd[@]}" "${images[@]}"
+fi
+
 if [ -f load_fail ]; then
   echo "Loading docker images into the cluster failed."
   rm load_fail
diff --git a/bin/install-pr b/bin/install-pr
index 0fd289d72..b06a7f443 100755
--- a/bin/install-pr
+++ b/bin/install-pr
@@ -102,7 +102,7 @@ then
   then
     distro="kind"
   fi
-  "$bindir"/image-load --"$distro" --archive "$cluster"
+  "$bindir"/image-load --"$distro" --archive --cluster "$cluster"
 else
   for image in cni-plugin controller debug grafana proxy web
   do
diff --git a/pkg/healthcheck/healthcheck.go b/pkg/healthcheck/healthcheck.go
index 0664798f8..a94beccb7 100644
--- a/pkg/healthcheck/healthcheck.go
+++ b/pkg/healthcheck/healthcheck.go
@@ -1429,7 +1429,8 @@ func (hc *HealthChecker) CheckProxyVersionsUpToDate(pods []corev1.Pod) error {
 func CheckProxyVersionsUpToDate(pods []corev1.Pod, versions version.Channels) error {
 	outdatedPods := []string{}
 	for _, pod := range pods {
-		if containsProxy(pod) {
+		status := k8s.GetPodStatus(pod)
+		if status == string(corev1.PodRunning) && containsProxy(pod) {
 			proxyVersion := k8s.GetProxyVersion(pod)
 			if err := versions.Match(proxyVersion); err != nil {
 				outdatedPods = append(outdatedPods, fmt.Sprintf("\t* %s (%s)", pod.Name, proxyVersion))
@@ -2453,7 +2454,7 @@ func getPodStatuses(pods []corev1.Pod) map[string]map[string][]corev1.ContainerS
 func validateControlPlanePods(pods []corev1.Pod) error {
 	statuses := getPodStatuses(pods)
 
-	names := []string{"identity", "proxy-injector"}
+	names := []string{"destination", "identity", "proxy-injector"}
 
 	for _, name := range names {
 		pods, found := statuses[name]
@@ -2506,7 +2507,7 @@ func validateDataPlanePods(pods []corev1.Pod, targetNamespace string) error {
 			continue
 		}
 
-		if status != "Running" && status != "Evicted" {
+		if status != string(corev1.PodRunning) && status != "Evicted" {
 			return fmt.Errorf("The \"%s\" pod is not running", pod.Name)
 		}
 
@@ -2580,7 +2581,7 @@ func CheckPodsRunning(pods []corev1.Pod, podsNotFoundMsg string) error {
 		return fmt.Errorf(podsNotFoundMsg)
 	}
 	for _, pod := range pods {
-		if pod.Status.Phase != "Running" {
+		if pod.Status.Phase != corev1.PodRunning {
 			return fmt.Errorf("%s status is %s", pod.Name, pod.Status.Phase)
 		}
 


Test patch:
diff --git a/bin/_test-helpers.sh b/bin/_test-helpers.sh
index 2be4a4c10..6a65ad5f0 100644
--- a/bin/_test-helpers.sh
+++ b/bin/_test-helpers.sh
@@ -277,11 +277,11 @@ image_load() {
   cluster_name=$1
   case $images in
     docker)
-      "$bindir"/image-load --k3d "$cluster_name"
+      "$bindir"/image-load --k3d --cluster "$cluster_name"
       exit_on_err "error calling '$bindir/image-load'"
       ;;
     archive)
-      "$bindir"/image-load --k3d --archive "$cluster_name"
+      "$bindir"/image-load --k3d --archive --cluster "$cluster_name"
       exit_on_err "error calling '$bindir/image-load'"
       ;;
   esac
diff --git a/pkg/healthcheck/healthcheck_test.go b/pkg/healthcheck/healthcheck_test.go
index ee3c9ae8f..ec2566248 100644
--- a/pkg/healthcheck/healthcheck_test.go
+++ b/pkg/healthcheck/healthcheck_test.go
@@ -1323,6 +1323,7 @@ func TestValidateControlPlanePods(t *testing.T) {
 	t.Run("Returns an error if not all pods are running", func(t *testing.T) {
 		pods := []corev1.Pod{
 			pod("linkerd-grafana-5b7d796646-hh46d", corev1.PodRunning, true),
+			pod("linkerd-destination-9849948665-37082", corev1.PodFailed, true),
 			pod("linkerd-identity-6849948664-27982", corev1.PodFailed, true),
 		}
 
@@ -1330,7 +1331,7 @@ func TestValidateControlPlanePods(t *testing.T) {
 		if err == nil {
 			t.Fatal("Expected error, got nothing")
 		}
-		if err.Error() != "No running pods for \"linkerd-identity\"" {
+		if err.Error() != "No running pods for \"linkerd-destination\"" {
 			t.Fatalf("Unexpected error message: %s", err.Error())
 		}
 	})
@@ -1349,6 +1350,7 @@ func TestValidateControlPlanePods(t *testing.T) {
 
 	t.Run("Returns nil if all pods are running and all containers are ready", func(t *testing.T) {
 		pods := []corev1.Pod{
+			pod("linkerd-destination-9849948665-37082", corev1.PodRunning, true),
 			pod("linkerd-identity-6849948664-27982", corev1.PodRunning, true),
 			pod("linkerd-proxy-injector-5f79ff4844-", corev1.PodRunning, true),
 		}
@@ -1361,6 +1363,9 @@ func TestValidateControlPlanePods(t *testing.T) {
 
 	t.Run("Returns nil if, HA mode, at least one pod of each control plane component is ready", func(t *testing.T) {
 		pods := []corev1.Pod{
+			pod("linkerd-destination-9843948665-48082", corev1.PodRunning, true),
+			pod("linkerd-destination-9843948665-48083", corev1.PodRunning, false),
+			pod("linkerd-destination-9843948665-48084", corev1.PodFailed, false),
 			pod("linkerd-identity-6849948664-27982", corev1.PodRunning, true),
 			pod("linkerd-identity-6849948664-27983", corev1.PodRunning, false),
 			pod("linkerd-identity-6849948664-27984", corev1.PodFailed, false),
@@ -1375,6 +1380,7 @@ func TestValidateControlPlanePods(t *testing.T) {
 
 	t.Run("Returns nil if all linkerd pods are running and pod list includes non-linkerd pod", func(t *testing.T) {
 		pods := []corev1.Pod{
+			pod("linkerd-destination-9843948665-48082", corev1.PodRunning, true),
 			pod("linkerd-identity-6849948664-27982", corev1.PodRunning, true),
 			pod("linkerd-proxy-injector-5f79ff4844-", corev1.PodRunning, true),
 			pod("hello-43c25d", corev1.PodRunning, true),
diff --git a/test/integration/install_test.go b/test/integration/install_test.go
index dc6321823..31762b9e8 100644
--- a/test/integration/install_test.go
+++ b/test/integration/install_test.go
@@ -430,7 +430,7 @@ func TestInstallOrUpgradeCli(t *testing.T) {
 			"'kubectl apply' command failed\n%s", cmdOut)
 	}
 
-	TestHelper.WaitRollout(t)
+	TestHelper.WaitRollout(t, testutil.LinkerdDeployReplicasEdge)
 
 	if TestHelper.ExternalPrometheus() {
 
@@ -462,6 +462,17 @@ func TestInstallOrUpgradeCli(t *testing.T) {
 		testutil.AnnotatedFatalf(t, "'kubectl apply' command failed",
 			"'kubectl apply' command failed\n%s", out)
 	}
+
+	// It is necessary to clone LinkerdVizDeployReplicas so that we do not
+	// mutate it's original value.
+	expectedVizDeployments := make(map[string]testutil.DeploySpec)
+	for k, v := range testutil.LinkerdVizDeployReplicas {
+		expectedVizDeployments[k] = v
+	}
+	if TestHelper.ExternalPrometheus() {
+		delete(expectedVizDeployments, "prometheus")
+	}
+	TestHelper.WaitRollout(t, expectedVizDeployments)
 }
 
 // These need to be updated (if there are changes) once a new stable is released
@@ -535,13 +546,23 @@ func TestInstallHelm(t *testing.T) {
 		testutil.AnnotatedFatalf(t, "'helm install' command failed",
 			"'helm install' command failed\n%s\n%s", stdout, stderr)
 	}
-
-	TestHelper.WaitRollout(t)
+	TestHelper.WaitRollout(t, testutil.LinkerdDeployReplicasEdge)
 
 	if stdout, stderr, err := TestHelper.HelmCmdPlain("install", vizChartToInstall, "l5d-viz", vizArgs...); err != nil {
 		testutil.AnnotatedFatalf(t, "'helm install' command failed",
 			"'helm install' command failed\n%s\n%s", stdout, stderr)
 	}
+
+	// It is necessary to clone LinkerdVizDeployReplicas so that we do not
+	// mutate it's original value.
+	expectedVizDeployments := make(map[string]testutil.DeploySpec)
+	for k, v := range testutil.LinkerdVizDeployReplicas {
+		expectedVizDeployments[k] = v
+	}
+	if TestHelper.ExternalPrometheus() {
+		delete(expectedVizDeployments, "prometheus")
+	}
+	TestHelper.WaitRollout(t, expectedVizDeployments)
 }
 
 func TestControlPlaneResourcesPostInstall(t *testing.T) {
@@ -637,12 +658,25 @@ func TestUpgradeHelm(t *testing.T) {
 		testutil.AnnotatedFatalf(t, "'helm upgrade' command failed",
 			"'helm upgrade' command failed\n%s\n%s", stdout, stderr)
 	}
+	TestHelper.WaitRollout(t, testutil.LinkerdDeployReplicasEdge)
 
 	vizChart := TestHelper.GetLinkerdVizHelmChart()
 	if stdout, stderr, err := TestHelper.HelmCmdPlain("upgrade", vizChart, "l5d-viz", vizArgs...); err != nil {
 		testutil.AnnotatedFatalf(t, "'helm upgrade' command failed",
 			"'helm upgrade' command failed\n%s\n%s", stdout, stderr)
 	}
+
+	// It is necessary to clone LinkerdVizDeployReplicas so that we do not
+	// mutate it's original value.
+	expectedVizDeployments := make(map[string]testutil.DeploySpec)
+	for k, v := range testutil.LinkerdVizDeployReplicas {
+		expectedVizDeployments[k] = v
+	}
+	if TestHelper.ExternalPrometheus() {
+		delete(expectedVizDeployments, "prometheus")
+	}
+	TestHelper.WaitRollout(t, expectedVizDeployments)
+
 	TestHelper.AddInstalledExtension(vizExtensionName)
 }
 
@@ -943,12 +977,18 @@ func getCheckOutput(t *testing.T, goldenFile string, namespace string) string {
 		testutil.AnnotatedFatal(t, fmt.Sprintf("failed to retrieve pods: %s", err), err)
 	}
 
+	proxyVersionErr := ""
+	err = healthcheck.CheckProxyVersionsUpToDate(pods, version.Channels{})
+	if err != nil {
+		proxyVersionErr = err.Error()
+	}
+
 	tpl := template.Must(template.ParseFiles("testdata" + "/" + goldenFile))
 	vars := struct {
 		ProxyVersionErr string
 		HintURL         string
 	}{
-		healthcheck.CheckProxyVersionsUpToDate(pods, version.Channels{}).Error(),
+		proxyVersionErr,
 		healthcheck.HintBaseURL(TestHelper.GetVersion()),
 	}
 
diff --git a/testutil/kubernetes_helper.go b/testutil/kubernetes_helper.go
index 8953c7b04..64d518f53 100644
--- a/testutil/kubernetes_helper.go
+++ b/testutil/kubernetes_helper.go
@@ -320,17 +320,15 @@ func (h *KubernetesHelper) URLFor(ctx context.Context, namespace, deployName str
 	return pf.URLFor(""), nil
 }
 
-// WaitRollout blocks until all the deployments in the linkerd namespace have been
-// completely rolled out (and their pods are ready)
-func (h *KubernetesHelper) WaitRollout(t *testing.T) {
-	for deploy, deploySpec := range LinkerdDeployReplicasEdge {
-		if deploySpec.Namespace == "linkerd" {
-			o, err := h.Kubectl("", "--namespace=linkerd", "rollout", "status", "--timeout=60m", "deploy/"+deploy)
-			if err != nil {
-				AnnotatedFatalf(t,
-					fmt.Sprintf("failed to wait rollout of deploy/%s", deploy),
-					"failed to wait for rollout of deploy/%s: %s: %s", deploy, err, o)
-			}
+// WaitRollout blocks until all the given deployments have been completely
+// rolled out (and their pods are ready)
+func (h *KubernetesHelper) WaitRollout(t *testing.T, deploys map[string]DeploySpec) {
+	for deploy, deploySpec := range deploys {
+		o, err := h.Kubectl("", "--namespace="+deploySpec.Namespace, "rollout", "status", "--timeout=60m", "deploy/"+deploy)
+		if err != nil {
+			AnnotatedFatalf(t,
+				fmt.Sprintf("failed to wait rollout of deploy/%s", deploy),
+				"failed to wait for rollout of deploy/%s: %s: %s", deploy, err, o)
 		}
 	}
 }
diff --git a/testutil/test_helper.go b/testutil/test_helper.go
index 899be08cb..10073ff29 100644
--- a/testutil/test_helper.go
+++ b/testutil/test_helper.go
@@ -67,13 +67,10 @@ type Service struct {
 }
 
 // LinkerdDeployReplicasEdge is a map containing the number of replicas for each Deployment and the main
-// container name, in the current code-base
+// container name in the current core installation
 var LinkerdDeployReplicasEdge = map[string]DeploySpec{
 	"linkerd-destination":    {"linkerd", 1},
-	"tap":                    {"linkerd-viz", 1},
-	"grafana":                {"linkerd-viz", 1},
 	"linkerd-identity":       {"linkerd", 1},
-	"web":                    {"linkerd-viz", 1},
 	"linkerd-proxy-injector": {"linkerd", 1},
 }
 
@@ -81,6 +78,18 @@ var LinkerdDeployReplicasEdge = map[string]DeploySpec{
 // container name. Override whenever edge deviates from stable.
 var LinkerdDeployReplicasStable = LinkerdDeployReplicasEdge
 
+// LinkerdVizDeployReplicas is a map containing the number of replicas for
+// each Deployment and the main container name in the current linkerd-viz
+// installation
+var LinkerdVizDeployReplicas = map[string]DeploySpec{
+	"prometheus":   {"linkerd-viz", 1},
+	"metrics-api":  {"linkerd-viz", 1},
+	"grafana":      {"linkerd-viz", 1},
+	"tap":          {"linkerd-viz", 1},
+	"tap-injector": {"linkerd-viz", 1},
+	"web":          {"linkerd-viz", 1},
+}
+
 // NewGenericTestHelper returns a new *TestHelper from the options provided as function parameters.
 // This helper was created to be able to reuse this package without hard restrictions
 // as seen in `NewTestHelper()` which is primarily used with integration tests