Updated openshift-tests images to utilize mapped images from external binary #30319
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
/cc @bertinatto PTAL |
|
@dinhxuanvu: GitHub didn't allow me to request PR reviews from the following users: PTAL. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
bertinatto
left a comment
bertinatto
left a comment
There was a problem hiding this comment.
Together with openshift/kubernetes#2447, is this change getting us both original and mapped images?
Just in case you are not familiar with it, we can do that with this command: |
|
@bertinatto: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/02629160-9e28-11f0-963d-726455cb7bc7-0 |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
dinhxuanvu
force-pushed
the
ot-images
branch
3 times, most recently
from
c325563 to
a88fe10
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dinhxuanvu 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 |
|
Job Failure Risk Analysis for sha: a88fe10
|
dinhxuanvu
force-pushed
the
ot-images
branch
2 times, most recently
from
8a7c5af to
0913c46
Compare
|
/payload-job-with-prs periodic-ci-openshift-release-master-ci-4.21-e2e-aws-ovn openshift/kubernetes#2447 |
|
@dinhxuanvu: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/d9236830-a631-11f0-909f-cfa810781685-0 |
|
/retest-required |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
Job Failure Risk Analysis for sha: 0913c46
|
dinhxuanvu
force-pushed
the
ot-images
branch
2 times, most recently
from
34d23eb to
52c391e
Compare
|
Job Failure Risk Analysis for sha: 52c391e
|
|
Job Failure Risk Analysis for sha: 6364e89
|
… binary Currently, openshift-tests images cmd uses vendored GetMappedImageConfigs() function to map upstream images to mirrored ones. After this change, images cmd will parsed mapped images directly from outputs from the k8s-tests-ext binary. This prevents unexpected issues from imcompatible upstreams changes with vendored funcs. Signed-off-by: Vu Dinh <[email protected]>
go mod vendor Signed-off-by: Vu Dinh <[email protected]>
|
@bertinatto Ready for another round of review. PTAL. Thanks. |
|
@dinhxuanvu: The following tests 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. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Job Failure Risk Analysis for sha: 4d6ce8e
|
|
/payload-job-with-prs periodic-ci-openshift-release-master-ci-4.21-e2e-aws-ovn openshift/kubernetes#2447 |
|
@bertinatto: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/29a7cf10-c952-11f0-8f49-cbdc43e1a601-0 |
bertinatto
left a comment
bertinatto
left a comment
There was a problem hiding this comment.
It seems like this isn't mapping to the right repository, for example:
openshift-tests images --to-repository quay.io/bertinatto/openshift-tests --upstream
(...)
registry.k8s.io/sig-storage/hostpathplugin:v1.15.0 quay.io/openshift/community-e2e-images:e2e-47-registry-k8s-io-sig-storage-hostpathplugin-v1-15-0-YS6opQN6AdImbOb6
| } | ||
| mu.Lock() | ||
| allImages = append(allImages, imageConfig) | ||
| allImages = append(allImages, image...) |
There was a problem hiding this comment.
I think this change (ImageSet to extension.Image) is making this PR a lot harder to review. We now have confusing var names, like this in images_command.go:
externalImageSets := []extension.Image{}
initialImageSets := []extensions.ImageSet{
If you have a chance, and if it makes sense, I'd love to see this kind of change in separate, logical commits.
There was a problem hiding this comment.
I can make changes to funcs in origin/pkg/test/extensions/binary.go but it means some funcs will have a different signatures moving forward. It may simplify the logic in images_command.go a bit but there will still be some changes.
Honestly, I find this whole operation combination (openshift-tests images + oc image mirror) is ugly because we modify from image paths from various image sources (upstream, internal and etc) which have their own sets of filters and modification on their own. Also, there are too many option combinations from two commands (are these mirrored already or not + upsteam or not) which lead to complex logic. Plus, we implemented this mirrored ImageSet only for one binary so it is a special case that is added on top of all of these options.
Anyway, let me make some changes on binary.go to see if those changes there will result in a simpler login on images_command.go side.
3 participants
Currently, openshift-tests images cmd uses vendored GetMappedImageConfigs() function to map upstream images to mirrored ones. After this change, images cmd will parsed mapped images directly from outputs from the k8s-tests-ext binary. This prevents unexpected issues from imcompatible upstreams changes with vendored funcs.