Skip to content

Commit 7676918

Browse files
mathetakemathetake
authored
Merge pull request #530 from weaveworks/remove-skipLivenessChecks-param
pkg/{canary,controller}: remove unused skipLivenessChecks
2 parents 9c46be1 + 65d4b28 commit 7676918

18 files changed

+275
-183
lines changed

‎pkg/canary/config_tracker_test.go

+4-7
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@ func TestConfigTracker_ConfigMaps(t *testing.T) {
1515
configMap := newDeploymentControllerTestConfigMap()
1616
configMapProjected := newDeploymentControllerTestConfigProjected()
1717

18-
err := mocks.controller.Initialize(mocks.canary, true)
19-
require.NoError(t, err)
18+
mocks.initializeCanary(t)
2019

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

56-
err := mocks.controller.Initialize(mocks.canary, true)
55+
err := mocks.controller.Initialize(mocks.canary)
5756
require.NoError(t, err)
5857

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

96-
err := mocks.controller.Initialize(mocks.canary, true)
97-
require.NoError(t, err)
95+
mocks.initializeCanary(t)
9896

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

134-
err := mocks.controller.Initialize(mocks.canary, true)
135-
require.NoError(t, err)
132+
mocks.controller.Initialize(mocks.canary)
136133

137134
daePrimary, err := mocks.kubeClient.AppsV1().DaemonSets("default").Get("podinfo-primary", metav1.GetOptions{})
138135
if assert.NoError(t, err) {

‎pkg/canary/controller.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ type Controller interface {
1313
SetStatusWeight(canary *flaggerv1.Canary, val int) error
1414
SetStatusIterations(canary *flaggerv1.Canary, val int) error
1515
SetStatusPhase(canary *flaggerv1.Canary, phase flaggerv1.CanaryPhase) error
16-
Initialize(canary *flaggerv1.Canary, skipLivenessChecks bool) error
16+
Initialize(canary *flaggerv1.Canary) error
1717
Promote(canary *flaggerv1.Canary) error
1818
HasTargetChanged(canary *flaggerv1.Canary) (bool, error)
1919
HaveDependenciesChanged(canary *flaggerv1.Canary) (bool, error)

‎pkg/canary/daemonset_controller.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -74,14 +74,14 @@ func (c *DaemonSetController) ScaleFromZero(cd *flaggerv1.Canary) error {
7474

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

8383
if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing {
84-
if !skipLivenessChecks && !cd.SkipAnalysis() {
84+
if !cd.SkipAnalysis() {
8585
if err := c.IsPrimaryReady(cd); err != nil {
8686
return fmt.Errorf("IsPrimaryReady failed: %w", err)
8787
}

‎pkg/canary/daemonset_controller_test.go

+7-7
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import (
1515

1616
func TestDaemonSetController_Sync(t *testing.T) {
1717
mocks := newDaemonSetFixture()
18-
err := mocks.controller.Initialize(mocks.canary, true)
18+
err := mocks.controller.Initialize(mocks.canary)
1919
require.NoError(t, err)
2020

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

3030
func TestDaemonSetController_Promote(t *testing.T) {
3131
mocks := newDaemonSetFixture()
32-
err := mocks.controller.Initialize(mocks.canary, true)
32+
err := mocks.controller.Initialize(mocks.canary)
3333
require.NoError(t, err)
3434

3535
dae2 := newDaemonSetControllerTestPodInfoV2()
@@ -60,7 +60,7 @@ func TestDaemonSetController_NoConfigTracking(t *testing.T) {
6060
mocks := newDaemonSetFixture()
6161
mocks.controller.configTracker = &NopTracker{}
6262

63-
err := mocks.controller.Initialize(mocks.canary, true)
63+
err := mocks.controller.Initialize(mocks.canary)
6464
require.NoError(t, err)
6565

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

7676
func TestDaemonSetController_HasTargetChanged(t *testing.T) {
7777
mocks := newDaemonSetFixture()
78-
err := mocks.controller.Initialize(mocks.canary, true)
78+
err := mocks.controller.Initialize(mocks.canary)
7979
require.NoError(t, err)
8080

8181
// save last applied hash
@@ -163,7 +163,7 @@ func TestDaemonSetController_HasTargetChanged(t *testing.T) {
163163
func TestDaemonSetController_Scale(t *testing.T) {
164164
t.Run("ScaleToZero", func(t *testing.T) {
165165
mocks := newDaemonSetFixture()
166-
err := mocks.controller.Initialize(mocks.canary, true)
166+
err := mocks.controller.Initialize(mocks.canary)
167167
require.NoError(t, err)
168168

169169
err = mocks.controller.ScaleToZero(mocks.canary)
@@ -179,7 +179,7 @@ func TestDaemonSetController_Scale(t *testing.T) {
179179
})
180180
t.Run("ScaleFromZeo", func(t *testing.T) {
181181
mocks := newDaemonSetFixture()
182-
err := mocks.controller.Initialize(mocks.canary, true)
182+
err := mocks.controller.Initialize(mocks.canary)
183183
require.NoError(t, err)
184184

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

198198
func TestDaemonSetController_Finalize(t *testing.T) {
199199
mocks := newDaemonSetFixture()
200-
err := mocks.controller.Initialize(mocks.canary, true)
200+
err := mocks.controller.Initialize(mocks.canary)
201201
require.NoError(t, err)
202202

203203
err = mocks.controller.Finalize(mocks.canary)

‎pkg/canary/daemonset_ready_test.go

+2-3
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"strings"
55
"testing"
66

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

1514
func TestDaemonSetController_IsReady(t *testing.T) {
1615
mocks := newDaemonSetFixture()
17-
err := mocks.controller.Initialize(mocks.canary, true)
18-
assert.NoError(t, err, "Expected primary readiness check to fail")
16+
err := mocks.controller.Initialize(mocks.canary)
17+
require.NoError(t, err)
1918

2019
err = mocks.controller.IsPrimaryReady(mocks.canary)
2120
require.NoError(t, err)

‎pkg/canary/daemonset_status_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import (
1212

1313
func TestDaemonSetController_SyncStatus(t *testing.T) {
1414
mocks := newDaemonSetFixture()
15-
err := mocks.controller.Initialize(mocks.canary, true)
15+
err := mocks.controller.Initialize(mocks.canary)
1616
require.NoError(t, err)
1717

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

3737
func TestDaemonSetController_SetFailedChecks(t *testing.T) {
3838
mocks := newDaemonSetFixture()
39-
err := mocks.controller.Initialize(mocks.canary, true)
39+
err := mocks.controller.Initialize(mocks.canary)
4040
require.NoError(t, err)
4141

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

5050
func TestDaemonSetController_SetState(t *testing.T) {
5151
mocks := newDaemonSetFixture()
52-
err := mocks.controller.Initialize(mocks.canary, true)
52+
err := mocks.controller.Initialize(mocks.canary)
5353
require.NoError(t, err)
5454

5555
err = mocks.controller.SetStatusPhase(mocks.canary, flaggerv1.CanaryPhaseProgressing)

‎pkg/canary/deployment_controller.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,14 @@ type DeploymentController struct {
2828

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

3737
if cd.Status.Phase == "" || cd.Status.Phase == flaggerv1.CanaryPhaseInitializing {
38-
if !skipLivenessChecks && !cd.SkipAnalysis() {
38+
if !cd.SkipAnalysis() {
3939
if err := c.IsPrimaryReady(cd); err != nil {
4040
return fmt.Errorf("IsPrimaryReady failed: %w", err)
4141
}

‎pkg/canary/deployment_controller_test.go

+21-38
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,7 @@ import (
1515

1616
func TestDeploymentController_Sync(t *testing.T) {
1717
mocks := newDeploymentFixture()
18-
err := mocks.controller.Initialize(mocks.canary, true)
19-
require.NoError(t, err)
18+
mocks.initializeCanary(t)
2019

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

3433
func TestDeploymentController_Promote(t *testing.T) {
3534
mocks := newDeploymentFixture()
36-
err := mocks.controller.Initialize(mocks.canary, true)
37-
require.NoError(t, err)
35+
mocks.initializeCanary(t)
3836

3937
dep2 := newDeploymentControllerTestV2()
40-
_, err = mocks.kubeClient.AppsV1().Deployments("default").Update(dep2)
38+
_, err := mocks.kubeClient.AppsV1().Deployments("default").Update(dep2)
4139
require.NoError(t, err)
4240

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

7573
func TestDeploymentController_ScaleToZero(t *testing.T) {
7674
mocks := newDeploymentFixture()
77-
err := mocks.controller.Initialize(mocks.canary, true)
78-
require.NoError(t, err)
75+
mocks.initializeCanary(t)
7976

80-
err = mocks.controller.ScaleToZero(mocks.canary)
77+
err := mocks.controller.ScaleToZero(mocks.canary)
8178
require.NoError(t, err)
8279

8380
c, err := mocks.kubeClient.AppsV1().Deployments("default").Get("podinfo", metav1.GetOptions{})
@@ -88,9 +85,7 @@ func TestDeploymentController_ScaleToZero(t *testing.T) {
8885
func TestDeploymentController_NoConfigTracking(t *testing.T) {
8986
mocks := newDeploymentFixture()
9087
mocks.controller.configTracker = &NopTracker{}
91-
92-
err := mocks.controller.Initialize(mocks.canary, true)
93-
require.NoError(t, err)
88+
mocks.initializeCanary(t)
9489

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

105100
func TestDeploymentController_HasTargetChanged(t *testing.T) {
106101
mocks := newDeploymentFixture()
107-
err := mocks.controller.Initialize(mocks.canary, true)
108-
require.NoError(t, err)
102+
mocks.initializeCanary(t)
109103

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

192186
func TestDeploymentController_Finalize(t *testing.T) {
193-
194187
mocks := newDeploymentFixture()
195188

196-
tables := []struct {
189+
for _, tc := range []struct {
197190
mocks deploymentControllerFixture
198191
callInitialize bool
199192
shouldError bool
200193
expectedReplicas int32
201194
canary *flaggerv1.Canary
202195
}{
203-
//Primary not found returns error
196+
// primary not found returns error
204197
{mocks, false, false, 1, mocks.canary},
205-
//Happy path
198+
// happy path
206199
{mocks, true, false, 1, mocks.canary},
207-
}
208-
209-
for _, table := range tables {
210-
if table.callInitialize {
211-
err := mocks.controller.Initialize(table.canary, true)
212-
if err != nil {
213-
t.Fatal(err.Error())
214-
}
200+
} {
201+
if tc.callInitialize {
202+
mocks.initializeCanary(t)
215203
}
216204

217-
err := mocks.controller.Finalize(table.canary)
218-
219-
if table.shouldError && err == nil {
220-
t.Error("Expected error while calling Finalize, but none was returned")
221-
} else if !table.shouldError && err != nil {
222-
t.Errorf("Expected no error would be returned while calling Finalize, but returned %s", err)
205+
err := mocks.controller.Finalize(tc.canary)
206+
if tc.shouldError {
207+
require.Error(t, err)
208+
} else {
209+
require.NoError(t, err)
223210
}
224211

225-
if table.expectedReplicas > 0 {
212+
if tc.expectedReplicas > 0 {
226213
c, err := mocks.kubeClient.AppsV1().Deployments(mocks.canary.Namespace).Get(mocks.canary.Name, metav1.GetOptions{})
227-
if err != nil {
228-
t.Fatal(err.Error())
229-
}
230-
if int32Default(c.Spec.Replicas) != table.expectedReplicas {
231-
t.Errorf("Expected replicas %d recieved replicas %d", table.expectedReplicas, c.Spec.Replicas)
232-
}
214+
require.NoError(t, err)
215+
require.Equal(t, tc.expectedReplicas, *c.Spec.Replicas)
233216
}
234217
}
235218
}

‎pkg/canary/deployment_fixture_test.go

+26
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
package canary
22

33
import (
4+
"fmt"
5+
"testing"
6+
7+
"github.com/stretchr/testify/require"
48
"go.uber.org/zap"
59
appsv1 "k8s.io/api/apps/v1"
610
hpav2 "k8s.io/api/autoscaling/v2beta1"
@@ -24,6 +28,28 @@ type deploymentControllerFixture struct {
2428
logger *zap.SugaredLogger
2529
}
2630

31+
func (d deploymentControllerFixture) initializeCanary(t *testing.T) {
32+
err := d.controller.Initialize(d.canary)
33+
require.Error(t, err) // not ready yet
34+
35+
primaryName := fmt.Sprintf("%s-primary", d.canary.Spec.TargetRef.Name)
36+
p, err := d.controller.kubeClient.AppsV1().
37+
Deployments(d.canary.Namespace).Get(primaryName, metav1.GetOptions{})
38+
require.NoError(t, err)
39+
40+
p.Status = appsv1.DeploymentStatus{
41+
Replicas: 1,
42+
UpdatedReplicas: 1,
43+
ReadyReplicas: 1,
44+
AvailableReplicas: 1,
45+
}
46+
47+
_, err = d.controller.kubeClient.AppsV1().Deployments(d.canary.Namespace).Update(p)
48+
require.NoError(t, err)
49+
50+
require.NoError(t, d.controller.Initialize(d.canary))
51+
}
52+
2753
func newDeploymentFixture() deploymentControllerFixture {
2854
// init canary
2955
canary := newDeploymentControllerTestCanary()

‎pkg/canary/deployment_ready_test.go

+4-3
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,13 @@ import (
88

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

14-
err = mocks.controller.IsPrimaryReady(mocks.canary)
13+
err := mocks.controller.IsPrimaryReady(mocks.canary)
1514
require.Error(t, err)
1615

1716
_, err = mocks.controller.IsCanaryReady(mocks.canary)
1817
require.NoError(t, err)
1918
}
19+
20+
// TODO: more detailed tests as daemonset

0 commit comments

Comments
 (0)