Refactor upgrade logic to skip pre upgrade job in case of patch upgrades #137
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vsethi09
reviewed
Feb 12, 2025
| @@ -45,7 +45,7 @@ spec: | |||
| {{end}} | |||
| {{if .PreUpgrade}} | |||
| - name: pre-upgrade | |||
| args: ["io.cdap.cdap.master.upgrade.UpgradeJobMain", "{{.HostName}}", "11015"] | |||
| args: ["io.cdap.cdap.master.upgrade.UpgradeJobMain", "{{.HostName}}", "11015", "{{.SkipPreUpgrade}}"] | |||
There was a problem hiding this comment.
Why is the argument being set if the preupgrade job doesn't need to be started?
vsethi09
reviewed
Feb 12, 2025
| @@ -418,9 +422,24 @@ func compareVersion(l, r *Version) int { | |||
| return -1 | |||
| } | |||
|
|
|||
| lenL, lenR := len(l.components), len(r.components) | |||
| // Check if it only a patch upgrade | |||
There was a problem hiding this comment.
Formatting is broken. Please run make vet fmt to fix it.
vsethi09
reviewed
Feb 12, 2025
| func (s *VersionUpgradeJobSpec) SetSkipPreUpgrade(isPatchUpgrade bool) *VersionUpgradeJobSpec { | ||
| s.SkipPreUpgrade = isPatchUpgrade && s.SkipPreUpgradeFlag | ||
| return s | ||
| } |
There was a problem hiding this comment.
nit: missing new line at the end of the file.
vsethi09
reviewed
Feb 12, 2025
| @@ -607,3 +611,8 @@ func (s *VersionUpgradeJobSpec) SetPostUpgrade(isPostUpgrade bool) *VersionUpgra | |||
| s.PostUpgrade = isPostUpgrade | |||
| return s | |||
| } | |||
|
|
|||
| func (s *VersionUpgradeJobSpec) SetSkipPreUpgrade(isPatchUpgrade bool) *VersionUpgradeJobSpec { | |||
There was a problem hiding this comment.
Fix the term patch upgrade
vsethi09
reviewed
Feb 12, 2025
| @@ -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.skip.preupgrade-job" | |||
There was a problem hiding this comment.
Can be named cdap-operator.preupgrade-job.skip for consistency.
vsethi09
reviewed
Feb 12, 2025
| @@ -577,6 +577,8 @@ type VersionUpgradeJobSpec struct { | |||
| HConf string `json:"hadoopConf,omitempty"` | |||
| PreUpgrade bool `json:"preUpgrade,omitempty"` | |||
| PostUpgrade bool `json:"postUpgrade,omitempty"` | |||
| SkipPreUpgradeFlag bool `json:"skipPreUpgradeFlag,omitempty"` | |||
There was a problem hiding this comment.
Why is there a need for 2 fields for the same things?
vsethi09
reviewed
Feb 12, 2025
| @@ -55,6 +55,21 @@ var _ = Describe("Controller Suite", func() { | |||
| Expect(compareVersion(high, low)).To(Equal(1)) | |||
| } | |||
| }) | |||
| It("Compare image versions in patch upgrade", func() { | |||
| imagePairs := []Pair{ | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactor upgrade logic to skip pre upgrade job in case of patch upgrades
Description
This change prevents pre upgrade job from stopping pipelines in case of patch upgrades.
Code change
upgrade-job.yaml, spec.go, version_update.go, constants.goUnit Tests
version_update_test.go, pre_upgrade_job.json