Merge pull request #210 from weaveworks/nop-router

Kubernetes service reconciliation improvements

.circleci/config.yml

@@ -6,7 +6,7 @@ jobs:
       - checkout
       - run: test/e2e-kind.sh
       - run: test/e2e-istio.sh
-      - run: test/e2e-build.sh
+      - run: test/e2e-istio-build.sh
       - run: test/e2e-tests.sh
 
   e2e-smi-istio-testing:

pkg/canary/deployer.go

@@ -32,7 +32,7 @@ type Deployer struct {
 
 // Initialize creates the primary deployment, hpa,
 // scales to zero the canary deployment and returns the pod selector label and container ports
-func (c *Deployer) Initialize(cd *flaggerv1.Canary) (label string, ports *map[string]int32, err error) {
+func (c *Deployer) Initialize(cd *flaggerv1.Canary, skipLivenessChecks bool) (label string, ports *map[string]int32, err error) {
 	primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name)
 	label, ports, err = c.createPrimaryDeployment(cd)
 	if err != nil {
@@ -40,6 +40,13 @@ func (c *Deployer) Initialize(cd *flaggerv1.Canary) (label string, ports *map[st
 	}
 
 	if cd.Status.Phase == "" {
+		if !skipLivenessChecks {
+			_, readyErr := c.IsPrimaryReady(cd)
+			if readyErr != nil {
+				return "", ports, readyErr
+			}
+		}
+
 		c.Logger.With("canary", fmt.Sprintf("%s.%s", cd.Name, cd.Namespace)).Infof("Scaling down %s.%s", cd.Spec.TargetRef.Name, cd.Namespace)
 		if err := c.Scale(cd, 0); err != nil {
 			return "", ports, err

pkg/canary/deployer_test.go

@@ -9,7 +9,7 @@ import (
 
 func TestCanaryDeployer_Sync(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -95,7 +95,7 @@ func TestCanaryDeployer_Sync(t *testing.T) {
 
 func TestCanaryDeployer_IsNewSpec(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -118,7 +118,7 @@ func TestCanaryDeployer_IsNewSpec(t *testing.T) {
 
 func TestCanaryDeployer_Promote(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -163,7 +163,7 @@ func TestCanaryDeployer_Promote(t *testing.T) {
 
 func TestCanaryDeployer_IsReady(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Error("Expected primary readiness check to fail")
 	}
@@ -181,7 +181,7 @@ func TestCanaryDeployer_IsReady(t *testing.T) {
 
 func TestCanaryDeployer_SetFailedChecks(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -203,7 +203,7 @@ func TestCanaryDeployer_SetFailedChecks(t *testing.T) {
 
 func TestCanaryDeployer_SetState(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -225,7 +225,7 @@ func TestCanaryDeployer_SetState(t *testing.T) {
 
 func TestCanaryDeployer_SyncStatus(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Fatal(err.Error())
 	}
@@ -264,7 +264,7 @@ func TestCanaryDeployer_SyncStatus(t *testing.T) {
 
 func TestCanaryDeployer_Scale(t *testing.T) {
 	mocks := SetupMocks()
-	_, _, err := mocks.deployer.Initialize(mocks.canary)
+	_, _, err := mocks.deployer.Initialize(mocks.canary, true)
 	if err != nil {
 		t.Fatal(err.Error())
 	}

pkg/controller/scheduler.go

@@ -91,7 +91,12 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh
 	primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name)
 
 	// create primary deployment and hpa if needed
-	label, ports, err := c.deployer.Initialize(cd)
+	// skip primary check for Istio since the deployment will become ready after the ClusterIP are created
+	skipPrimaryCheck := false
+	if skipLivenessChecks || strings.Contains(c.meshProvider, "istio") {
+		skipPrimaryCheck = true
+	}
+	label, ports, err := c.deployer.Initialize(cd, skipPrimaryCheck)
 	if err != nil {
 		c.recordEventWarningf(cd, "%v", err)
 		return
@@ -269,7 +274,7 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh
 	}
 
 	// canary fix routing: A/B testing
-	if len(cd.Spec.CanaryAnalysis.Match) > 0 {
+	if len(cd.Spec.CanaryAnalysis.Match) > 0 || cd.Spec.CanaryAnalysis.Iterations > 0 {
 		// route traffic to canary and increment iterations
 		if cd.Spec.CanaryAnalysis.Iterations > cd.Status.Iterations {
 			if err := meshRouter.SetRoutes(cd, 0, 100); err != nil {
@@ -614,7 +619,7 @@ func (c *Controller) analyseCanary(r *flaggerv1.Canary) bool {
 					c.recordEventWarningf(r, "Halt advancement no values found for metric %s probably %s.%s is not receiving traffic",
 						metric.Name, r.Spec.TargetRef.Name, r.Namespace)
 				} else {
-					c.recordEventErrorf(r, "Metrics server %s query failed: %v", c.observerFactory.Client.GetMetricsServer(), err)
+					c.recordEventErrorf(r, "Metrics server %s query failed for %s: %v", c.observerFactory.Client.GetMetricsServer(), metric.Name, err)
 				}
 				return false
 			}

pkg/metrics/client.go

@@ -183,5 +183,5 @@ func (p *PrometheusClient) IsOnline() (bool, error) {
 }
 
 func (p *PrometheusClient) GetMetricsServer() string {
-	return p.url.RawQuery
+	return p.url.String()
 }

pkg/router/kubernetes.go

@@ -128,10 +128,13 @@ func (c *KubernetesRouter) reconcileService(canary *flaggerv1.Canary, name strin
 	}
 
 	if svc != nil {
-		if diff := cmp.Diff(svcSpec.Ports, svc.Spec.Ports); diff != "" {
+		portsDiff := cmp.Diff(svcSpec.Ports, svc.Spec.Ports)
+		selectorsDiff := cmp.Diff(svcSpec.Selector, svc.Spec.Selector)
+
+		if portsDiff != "" || selectorsDiff != "" {
 			svcClone := svc.DeepCopy()
-			svcClone.Spec = svcSpec
-			svcClone.Spec.ClusterIP = svc.Spec.ClusterIP
+			svcClone.Spec.Ports = svcSpec.Ports
+			svcClone.Spec.Selector = svcSpec.Selector
 			_, err = c.kubeClient.CoreV1().Services(canary.Namespace).Update(svcClone)
 			if err != nil {
 				return fmt.Errorf("service %s update error %v", name, err)

test/README.md

@@ -11,9 +11,9 @@ The e2e testing infrastructure is powered by CircleCI and [Kubernetes Kind](http
 * deploy Tiller on the local cluster [e2e-istio.sh](e2e-istio.sh)
 * install Istio CRDs with Helm [e2e-istio.sh](e2e-istio.sh)
 * install Istio control plane and Prometheus with Helm [e2e-istio.sh](e2e-istio.sh)
-* build Flagger container image [e2e-build.sh](e2e-build.sh)
-* load Flagger image onto the local cluster [e2e-build.sh](e2e-build.sh)
-* deploy Flagger in the istio-system namespace [e2e-build.sh](e2e-build.sh)
+* build Flagger container image [e2e-istio-build.sh](e2e-istio-build.sh)
+* load Flagger image onto the local cluster [e2e-istio-build.sh.sh](e2e-istio-build.sh)
+* deploy Flagger in the istio-system namespace [e2e-istio-build.sh.sh](e2e-istio-build.sh)
 * create a test namespace with Istio injection enabled [e2e-tests.sh](e2e-tests.sh)
 * deploy the load tester in the test namespace [e2e-tests.sh](e2e-tests.sh)
 * deploy a demo workload (podinfo) in the test namespace [e2e-tests.sh](e2e-tests.sh)

test/e2e-istio-build.sh

@@ -0,0 +1,20 @@
+#!/usr/bin/env bash
+
+set -o errexit
+
+REPO_ROOT=$(git rev-parse --show-toplevel)
+export KUBECONFIG="$(kind get kubeconfig-path --name="kind")"
+
+echo '>>> Building Flagger'
+cd ${REPO_ROOT} && docker build -t test/flagger:latest . -f Dockerfile
+
+echo '>>> Load Flagger image in Kind'
+kind load docker-image test/flagger:latest
+
+echo '>>> Installing Flagger'
+helm upgrade -i flagger ${REPO_ROOT}/charts/flagger \
+--namespace istio-system \
+--set meshProvider=istio
+
+kubectl -n istio-system set image deployment/flagger flagger=test/flagger:latest
+kubectl -n istio-system rollout status deployment/flagger