From c11dff7923b4e9fabff53d0c388020b2653a0345 Mon Sep 17 00:00:00 2001 From: juliogreff Date: Tue, 1 Oct 2019 10:38:20 +0200 Subject: [PATCH] release controller: move cluster choosing to scheduling This may or may not make conceptual sense (it does to me, but I could not find out why choosing clusters and scheduling a release was two separate steps in the first place, although after #166[1] I'm fairly convinced this was just a technical artifact), but it sure is convenient: we move all the error handling during the scheduling step to a single chunk of code. This fixes an issue where errors in ChooseClusters were not reflected in any condition, making the Release object not change during the sync, and therefore not triggering any events, being essentially invisible to users. As a bonus, I restored the actual testing part of this in the unit tests. We were previously just checking that ChooseClusters didn't trigger any updates, without actually checking if it was doing the right thing (choosing clusters). [1] https://github.com/bookingcom/shipper/pull/166/files#diff-caffe52421149f1f8d77a0e7c749867dR327-R341 --- pkg/controller/release/release_controller.go | 18 ----------- pkg/controller/release/scheduler.go | 18 +++++++++-- pkg/controller/release/scheduler_test.go | 32 ++++++++++---------- 3 files changed, 31 insertions(+), 37 deletions(-) diff --git a/pkg/controller/release/release_controller.go b/pkg/controller/release/release_controller.go index 2d8409cc3..4dd31b467 100644 --- a/pkg/controller/release/release_controller.go +++ b/pkg/controller/release/release_controller.go @@ -386,24 +386,6 @@ func (c *Controller) scheduleRelease(rel *shipper.Release) (*shipper.Release, er ) releaseutil.SetReleaseCondition(&rel.Status, *condition) - if !releaseHasClusters(rel) { - shouldForce := false - rel, err = scheduler.ChooseClusters(rel, shouldForce) - - if err != nil { - return initialRel, err - } - - c.recorder.Eventf( - rel, - corev1.EventTypeNormal, - "ClustersSelected", - "Set clusters for %q to %v", - controller.MetaKey(rel), - rel.Annotations[shipper.ReleaseClustersAnnotation], - ) - } - rel, err = scheduler.ScheduleRelease(rel.DeepCopy()) if err != nil { reason := reasonForReleaseCondition(err) diff --git a/pkg/controller/release/scheduler.go b/pkg/controller/release/scheduler.go index 217d423f1..0d022dee5 100644 --- a/pkg/controller/release/scheduler.go +++ b/pkg/controller/release/scheduler.go @@ -63,9 +63,9 @@ func NewScheduler( } } -func (s *Scheduler) ChooseClusters(rel *shipper.Release, force bool) (*shipper.Release, error) { +func (s *Scheduler) ChooseClusters(rel *shipper.Release) (*shipper.Release, error) { metaKey := controller.MetaKey(rel) - if !force && releaseHasClusters(rel) { + if releaseHasClusters(rel) { return nil, shippererrors.NewUnrecoverableError(fmt.Errorf("release %q has already been assigned to clusters", metaKey)) } klog.Infof("Choosing clusters for release %q", metaKey) @@ -93,7 +93,19 @@ func (s *Scheduler) ScheduleRelease(rel *shipper.Release) (*shipper.Release, err defer klog.Infof("Finished processing %q", metaKey) if !releaseHasClusters(rel) { - return nil, shippererrors.NewUnrecoverableError(fmt.Errorf("release %q clusters have not been chosen yet", metaKey)) + rel, err := s.ChooseClusters(rel) + if err != nil { + return nil, err + } + + s.recorder.Eventf( + rel, + corev1.EventTypeNormal, + "ClustersSelected", + "Set clusters for %q to %v", + controller.MetaKey(rel), + rel.Annotations[shipper.ReleaseClustersAnnotation], + ) } replicaCount, err := s.fetchChartAndExtractReplicaCount(rel) diff --git a/pkg/controller/release/scheduler_test.go b/pkg/controller/release/scheduler_test.go index 24641977f..4d2e6915c 100644 --- a/pkg/controller/release/scheduler_test.go +++ b/pkg/controller/release/scheduler_test.go @@ -352,21 +352,18 @@ func TestSchedule(t *testing.T) { expected := release.DeepCopy() expected.Annotations[shipper.ReleaseClustersAnnotation] = clusterA.GetName() + "," + clusterB.GetName() - relWithConditions := expected.DeepCopy() - - // release should be marked as Scheduled - condition := releaseutil.NewReleaseCondition(shipper.ReleaseConditionTypeScheduled, corev1.ConditionTrue, "", "") - releaseutil.SetReleaseCondition(&relWithConditions.Status, *condition) - - expectedActions := []kubetesting.Action{} + c, _ := newScheduler(fixtures) - c, clientset := newScheduler(fixtures) - if _, err := c.ChooseClusters(release.DeepCopy(), false); err != nil { + got, err := c.ChooseClusters(release.DeepCopy()) + if err != nil { t.Fatal(err) } - filteredActions := filterActions(clientset.Actions(), []string{"update"}, []string{"releases"}) - shippertesting.CheckActions(expectedActions, filteredActions, t) + if expected.Annotations[shipper.ReleaseClustersAnnotation] != got.Annotations[shipper.ReleaseClustersAnnotation] { + t.Errorf("expected release to have clusters %q, got %q", + expected.Annotations[shipper.ReleaseClustersAnnotation], + got.Annotations[shipper.ReleaseClustersAnnotation]) + } } // TestScheduleSkipsUnschedulable tests the first part of the cluster @@ -391,15 +388,18 @@ func TestScheduleSkipsUnschedulable(t *testing.T) { condition := releaseutil.NewReleaseCondition(shipper.ReleaseConditionTypeScheduled, corev1.ConditionTrue, "", "") releaseutil.SetReleaseCondition(&relWithConditions.Status, *condition) - expectedActions := []kubetesting.Action{} + c, _ := newScheduler(fixtures) - c, clientset := newScheduler(fixtures) - if _, err := c.ChooseClusters(release.DeepCopy(), false); err != nil { + got, err := c.ChooseClusters(release.DeepCopy()) + if err != nil { t.Fatal(err) } - filteredActions := filterActions(clientset.Actions(), []string{"update"}, []string{"releases"}) - shippertesting.CheckActions(expectedActions, filteredActions, t) + if expected.Annotations[shipper.ReleaseClustersAnnotation] != got.Annotations[shipper.ReleaseClustersAnnotation] { + t.Errorf("expected release to have clusters %q, got %q", + expected.Annotations[shipper.ReleaseClustersAnnotation], + got.Annotations[shipper.ReleaseClustersAnnotation]) + } } // TestCreateAssociatedObjects checks whether the associated object set is being