OTA-1531: [2/x] cvo: refactor LoadUpdate
#1185
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@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. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
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. |
1 similar comment
|
@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 images unit lint |
petr-muller
force-pushed
the
ocpbugs-30080-02-payload-metadata-load-refactor
branch
from
db47626 to
4d67ad3
Compare
|
/test images unit lint |
petr-muller
force-pushed
the
ocpbugs-30080-02-payload-metadata-load-refactor
branch
from
4d67ad3 to
c2a8f5d
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest-required |
|
/cc |
|
/hold I'll need to change this a bit more |
openshift-ci
bot
added
the
do-not-merge/hold
Extract loading tasks from `loadUpdatePayloadMetadata` one layer up to `LoadUpdate`, which makes more sense as a logical sequence of "loading the update". This also allows simplifying `loadUpdatePayloadMetadata` signature because the inputs for loading tasks are partially disjoint from the ones needed for loading the metadata.
The architecture is either read from release metadata when present (and in that case it must be a string and must be multi) or determined from runtime in all other cases. Refactor `loadReleaseFromMetadata` so that it only really reads the metadata, and move the "when metadata does not have architecture, read from runtime" logic to `loadPayloadMetadata` above (the `loadReleaseFromMetadata` caller). This will allow to reuse the release metadata loading code without involving more logic than necessary. This also simplifies `loadReleaseFromMetadata` signature, improving readability. Also turn some nested conditionals into guard clauses for readability.
…data` I believe this is a better name in the context of the full loading code. Release metadata is a subset of the payload metadata, and the `LoadUpdate` -> `loadPayloadMetadata` -> `loadReleaseMetadata` chain makes the method names more cohesive.
petr-muller
force-pushed
the
ocpbugs-30080-02-payload-metadata-load-refactor
branch
from
425b59e to
4fe21f6
Compare
|
/hold cancel
|
openshift-ci
bot
removed
the
do-not-merge/hold
|
@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. |
| klog.V(2).Infof("Loading updatepayload from %q", dir) | ||
| if err := ValidateDirectory(dir); err != nil { | ||
| return nil, err | ||
| } | ||
| var ( | ||
| cvoDir = filepath.Join(dir, CVOManifestDir) | ||
| releaseDir = filepath.Join(dir, ReleaseManifestDir) | ||
| ) |
There was a problem hiding this comment.
Just moved here from loadUpdatePayloadMetadata
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| tasks := loadPayloadTasks(releaseDir, cvoDir, releaseImage, profile) |
There was a problem hiding this comment.
Just moved here from loadUpdatePayloadMetadata
| arch := string(release.Architecture) | ||
| if arch == "" { | ||
| arch = runtime.GOARCH | ||
| klog.V(2).Infof("Architecture from %s (%s) retrieved from runtime: %q", cincinnatiJSONFile, release.Version, arch) |
There was a problem hiding this comment.
Moved here from loadReleaseFromMetadata
There was a problem hiding this comment.
nit: I would move this part right after line 307.
Then I would not need to check if arch is used between 307 and 318.
There was a problem hiding this comment.
that's a good point, I'll move it right after the error check in a followup PR #1188
| } | ||
|
|
||
| release.Architecture = configv1.ClusterVersionArchitectureMulti | ||
| klog.V(2).Infof("Architecture from %s (%s) is multi: %q", cincinnatiJSONFile, release.Version, arch) |
There was a problem hiding this comment.
The "metadata does not have architecture key, lets read runtime" part moved one layer above to loadPayloadMetadata
Otherwise the logic is identical, just was refactored into guard clause sequence.
|
/test e2e-hypershift |
|
There is only one failure which is usually unrelated and normally I would override, but let's be sure on PRs that touch CVO in a potentially sensitive way. /test e2e-agnostic-ovn-upgrade-into-change |
|
/label no-qe We agreed on testing approach with QE, we'll want to pre-merge the last PR in the series and will rely on CI for the partial PRs |
|
/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 |
|
@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. |
| }, nil | ||
| } | ||
|
|
||
| type payloadTasks struct { |
There was a problem hiding this comment.
super nit: Without moving this struct (feels like the code did not change) it would be easier to comparing the change.
There was a problem hiding this comment.
Yeah I just wanted this struct closer to loadPayloadTasks that creates this data
| arch := string(release.Architecture) | ||
| if arch == "" { | ||
| arch = runtime.GOARCH | ||
| klog.V(2).Infof("Architecture from %s (%s) retrieved from runtime: %q", cincinnatiJSONFile, release.Version, arch) |
There was a problem hiding this comment.
nit: I would move this part right after line 307.
Then I would not need to check if arch is used between 307 and 318.
|
[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 5d71fb4
into
openshift:main
|
[ART PR BUILD NOTIFIER] Distgit: cluster-version-operator |
Address Hongkai's review point from openshift#1185 (comment)
Builds on: #1184
Extract loading tasks from
loadUpdatePayloadMetadataone layer up toLoadUpdate, which makes more sense as a logical sequence of "loading the update". This also allows simplifyingloadUpdatePayloadMetadatasignature because the inputs for loading tasks are partially disjoint from the ones needed for loading the metadata.The architecture is either read from release metadata when present (and in that case it must be a string and must be multi) or determined from runtime in all other cases. Refactor
loadReleaseFromMetadataso that it only really reads the metadata, and move the "when metadata does not have architecture, read from runtime" logic toloadPayloadMetadataabove (theloadReleaseFromMetadatacaller). This will allow to reuse the release metadata loading code without involving more logic than necessary.Rename loadReleaseFromMetadata->
loadReleaseMetadata. I believe this is a better name in the context of the full loading code. Release metadata is a subset of the payload metadata, and theLoadUpdate->loadPayloadMetadata->loadReleaseMetadatachain makes the method names more cohesive.The reason for this refactor is that for OTA-1531 I need to isolate the code that loads payload metadata from the code that loads the full payload. This will allow to probe the metadata for release version near the start of CVO executing, so that the version can be used to set up the desired feature gated behavior to enable or disable.