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

Author: juliogreff

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)

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)

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