ReleaseController application picks the tip of the release chain as contender

pkg/chart/repo/catalog.go

@@ -23,7 +23,7 @@ func DefaultRemoteFetcher(url string) ([]byte, error) {
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK {
-		return nil, fmt.Errorf("unsuccessful response code: %s (%d)", resp.Status, resp.StatusCode)
+		return nil, fmt.Errorf("bad response code: %s (%d)", resp.Status, resp.StatusCode)
 	}
 
 	return ioutil.ReadAll(resp.Body)

pkg/chart/repo/repo.go

@@ -224,7 +224,11 @@ func (r *Repo) FetchRemote(cv *repo.ChartVersion) (*chart.Chart, error) {
 	url := chartURL.String()
 	data, err := r.fetcher(url)
 	if err != nil {
-		return nil, err
+		chart, convErr := newChart(cv)
+		if convErr != nil {
+			return nil, shippererrors.NewChartRepoInternalError(convErr)
+		}
+		return nil, shippererrors.NewChartFetchFailureError(chart, err)
 	}
 
 	chart, err := loadChartData(data)
@@ -307,3 +311,14 @@ func chart2file(cv *repo.ChartVersion) string {
 
 	return fmt.Sprintf("%s-%s.tgz", name, version)
 }
+
+func newChart(cv *repo.ChartVersion) (*shipper.Chart, error) {
+	if len(cv.URLs) < 1 {
+		return nil, fmt.Errorf("chart version is missing URLs")
+	}
+	return &shipper.Chart{
+		Name:    cv.GetName(),
+		Version: cv.GetVersion(),
+		RepoURL: cv.URLs[0],
+	}, nil
+}

pkg/chart/repo/repo_test.go

@@ -345,7 +345,7 @@ func TestFetch(t *testing.T) {
 			"0.0.2",
 			"",
 			"",
-			fmt.Errorf("failed to read file \"non-existing-0.0.2.tgz\": open testdata/non-existing-0.0.2.tgz: no such file or directory"),
+			fmt.Errorf("failed to fetch chart [name: \"non-existing\", version: \"0.0.2\", repo: \"https://charts.example.com/non-existing-0.0.2.tgz\"]: failed to read file \"non-existing-0.0.2.tgz\": open testdata/non-existing-0.0.2.tgz: no such file or directory"),
 		},
 	}
 

pkg/controller/release/application.go

@@ -5,7 +5,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/tools/cache"
@@ -14,6 +13,7 @@ import (
 	shipper "github.com/bookingcom/shipper/pkg/apis/shipper/v1alpha1"
 	shippercontroller "github.com/bookingcom/shipper/pkg/controller"
 	shippererrors "github.com/bookingcom/shipper/pkg/errors"
+	apputil "github.com/bookingcom/shipper/pkg/util/application"
 	releaseutil "github.com/bookingcom/shipper/pkg/util/release"
 )
 
@@ -180,64 +180,23 @@ func (c *Controller) buildExecutor(incumbentRelease, contenderRelease *shipper.R
 	}, nil
 }
 
