From f25023ed1bae7656ea8fd5d4e2ecad4c0250eb84 Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Tue, 18 Jun 2019 17:57:00 +0300 Subject: [PATCH 1/6] Include selector in service reconciliation - detect changes in the Kubernetes service selectors and ports - preserve the immutable fields when updating the ClusterIP services --- pkg/router/kubernetes.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pkg/router/kubernetes.go b/pkg/router/kubernetes.go index 8760a41f..07710fcc 100644 --- a/pkg/router/kubernetes.go +++ b/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) From 9a87d47f45b3cd661e811b0e9cda0e62835b462b Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Wed, 19 Jun 2019 09:49:25 +0300 Subject: [PATCH 2/6] Check primary readiness on initialisation Wait for the primary to become ready before scaling down the canary in the init phase --- pkg/canary/deployer.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/canary/deployer.go b/pkg/canary/deployer.go index fdeb2fbc..83e549de 100644 --- a/pkg/canary/deployer.go +++ b/pkg/canary/deployer.go @@ -40,6 +40,11 @@ func (c *Deployer) Initialize(cd *flaggerv1.Canary) (label string, ports *map[st } if cd.Status.Phase == "" { + _, 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 From 8c59e9d2b492257efcd43d53c6f64a9adce417dd Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Wed, 19 Jun 2019 10:30:19 +0300 Subject: [PATCH 3/6] Fix metrics URL getter --- pkg/metrics/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/metrics/client.go b/pkg/metrics/client.go index 6f6615df..97647ccc 100644 --- a/pkg/metrics/client.go +++ b/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() } From 98beb1011e87172f2b783817dc5ba0ccc2b8228e Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Wed, 19 Jun 2019 10:50:55 +0300 Subject: [PATCH 4/6] Skip primary check on init when using Istio The deployment will become ready after the ClusterIP are created --- pkg/canary/deployer.go | 10 ++++++---- pkg/canary/deployer_test.go | 16 ++++++++-------- pkg/controller/scheduler.go | 11 ++++++++--- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/pkg/canary/deployer.go b/pkg/canary/deployer.go index 83e549de..4b8ea18b 100644 --- a/pkg/canary/deployer.go +++ b/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,9 +40,11 @@ func (c *Deployer) Initialize(cd *flaggerv1.Canary) (label string, ports *map[st } if cd.Status.Phase == "" { - _, readyErr := c.IsPrimaryReady(cd) - if readyErr != nil { - return "", ports, readyErr + 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) diff --git a/pkg/canary/deployer_test.go b/pkg/canary/deployer_test.go index cba0bb5d..d6164dd8 100644 --- a/pkg/canary/deployer_test.go +++ b/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()) } diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 777f27ab..421632f3 100644 --- a/pkg/controller/scheduler.go +++ b/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 || 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 } From dc3cde88d23edff3946aed46ebd40cfb3620015a Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Wed, 19 Jun 2019 11:03:44 +0300 Subject: [PATCH 5/6] Use Helm to install Flagger for Istio e2e tests --- .circleci/config.yml | 2 +- test/README.md | 6 +++--- test/e2e-istio-build.sh | 20 ++++++++++++++++++++ 3 files changed, 24 insertions(+), 4 deletions(-) create mode 100755 test/e2e-istio-build.sh diff --git a/.circleci/config.yml b/.circleci/config.yml index 07411ff4..9c934cd1 100644 --- a/.circleci/config.yml +++ b/.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: diff --git a/test/README.md b/test/README.md index 43c04b0a..57bc3950 100644 --- a/test/README.md +++ b/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) diff --git a/test/e2e-istio-build.sh b/test/e2e-istio-build.sh new file mode 100755 index 00000000..4be983b4 --- /dev/null +++ b/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 From bf7ebc97082dc1e8538a6d772ab3fb4fde605d46 Mon Sep 17 00:00:00 2001 From: stefanprodan Date: Wed, 19 Jun 2019 11:16:11 +0300 Subject: [PATCH 6/6] Skip readiness check on init for Istio SMI --- pkg/controller/scheduler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/controller/scheduler.go b/pkg/controller/scheduler.go index 421632f3..a6f9ddd3 100644 --- a/pkg/controller/scheduler.go +++ b/pkg/controller/scheduler.go @@ -93,7 +93,7 @@ func (c *Controller) advanceCanary(name string, namespace string, skipLivenessCh // create primary deployment and hpa if needed // skip primary check for Istio since the deployment will become ready after the ClusterIP are created skipPrimaryCheck := false - if skipLivenessChecks || c.meshProvider == "istio" { + if skipLivenessChecks || strings.Contains(c.meshProvider, "istio") { skipPrimaryCheck = true } label, ports, err := c.deployer.Initialize(cd, skipPrimaryCheck)