Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/{canary,controller}: remove unused skipLivenessChecks #530

Merged
merged 1 commit into from
Mar 28, 2020
Merged

pkg/{canary,controller}: remove unused skipLivenessChecks #530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions pkg/canary/config_tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ func TestConfigTracker_ConfigMaps(t *testing.T) {
configMap := newDeploymentControllerTestConfigMap()
configMapProjected := newDeploymentControllerTestConfigProjected()

err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.initializeCanary(t)

depPrimary, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo-primary", metav1.GetOptions{})
require.NoError(t, err)
Expand Down Expand Up @@ -53,7 +52,7 @@ func TestConfigTracker_ConfigMaps(t *testing.T) {
configMap := newDaemonSetControllerTestConfigMap()
configMapProjected := newDaemonSetControllerTestConfigProjected()

err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

depPrimary, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo-primary", metav1.GetOptions{})
Expand Down Expand Up @@ -93,8 +92,7 @@ func TestConfigTracker_Secrets(t *testing.T) {
secret := newDeploymentControllerTestSecret()
secretProjected := newDeploymentControllerTestSecretProjected()

err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.initializeCanary(t)

depPrimary, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo-primary", metav1.GetOptions{})
if assert.NoError(t, err) {
Expand Down Expand Up @@ -131,8 +129,7 @@ func TestConfigTracker_Secrets(t *testing.T) {
secret := newDaemonSetControllerTestSecret()
secretProjected := newDaemonSetControllerTestSecretProjected()

err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.controller.Initialize(mocks.canary)

daePrimary, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo-primary", metav1.GetOptions{})
if assert.NoError(t, err) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/canary/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ type Controller interface {
SetStatusWeight(canary *flaggerv1.Canary, val int) error
SetStatusIterations(canary *flaggerv1.Canary, val int) error
SetStatusPhase(canary *flaggerv1.Canary, phase flaggerv1.CanaryPhase) error
Initialize(canary *flaggerv1.Canary, skipLivenessChecks bool) error
Initialize(canary *flaggerv1.Canary) error
Promote(canary *flaggerv1.Canary) error
HasTargetChanged(canary *flaggerv1.Canary) (bool, error)
HaveDependenciesChanged(canary *flaggerv1.Canary) (bool, error)
Expand Down
4 changes: 2 additions & 2 deletions pkg/canary/daemonset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,14 @@ func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error {

// Initialize creates the primary DaemonSet, scales down the canary DaemonSet,
// and returns the pod selector label and container ports
func (c *DaemonSetController) Initialize(cd *flaggerv1.Canary, skipLivenessChecks bool) (err error) {
func (c *DaemonSetController) Initialize(cd *flaggerv1.Canary) (err error) {
err = c.createPrimaryDaemonSet(cd)
if err != nil {
return fmt.Errorf("createPrimaryDaemonSet failed: %w", err)
}

if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing {
if !skipLivenessChecks && !cd.SkipAnalysis() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the SkipAnalysis condition changes the current behaviour. Is this intentional? if so please add a separate commit with it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I misunderstood this condition - this is not intentional so I am going to revert this change

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

force pushed the reverted version

if !cd.SkipAnalysis() {
if err := c.IsPrimaryReady(cd); err != nil {
return fmt.Errorf("IsPrimaryReady failed: %w", err)
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/canary/daemonset_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import (

func TestDaemonSetController_Sync(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

daePrimary, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo-primary", metav1.GetOptions{})
Expand All @@ -29,7 +29,7 @@ func TestDaemonSetController_Sync(t *testing.T) {

func TestDaemonSetController_Promote(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

dae2 := newDaemonSetControllerTestPodInfoV2()
Expand Down Expand Up @@ -60,7 +60,7 @@ func TestDaemonSetController_NoConfigTracking(t *testing.T) {
mocks := newDaemonSetFixture()
mocks.controller.configTracker = &NopTracker{}

err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

daePrimary, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo-primary", metav1.GetOptions{})
Expand All @@ -75,7 +75,7 @@ func TestDaemonSetController_NoConfigTracking(t *testing.T) {

func TestDaemonSetController_HasTargetChanged(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

// save last applied hash
Expand Down Expand Up @@ -163,7 +163,7 @@ func TestDaemonSetController_HasTargetChanged(t *testing.T) {
func TestDaemonSetController_Scale(t *testing.T) {
t.Run("ScaleToZero", func(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

err = mocks.controller.ScaleToZero(mocks.canary)
Expand All @@ -179,7 +179,7 @@ func TestDaemonSetController_Scale(t *testing.T) {
})
t.Run("ScaleFromZeo", func(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

err = mocks.controller.ScaleFromZero(mocks.canary)
Expand All @@ -197,7 +197,7 @@ func TestDaemonSetController_Scale(t *testing.T) {

func TestDaemonSetController_Finalize(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

err = mocks.controller.Finalize(mocks.canary)
Expand Down
5 changes: 2 additions & 3 deletions pkg/canary/daemonset_ready_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
appsv1 "k8s.io/api/apps/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -14,8 +13,8 @@ import (

func TestDaemonSetController_IsReady(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
assert.NoError(t, err, "Expected primary readiness check to fail")
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

err = mocks.controller.IsPrimaryReady(mocks.canary)
require.NoError(t, err)
Expand Down
6 changes: 3 additions & 3 deletions pkg/canary/daemonset_status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (

func TestDaemonSetController_SyncStatus(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

status := flaggerv1.CanaryStatus{
Expand All @@ -36,7 +36,7 @@ func TestDaemonSetController_SyncStatus(t *testing.T) {

func TestDaemonSetController_SetFailedChecks(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

err = mocks.controller.SetStatusFailedChecks(mocks.canary, 1)
Expand All @@ -49,7 +49,7 @@ func TestDaemonSetController_SetFailedChecks(t *testing.T) {

func TestDaemonSetController_SetState(t *testing.T) {
mocks := newDaemonSetFixture()
err := mocks.controller.Initialize(mocks.canary, true)
err := mocks.controller.Initialize(mocks.canary)
require.NoError(t, err)

err = mocks.controller.SetStatusPhase(mocks.canary, flaggerv1.CanaryPhaseProgressing)
Expand Down
4 changes: 2 additions & 2 deletions pkg/canary/deployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,14 @@ type DeploymentController struct {

// Initialize creates the primary deployment, hpa,
// scales to zero the canary deployment and returns the pod selector label and container ports
func (c *DeploymentController) Initialize(cd *flaggerv1.Canary, skipLivenessChecks bool) (err error) {
func (c *DeploymentController) Initialize(cd *flaggerv1.Canary) (err error) {
primaryName := fmt.Sprintf("%s-primary", cd.Spec.TargetRef.Name)
if err := c.createPrimaryDeployment(cd); err != nil {
return fmt.Errorf("createPrimaryDeployment failed: %w", err)
}

if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing {
if !skipLivenessChecks && !cd.SkipAnalysis() {
if !cd.SkipAnalysis() {
if err := c.IsPrimaryReady(cd); err != nil {
return fmt.Errorf("IsPrimaryReady failed: %w", err)
}
Expand Down
59 changes: 21 additions & 38 deletions pkg/canary/deployment_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ import (

func TestDeploymentController_Sync(t *testing.T) {
mocks := newDeploymentFixture()
err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.initializeCanary(t)

depPrimary, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo-primary", metav1.GetOptions{})
require.NoError(t, err)
Expand All @@ -33,11 +32,10 @@ func TestDeploymentController_Sync(t *testing.T) {

func TestDeploymentController_Promote(t *testing.T) {
mocks := newDeploymentFixture()
err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.initializeCanary(t)

dep2 := newDeploymentControllerTestV2()
_, err = mocks.kubeClient.AppsV1().Deployments("default").Update(dep2)
_, err := mocks.kubeClient.AppsV1().Deployments("default").Update(dep2)
require.NoError(t, err)

config2 := newDeploymentControllerTestConfigMapV2()
Expand Down Expand Up @@ -74,10 +72,9 @@ func TestDeploymentController_Promote(t *testing.T) {

func TestDeploymentController_ScaleToZero(t *testing.T) {
mocks := newDeploymentFixture()
err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.initializeCanary(t)

err = mocks.controller.ScaleToZero(mocks.canary)
err := mocks.controller.ScaleToZero(mocks.canary)
require.NoError(t, err)

c, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo", metav1.GetOptions{})
Expand All @@ -88,9 +85,7 @@ func TestDeploymentController_ScaleToZero(t *testing.T) {
func TestDeploymentController_NoConfigTracking(t *testing.T) {
mocks := newDeploymentFixture()
mocks.controller.configTracker = &NopTracker{}

err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.initializeCanary(t)

depPrimary, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo-primary", metav1.GetOptions{})
require.NoError(t, err)
Expand All @@ -104,8 +99,7 @@ func TestDeploymentController_NoConfigTracking(t *testing.T) {

func TestDeploymentController_HasTargetChanged(t *testing.T) {
mocks := newDeploymentFixture()
err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err)
mocks.initializeCanary(t)

// save last applied hash
canary, err := mocks.flaggerClient.FlaggerV1beta1().Canaries("default").Get("podinfo", metav1.GetOptions{})
Expand Down Expand Up @@ -190,46 +184,35 @@ func TestDeploymentController_HasTargetChanged(t *testing.T) {
}

func TestDeploymentController_Finalize(t *testing.T) {

mocks := newDeploymentFixture()

tables := []struct {
for _, tc := range []struct {
mocks deploymentControllerFixture
callInitialize bool
shouldError bool
expectedReplicas int32
canary *flaggerv1.Canary
}{
//Primary not found returns error
// primary not found returns error
{mocks, false, false, 1, mocks.canary},
//Happy path
// happy path
{mocks, true, false, 1, mocks.canary},
}

for _, table := range tables {
if table.callInitialize {
err := mocks.controller.Initialize(table.canary, true)
if err != nil {
t.Fatal(err.Error())
}
} {
if tc.callInitialize {
mocks.initializeCanary(t)
}

err := mocks.controller.Finalize(table.canary)

if table.shouldError && err == nil {
t.Error("Expected error while calling Finalize, but none was returned")
} else if !table.shouldError && err != nil {
t.Errorf("Expected no error would be returned while calling Finalize, but returned %s", err)
err := mocks.controller.Finalize(tc.canary)
if tc.shouldError {
require.Error(t, err)
} else {
require.NoError(t, err)
}

if table.expectedReplicas > 0 {
if tc.expectedReplicas > 0 {
c, err := mocks.kubeClient.AppsV1().Deployments(mocks.canary.Namespace).Get(mocks.canary.Name, metav1.GetOptions{})
if err != nil {
t.Fatal(err.Error())
}
if int32Default(c.Spec.Replicas) != table.expectedReplicas {
t.Errorf("Expected replicas %d recieved replicas %d", table.expectedReplicas, c.Spec.Replicas)
}
require.NoError(t, err)
require.Equal(t, tc.expectedReplicas, *c.Spec.Replicas)
}
}
}
26 changes: 26 additions & 0 deletions pkg/canary/deployment_fixture_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package canary

import (
"fmt"
"testing"

"github.com/stretchr/testify/require"
"go.uber.org/zap"
appsv1 "k8s.io/api/apps/v1"
hpav2 "k8s.io/api/autoscaling/v2beta1"
Expand All @@ -24,6 +28,28 @@ type deploymentControllerFixture struct {
logger *zap.SugaredLogger
}

func (d deploymentControllerFixture) initializeCanary(t *testing.T) {
err := d.controller.Initialize(d.canary)
require.Error(t, err) // not ready yet

primaryName := fmt.Sprintf("%s-primary", d.canary.Spec.TargetRef.Name)
p, err := d.controller.kubeClient.AppsV1().
Deployments(d.canary.Namespace).Get(primaryName, metav1.GetOptions{})
require.NoError(t, err)

p.Status = appsv1.DeploymentStatus{
Replicas: 1,
UpdatedReplicas: 1,
ReadyReplicas: 1,
AvailableReplicas: 1,
}

_, err = d.controller.kubeClient.AppsV1().Deployments(d.canary.Namespace).Update(p)
require.NoError(t, err)

require.NoError(t, d.controller.Initialize(d.canary))
}

func newDeploymentFixture() deploymentControllerFixture {
// init canary
canary := newDeploymentControllerTestCanary()
Expand Down
7 changes: 4 additions & 3 deletions pkg/canary/deployment_ready_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@ import (

func TestDeploymentController_IsReady(t *testing.T) {
mocks := newDeploymentFixture()
err := mocks.controller.Initialize(mocks.canary, true)
require.NoError(t, err, "Expected primary readiness check to fail")
mocks.controller.Initialize(mocks.canary)

err = mocks.controller.IsPrimaryReady(mocks.canary)
err := mocks.controller.IsPrimaryReady(mocks.canary)
require.Error(t, err)

_, err = mocks.controller.IsCanaryReady(mocks.canary)
require.NoError(t, err)
}

// TODO: more detailed tests as daemonset
Loading