ceph: compare ceph commit ID for upgrade check #8060
Conversation
There was a problem hiding this comment.
I see this fixes the version check issue, but I think there is a bigger issue as well... If the cephVersion.image field changes for any reason, we should make sure all the ceph daemons get the new image, even if the ceph version is unchanged. Otherwise, we could have daemons running different images until the operator is restarted or there is some other update to the cluster CR that causes a full reconcile. This is a corner case and could be a separate PR, but I'd still propose we address it.
pkg/operator/ceph/version/version.go
Outdated
| @@ -128,7 +132,13 @@ func ExtractCephVersion(src string) (*CephVersion, error) { | |||
| } | |||
| } | |||
|
|
|||
| return &CephVersion{major, minor, extra, build}, nil | |||
| // get build suffix if running on a build release | |||
| mmm := buildSuffixPattern.FindStringSubmatch(src) | |||
There was a problem hiding this comment.
nit: how about a more descriptive name than mmm? It's interesting to follow the pattern of the rest of the method, but I'd vote for descriptive names instead of a pattern like that. :)
There was a problem hiding this comment.
updated. Using <>Match as variable names
Added another commit that compares image ID. Now we check for both Build Suffix and Image ID. I think we can remove Build suffix check and only keep image ID check as that would suffice for our use case. |
sp98
changed the title
| fakeRunningVersions = []byte(` | ||
| { | ||
| "overall": { | ||
| "ceph version 14.2.11-139.0.hotfix.bz1959254.el8cp (5c0dc966af809fd1d429ec7bac48962a746af243) nautilus (stable)": 2 |
There was a problem hiding this comment.
So this 139.0.hotfix.bz1959254 is the Build? and el8cp the BuildSuffix?
There was a problem hiding this comment.
139 is the build and anything after that is the build suffix 0.hotfix.bz1959254.el8cp in this case. The implementation in master branch only checks till 14.2.11-139 . Reference
pkg/operator/ceph/version/version.go
Outdated
| buildSuffix = buildSuffixMatch[5] | ||
| } | ||
|
|
||
| imageMatch := imagePattern.FindStringSubmatch(src) |
There was a problem hiding this comment.
trying to fetch the commit ID of the image here. I believe this should address the commit
pkg/operator/ceph/version/version.go
Outdated
| return true | ||
| } | ||
| } | ||
| if a.Image != b.Image { |
There was a problem hiding this comment.
If the image changed we don't know if it's superior, right? We only know it's different. For superiority I think we don't need to know at a depth past a.Build so we could revert the changes to this method.
sp98
changed the title
Currently we only check the Major, Minor, Extra and Build number in the ceph version to determine if all daemons should be upgraded or not. This PR also checks the commit hash for upgrading all the daemons. This will be useful in cases where build number has not changed due to hotfix. For example: existing ceph version - ceph version 14.2.11-139.el8cp (b8e1f91c99491fb2e5ede748a1c0738ed158d0f5) nautilus (stable) new ceph version with hotfix - ceph version 14.2.11-139.0.hotfix.bz1959254.el8cp (5c0dc966af809fd1d429ec7bac48962a746af243) nautilus (stable) Signed-off-by: Santosh Pillai <sapillai@redhat.com>
|
@sp98 please open an issue to follow with what we discussed yesterday, thanks. |
ceph: compare ceph commit ID for upgrade check (backport #8060)
.Compare Image ID while checking for upgrades. This will be useful in the case where ceph version is same but image ID is different and also when build number has not changed for a hotfix build.
For example:
existing ceph version -
ceph version 14.2.11-139.el8cp (b8e1f91c99491fb2e5ede748a1c0738ed158d0f5) nautilus (stable)new ceph version with hotfix -
ceph version 14.2.11-139.0.hotfix.bz1959254.el8cp (5c0dc966af809fd1d429ec7bac48962a746af243) nautilus (stable)Signed-off-by: Santosh Pillai sapillai@redhat.com
Description of your changes:
Which issue is resolved by this Pull Request:
Resolves #7964
Commit ID Regex test: https://play.golang.org/p/S5wBQrueC_m
Checklist:
make codegen) has been run to update object specifications, if necessary.