OTA-1531: [3/x] cvo: read version from release metadata on startup #1188
Conversation
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@petr-muller: This pull request references OTA-1531 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
approved
|
@petr-muller: This pull request references OTA-1531 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@petr-muller: This pull request references OTA-1531 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/test unit lint images |
|
/test e2e-agnostic-operator |
petr-muller
changed the title
|
/test ci/prow/e2e-agnostic-operator |
|
@petr-muller: The specified target(s) for The following commands are available to trigger optional jobs: Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/test e2e-agnostic-operator |
|
/test e2e-agnostic-operator e2e-agnostic-ovn-update-into-change |
|
/test e2e-agnostic-ovn-upgrade-into-change e2e-agnostic-ovn-upgrade-into-change-techpreview |
CVO is typically executed in a Pod using the release payload image, which means its filesystem contains (among other content) the release metadata which the CVO can use to determine its OCP version, which is helpful to establish the feature gate enablement data to drive gated CVO behaviors. It is useful to establish the feature gate enablement checker early in the CVO execution because it broadens the part of CVO code that can contain gated behavior (until the checker is stablished it is not possible to determine whether a gate is enabled or disabled) Loading the full payload is still quite heavy operation and can only be done later in the execution (after the CVO acquires leader lease). The early metadata peek should read and utilize as few data as is necessary to establish the gate checker.
Address Hongkai's review point from openshift#1185 (comment)
petr-muller
force-pushed
the
ota-1531-03-load-metadata-earlyy
branch
from
e2edeb6 to
a6cf93c
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
This move only addresses a review comment from an earlier PR: #1185 (comment)
There was a problem hiding this comment.
It is interesting that github chose to interpret the change as stanza from 311 (the release part) is moved down to 318, instead of stanza 318 (the arch part) is moved up.
Github did not read our conversation carefully enough. 😄
|
@petr-muller: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/label no-qe |
|
This is what I wanted to see ❤️ |
|
/cc |
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
There was a problem hiding this comment.
It is interesting that github chose to interpret the change as stanza from 311 (the release part) is moved down to 318, instead of stanza 318 (the arch part) is moved up.
Github did not read our conversation carefully enough. 😄
| cvoOcpVersion = releaseMetadata.Version | ||
| klog.Infof("Determined OCP version for this CVO: %q", cvoOcpVersion) |
There was a problem hiding this comment.
NIT:
| cvoOcpVersion = releaseMetadata.Version | |
| klog.Infof("Determined OCP version for this CVO: %q", cvoOcpVersion) | |
| klog.Infof("Determined OCP version for this CVO: %q", releaseMetadata.Version) |
(Definitely just a matter of choice) I would use "0.0.1-snapshot" in the other 2 places as well (and we could remove cvoOcpVersion). For example,
klog.Warningf("Failed to read release metadata to determine OCP version for this CVO (will use placeholder '0.0.1-snapshot'): %v", err)I feel it would be easier a bit to read the code that way and without having to think if cvoOcpVersion is used later.
There was a problem hiding this comment.
I was kind of expecting releaseMetadata.Version is used somewhere because it's value has been determined earlier. Will I see it in another PR soon?
Not the releaseMetadata.Version directly but cvoOcpVersion will be used later, it will either have the value read from the release metadata or the placeholder one. It makes sense to log the value that will be eventually used, and also we should keep the placeholder value in a single place, not inline it in logging strings.
Check out the followups:
The "determine OCP version" code will be made into a method:
cluster-version-operator/pkg/start/start.go
Lines 217 to 243 in 280e27f
And the determined version will be used to determine enable CVO feature gates:
cluster-version-operator/pkg/start/start.go
Lines 253 to 254 in 280e27f
| case err != nil: | ||
| klog.Warningf("Failed to read release metadata to determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err) | ||
| case releaseMetadata.Version == "": | ||
| klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err) |
There was a problem hiding this comment.
Would err have to be nil when it reaches this case?
| klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q): %v", cvoOcpVersion, err) | |
| klog.Warningf("Version missing from release metadata, cannot determine OCP version for this CVO (will use placeholder version %q)", cvoOcpVersion) |
There was a problem hiding this comment.
Good point, will address
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu, petr-muller The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 6fe4a71
into
openshift:main
|
[ART PR BUILD NOTIFIER] Distgit: cluster-version-operator |
Address an earlier review comment: openshift#1188 (comment)
Address an earlier review comment: openshift#1188 (comment)
Builds on #1185
CVO is typically executed in a Pod using the release payload image, which means its filesystem contains (among other content) the release metadata which the CVO can use to determine its OCP version, which is helpful to establish the feature gate enablement data to drive gated CVO behaviors.
It is useful to establish the feature gate enablement checker early in the CVO execution because it broadens the part of CVO code that can contain gated behavior (until the checker is established it is not possible to determine whether a gate is enabled or disabled)
Loading the full payload is still quite heavy operation and can only be done later in the execution (after the CVO acquires leader lease). The early metadata peek should read and utilize as few data as is necessary to establish the gate checker.