-func (c *Controller) sortedReleasesForApp(namespace, name string) ([]*shipper.Release, error) {
-	selector := labels.Set{
-		shipper.AppLabel: name,
-	}.AsSelector()
-
-	releases, err := c.releaseLister.Releases(namespace).List(selector)
-	if err != nil {
-		return nil, shippererrors.NewKubeclientListError(
-			shipper.SchemeGroupVersion.WithKind("Release"),
-			namespace, selector, err)
-	}
-
-	sorted, err := shippercontroller.SortReleasesByGeneration(releases)
-	if err != nil {
-		return nil, err
-	}
-
-	return sorted, nil
-}
-
 func (c *Controller) getWorkingReleasePair(app *shipper.Application) (*shipper.Release, *shipper.Release, error) {
-	appReleases, err := c.sortedReleasesForApp(app.GetNamespace(), app.GetName())
+
+	appReleases, err := c.releaseLister.Releases(app.Namespace).ReleasesForApplication(app.Name)
 	if err != nil {
 		return nil, nil, err
 	}
+	// Required by subsequent calls to GetContender and GetIncumbent.
+	appReleases = releaseutil.SortByGenerationDescending(appReleases)
 
-	if len(appReleases) == 0 {
-		err := fmt.Errorf(
-			"zero release records in app %q: will not execute strategy",
-			shippercontroller.MetaKey(app))
-		return nil, nil, shippererrors.NewRecoverableError(err)
-	}
-
-	// Walk backwards until we find a scheduled release. There may be pending
-	// releases ahead of the actual contender, that's not we're looking for.
-	var contender *shipper.Release
-	for i := len(appReleases) - 1; i >= 0; i-- {
-		if releaseutil.ReleaseScheduled(appReleases[i]) {
-			contender = appReleases[i]
-			break
-		}
-	}
-
-	if contender == nil {
-		err := fmt.Errorf("couldn't find a contender for Application %q",
-			shippercontroller.MetaKey(app))
+	contender, err := apputil.GetContender(app.Name, appReleases)
+	if err != nil {
 		return nil, nil, shippererrors.NewRecoverableError(err)
 	}
 
-	var incumbent *shipper.Release
-	// Walk backwards until we find an installed release that isn't the head of
-	// history. For releases A -> B -> C, if B was never finished this allows C to
-	// ignore it and let it get deleted so the transition is A->C.
-	for i := len(appReleases) - 1; i >= 0; i-- {
-		if releaseutil.ReleaseComplete(appReleases[i]) && contender != appReleases[i] {
-			incumbent = appReleases[i]
-			break
-		}
+	incumbent, err := apputil.GetIncumbent(app.Name, appReleases)
+	if err != nil && !shippererrors.IsIncumbentNotFoundError(err) {
+		return nil, nil, err
 	}
 
 	// It is OK if incumbent is nil. It just means this is our first rollout.

pkg/controller/release/release_controller.go

@@ -2,6 +2,7 @@ package release
 
 import (
 	"fmt"
+	"reflect"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -19,6 +20,7 @@ import (
 	shipperclient "github.com/bookingcom/shipper/pkg/client/clientset/versioned"
 	shipperinformers "github.com/bookingcom/shipper/pkg/client/informers/externalversions"
 	shipperlisters "github.com/bookingcom/shipper/pkg/client/listers/shipper/v1alpha1"
+	"github.com/bookingcom/shipper/pkg/controller"
 	shippererrors "github.com/bookingcom/shipper/pkg/errors"
 	releaseutil "github.com/bookingcom/shipper/pkg/util/release"
 )
@@ -304,14 +306,13 @@ func (c *Controller) syncOneReleaseHandler(key string) error {
 			WithShipperKind("Release")
 	}
 
-	rel = rel.DeepCopy()
-
 	if releaseutil.HasEmptyEnvironment(rel) {
 		return nil
 	}
 
-	klog.V(4).Infof("Start processing Release %q", key)
+	var initialRel *shipper.Release = rel.DeepCopy()
 
+	klog.V(4).Infof("Start processing Release %q", key)
 	scheduler := NewScheduler(
 		c.clientset,
 		c.clusterLister,
@@ -323,46 +324,51 @@ func (c *Controller) syncOneReleaseHandler(key string) error {
 		c.recorder,
 	)
 
-	// This is a 2-round handler: the 1st round schedules the release on a
-	// set of clusters, and the 2nd round creates associated objects and
-	// finalizes release scheduling process. This approach comes for a
-	// reason: we are eliminating a replication lag problem. While running
-	// the system in production, we observed some cases where a few
-	// sequentual updates on the same object experienced apiserver
-	// rejections due to the passed object outdate state. This happened due
-	// to the synchronisation/replication lag in etcd. This approach helps
-	// to eliminate this as a notification will be delivered once all
-	// parties are in sync.
 	if !releaseHasClusters(rel) {
-		_, err = scheduler.ChooseClusters(rel, false)
-	} else {
-		_, err = scheduler.ScheduleRelease(rel)
+		shouldForce := false
+		rel, err = scheduler.ChooseClusters(rel.DeepCopy(), shouldForce)
+		if err != nil {
+			return err
+		}
+		c.recorder.Eventf(
+			rel,
+			corev1.EventTypeNormal,
+			"ClustersSelected",
+			"Set clusters for %q to %v",
+			controller.MetaKey(rel),
+			rel.Annotations[shipper.ReleaseClustersAnnotation],
+		)
 	}
 
-	if err != nil {
+	var scheduleErr error
+	rel, scheduleErr = scheduler.ScheduleRelease(rel.DeepCopy())
+	if scheduleErr != nil {
 		if shippererrors.ShouldBroadcast(err) {
 			c.recorder.Eventf(
-				rel,
+				initialRel,
 				corev1.EventTypeWarning,
 				"FailedReleaseScheduling",
-				err.Error(),
+				scheduleErr.Error(),
 			)
 		}
-
-		reason := reasonForReleaseCondition(err)
+		reason := reasonForReleaseCondition(scheduleErr)
 		condition := releaseutil.NewReleaseCondition(
 			shipper.ReleaseConditionTypeScheduled,
 			corev1.ConditionFalse,
 			reason,
-			err.Error(),
+			scheduleErr.Error(),
 		)
-		releaseutil.SetReleaseCondition(&rel.Status, *condition)
-
-		if _, updateErr := c.clientset.ShipperV1alpha1().Releases(namespace).Update(rel); updateErr != nil {
-			return shippererrors.NewKubeclientUpdateError(rel, updateErr)
+		releaseutil.SetReleaseCondition(&initialRel.Status, *condition)
+		if _, err := c.clientset.ShipperV1alpha1().Releases(namespace).Update(initialRel); err != nil {
+			return shippererrors.NewKubeclientUpdateError(initialRel, err)
 		}
+		return scheduleErr
+	}
 
-		return err
+	if !reflect.DeepEqual(initialRel, rel) {
+		if _, err := c.clientset.ShipperV1alpha1().Releases(namespace).Update(rel); err != nil {
+			return shippererrors.NewKubeclientUpdateError(rel, err)
+		}
 	}
 
 	klog.V(4).Infof("Release %q has been successfully scheduled", key)
@@ -557,6 +563,8 @@ func reasonForReleaseCondition(err error) string {
 		return "WrongChartDeployments"
 	case shippererrors.RolloutBlockError:
 		return "RolloutBlock"
+	case shippererrors.ChartRepoInternalError:
+		return "ChartRepoInternal"
 	}
 
 	if shippererrors.IsKubeclientError(err) {