Refactor upgrade logic to skip pre upgrade job in case of patch revisions

controllers/constants.go

@@ -68,6 +68,7 @@ const (
 	confTwillSecurityWorkerSecretDiskPath = "twill.security.worker.secret.disk.path"
 	confJMXServerPort                     = "jmx.metrics.collector.server.port"
 	confSecretMountDefaultMode            = "secret.mount.default.mode"
+	confSkipPreUpgradeFlag                = "cdap-operator.preupgrade-job.skip"
 
 	// default values
 	defaultImage                  = "gcr.io/cdapio/cdap:latest"

controllers/version_update.go

@@ -32,10 +32,21 @@ func init() {
 /////////////////////////////////////////////////////////////
 
 func handleVersionUpdate(master *v1alpha1.CDAPMaster, labels map[string]string, observed []reconciler.Object) ([]reconciler.Object, error) {
+	curVersion, err := getCurrentImageVersion(master)
+	if err != nil {
+		return nil, err
+	}
+	newVersion, err := getNewImageVersion(master)
+	if err != nil {
+		return nil, err
+	}
+	versionComparison := compareVersion(curVersion, newVersion)
+	isPatchRevision := versionComparison == -2
+
 	// Let the current update complete if there is any
 	if isConditionTrue(master, updateStatus.Inprogress) {
 		log.Printf("Version update ingress. Continue... ")
-		return upgradeForBackend(master, labels, observed)
+		return upgradeForBackend(master, labels, observed, isPatchRevision)
 	}
 
 	if objs, versionUpdated, err := updateForUserInterface(master); err != nil {
@@ -45,21 +56,13 @@ func handleVersionUpdate(master *v1alpha1.CDAPMaster, labels map[string]string,
 	}
 
 	// Update backend service image version
-	curVersion, err := getCurrentImageVersion(master)
-	if err != nil {
-		return nil, err
-	}
-	newVersion, err := getNewImageVersion(master)
-	if err != nil {
-		return nil, err
-	}
 	if len(curVersion.rawString) == 0 {
 		setImageToUse(master)
 		return []reconciler.Object{}, nil
 	}
 
-	switch compareVersion(curVersion, newVersion) {
-	case -1:
+	switch versionComparison {
+	case -2, -1:
 		// Upgrade case
 
 		// Don't retry upgrade if it failed.
@@ -73,7 +76,7 @@ func handleVersionUpdate(master *v1alpha1.CDAPMaster, labels map[string]string,
 		setCondition(master, updateStatus.Inprogress)
 		master.Status.UpgradeStartTimeMillis = getCurrentTimeMs()
 		log.Printf("Version update: start upgrading %s -> %s ", curVersion.rawString, newVersion.rawString)
-		return upgradeForBackend(master, labels, observed)
+		return upgradeForBackend(master, labels, observed, isPatchRevision)
 	case 0:
 		// Reset all condition so that failed upgraded/downgrade can be retried later if needed.
 		// This is needed when last upgrade failed and user has reset the version in spec.
@@ -120,7 +123,7 @@ func downgradeForBackend(master *v1alpha1.CDAPMaster) ([]reconciler.Object, erro
 	return []reconciler.Object{}, nil
 }
 
-func upgradeForBackend(master *v1alpha1.CDAPMaster, labels map[string]string, observed []reconciler.Object) ([]reconciler.Object, error) {
+func upgradeForBackend(master *v1alpha1.CDAPMaster, labels map[string]string, observed []reconciler.Object, isPatchRevision bool) ([]reconciler.Object, error) {
 	// Find either pre- or post- upgrade job
 	findJob := func(jobName string) *batchv1.Job {
 		var job *batchv1.Job = nil
@@ -154,6 +157,19 @@ func upgradeForBackend(master *v1alpha1.CDAPMaster, labels map[string]string, ob
 		return jobObj
 	}
 
+	SkipPreUpgradeFlag := !(master.Spec.Config[confSkipPreUpgradeFlag] == "false")
+
+	// Skip pre-upgrade and post-upgrade jobs for patch revision
+	if isPatchRevision && SkipPreUpgradeFlag {
+		log.Printf("Version update: patch revision detected, skipping pre-upgrade and post-upgrade jobs.")
+		setImageToUse(master)
+		setCondition(master, updateStatus.VersionUpdated)
+		setCondition(master, updateStatus.UpgradeSucceeded)
+		clearCondition(master, updateStatus.Inprogress)
+		log.Printf("Version update: patch revision completed.")
+		return []reconciler.Object{}, nil
+	}
+
 	// First, run pre-upgrade job
 	//
 	// Note that pre-upgrade job doesn't have an "activeDeadlineSeconds" set it on, so it will
@@ -406,6 +422,7 @@ func parseImageString(imageString string) (*Version, error) {
 }
 
 // compare two parsed versions
+// -2: left < right, patch revision
 // -1: left < right
 // 0: left = right
 // 1: left > right
@@ -418,9 +435,24 @@ func compareVersion(l, r *Version) int {
 		return -1
 	}
 
+	lenL, lenR := len(l.components), len(r.components)
+	// Check if it only a patch revision
+	if lenL == lenR && lenL > 0 && l.components[lenL-1] < r.components[lenL-1] {
+		allEqual := true
+		for i := 0; i < lenL-1; i++ {
+			if l.components[i] != r.components[i] {
+				allEqual = false
+				break
+			}
+		}
+		if allEqual {
+			return -2
+		}
+	}
+
 	i := 0
 	j := 0
-	for i < len(l.components) && j < len(r.components) {
+	for i < lenL && j < lenR {
 		if l.components[i] > r.components[j] {
 			return 1
 		} else if l.components[i] < r.components[j] {
@@ -429,13 +461,13 @@ func compareVersion(l, r *Version) int {
 		i++
 		j++
 	}
-	for i < len(l.components) {
+	for i < lenL {
 		if l.components[i] > 0 {
 			return 1
 		}
 		i++
 	}
-	for j < len(r.components) {
+	for j < lenR {
 		if r.components[j] > 0 {
 			return 1
 		}
@@ -504,12 +536,12 @@ func getCurrentTimeMs() int64 {
 
 // The returned name is just the suffix of actual k8s object name, as we prepend it with const string + CR name
 func getPreUpgradeJobName(startTimeMs int64) string {
-	return fmt.Sprintf("pre-upgrade-job-%d", startTimeMs / 1000)
+	return fmt.Sprintf("pre-upgrade-job-%d", startTimeMs/1000)
 }
 
 // The returned name is just the suffix of actual k8s object name, as we prepend it with const string + CR name
 func getPostUpgradeJobName(startTimeMs int64) string {
-	return fmt.Sprintf("post-upgrade-job-%d", startTimeMs / 1000)
+	return fmt.Sprintf("post-upgrade-job-%d", startTimeMs/1000)
 }
 
 // Return pre-upgrade job spec

controllers/version_update_test.go

@@ -42,7 +42,7 @@ var _ = Describe("Controller Suite", func() {
 		It("Compare image versions", func() {
 			imagePairs := []Pair{
 				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:latest"},
-				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:6.0.0.1"},
+				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:6.0.1.0"},
 				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:6.1.0"},
 				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:7"},
 			}
@@ -55,6 +55,21 @@ var _ = Describe("Controller Suite", func() {
 				Expect(compareVersion(high, low)).To(Equal(1))
 			}
 		})
+		It("Compare image versions in patch revision", func() {
+			imagePairs := []Pair{
+				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:6.0.0.1"},
+				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:6.0.0.3"},
+				Pair{"gcr.io/cdapio/cdap:6.0.0.0", "gcr.io/cdapio/cdap:6.0.0.9"},
+			}
+			for _, imagePair := range imagePairs {
+				low, err := parseImageString(imagePair.first.(string))
+				Expect(err).To(BeNil())
+				high, err := parseImageString(imagePair.second.(string))
+				Expect(err).To(BeNil())
+				Expect(compareVersion(low, high)).To(Equal(-2))
+				Expect(compareVersion(high, low)).To(Equal(1))
+			}
+		})
 		It("Compare same image versions", func() {
 			imagePairs := []Pair{
 				Pair{"gcr.io/cdapio/cdap:latest", "gcr.io/cdapio/cdap:latest"},