new controller for managing upgrade of operator #53
Conversation
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
Signed-off-by: Leela Venkaiah G <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leelavg The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold for merges due to a dependency of updated api from ocs-operator |
Signed-off-by: Leela Venkaiah G <[email protected]>
leelavg
force-pushed
the
new-controller
branch
from
5de31b6 to
ad418a4
Compare
| currentVersion := instance.Status.Operator.CurrentVersion | ||
| operatorVersion, _ := semver.Make(s.OperatorVersion) | ||
| if currentVersion == "" { | ||
| // TODO(lgangava): should we set Major.Minor.Patch or Major.Minor only |
There was a problem hiding this comment.
this is visible to user, desiredVersion will only be M.m and I'm setting currentVersion also as M.m and this TODO is to think of any other reasons that it should be M.m.p which I couldn't find.
| } | ||
| testAheadVersion = "4.15" | ||
| testCurrentVersion = "4.14" | ||
| testLaggingVersion = "4.13" |
There was a problem hiding this comment.
I'd also test ascending/descending major versions
There was a problem hiding this comment.
although it isn't urgent, having major version checks wherever possible also makes sense, will try to push in next revision.
| result, err := r.reconcilePhases() | ||
| if err != nil { | ||
| // TODO (lgangava): we are being very restrictive as of now, not upgrading if we hit any error | ||
| // differentiate between reconciler errors and not upgradeable errors |
There was a problem hiding this comment.
Don't those coincide? Meaning surely there are some reconciling errors that imply non-upgradability
There was a problem hiding this comment.
there are some reconciling errors that imply non-upgradability
- currently we are implying all reconcile errors as non-upgradability
- do you mean to say, not all reconcile errors shouldn't be viewed as not upgradable?
| return nil | ||
| } | ||
|
|
||
| desiredVersions := make([]string, 0, len(storageClientList.Items)) |
There was a problem hiding this comment.
Better to declare as a nil slice with var
There was a problem hiding this comment.
nope, if we know the capacity why to declare without it?
| for idx := range storageClientList.Items { | ||
| desiredVersion := storageClientList.Items[idx].Status.Operator.DesiredVersion | ||
| // usually these versions will not have patch version, so we are adding an arbitrary num for semver comparison | ||
| desiredVersion += dummyPatchVersion |
There was a problem hiding this comment.
Doesn't not having a patch version translate to _._.0 and not some arbitrary number?
There was a problem hiding this comment.
nope, 0 is as good as any other random number and we just need a random number.
the background is:
- provider sends desiredversion and it'll have only Major.minor
- client queries it's version and sets currentVersion, also Major.minor
- but semver is strict about version be Major.minor.patch and for comparison we just need a constant random number added to it.
why not 0 then?
- if this code changes and it's logged, M.m.0 will actually be a release version but the release to be M.m.99 is slim to none so I chose 99
| platformVersion, _ := semver.Make(r.PlatformVersion) | ||
| if desiredVersion.Major > platformVersion.Major || desiredVersion.Minor > platformVersion.Minor { | ||
| r.log.Info("backing off from updating subscription as operator version can become ahead of platform version") | ||
| } else if desiredVersion.Major == channelVersion.Major && desiredVersion.Minor == channelVersion.Minor+1 { |
There was a problem hiding this comment.
Wondering is this the only possible scenario where the minor versions differ?
There was a problem hiding this comment.
No, this is the only scenario we are interested and wanted to take an action on. There's a corner case which I don't want to include which will complicate it.
4.20 -> released
4.21 -> released but whole minor version is faulty and redacted
4.22 -> released, now if desired is 22 but channel is at 20 this branch will not be taken
however, with an entire minor version being faulty we might have more serious issues that this.
do you mean any other scenarios?
| } | ||
|
|
||
| func getOldestVersion(versions []string) (semver.Version, error) { | ||
| if len(versions) < 1 { |
There was a problem hiding this comment.
Missing a preceding nil check (which could have been more straightforward without using make upon init)
There was a problem hiding this comment.
nope, len is an all rounder, it's always better to use that for checking emptiness on items
> var first []string
: 0xc00011c0e0
> var second []string = nil
: 0xc0004040f0
> third := []string{}
: []
> fourth := make([]string, 0, 5)
: []
> fifth := make([]string, 5)
: [ ]
> fmt.Println(len(first)==0)
true
: 5
> fmt.Println(len(second)==0)
true
: 5
> fmt.Println(len(third)==0)
true
: 5
> fmt.Println(len(fourth)==0)
true
: 5
> fmt.Println(len(fifth)==0)
false
: 6| func getVersionFromChannel(name string) (semver.Version, error) { | ||
| version, _ := strings.CutPrefix(name, subscriptionChannelNamePrefix) | ||
| // usually channel names will not have patch version and semver will fail w/o patch version | ||
| version += dummyPatchVersion |
There was a problem hiding this comment.
answered #53 (comment)
nb-ohad
changed the title
|
Depends on red-hat-storage/ocs-operator#2329 |
|
not be revived |
As part of heartbeat response provider sends desired version that it wants client operator to match and this PR introduces a new (upgrade) controller to take action based on desired version.