utils: Use registry-scoped sources in imageContentSources #1333
Conversation
|
Hi @wking. Thanks for your PR. I'm waiting for a openshift-metal3 member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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/test-infra repository. |
openshift-ci
bot
added
the
needs-ok-to-test
wking
force-pushed
the
registry-scoped-imageContentSources
branch
2 times, most recently
from
2df9eb4 to
2a1e3c4
Compare
|
I don't know how to test this, since apparently no presubmits in this repo (if there were, we probably would have caught this issue sooner), but here's a tiny proof-of-concept to exercise some of code I'm adding: $ cat test.sh
#!/bin/bash
function pullspec_registry {
PULLSPEC="${1}"
echo "${PULLSPEC%%/*}" # strip everything after the first slash
}
TAGGED=quay.io/openshift-release-dev/ocp-v4.0-art-dev
RELEASE=registry.build01.ci.openshift.org/ci-op-1yt550tk/release
CANONICAL_REGISTRIES="$(printf "%s\n%s\n" "${RELEASE}" "${TAGGED}" | while read PULLSPEC; do pullspec_registry "${PULLSPEC}"; done | sort | uniq)"
echo imageContentSources:
echo "${CANONICAL_REGISTRIES}" | while read REGISTRY; do
printf -- "- mirrors:\n - %s:\n source: %s\n" "${LOCAL_REGISTRY_DNS_NAME}:${LOCAL_REGISTRY_PORT}/localimages/local-release-image" "${REGISTRY}"
done
$ ./test.sh
imageContentSources:
- mirrors:
- :/localimages/local-release-image:
source: quay.io
- mirrors:
- :/localimages/local-release-image:
source: registry.build01.ci.openshift.org |
The previous configuration created entries which included the full repository path within the registry. The installer creates ImageContentSourcePolicies from the install-config entries, and they end up in the cluster like [1]: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/openshift-kubernetes-1087-nightly-4.10-e2e-metal-ipi-upgrade-ovn-ipv6/1479101028317007872/artifacts/e2e-metal-ipi-upgrade-ovn-ipv6/gather-must-gather/artifacts/must-gather.tar | tar xOz quay-io-openshift-release-dev-ocp-v4-0-art-dev-sha256-6da7281285800ef1fca3ae4da6fad9f321ff757145212b74c94c4fe08fc3f055/cluster-scoped-resources/operator.openshift.io/imagecontentsourcepolicies/image-policy-0.yaml --- apiVersion: operator.openshift.io/v1alpha1 kind: ImageContentSourcePolicy metadata: creationTimestamp: "2022-01-06T15:52:21Z" generation: 1 name: image-policy-0 resourceVersion: "1567" uid: eb48e7b9-5768-42d9-b7a1-9bcec6800197 spec: repositoryDigestMirrors: - mirrors: - virthost.ostest.test.metalkube.org:5000/localimages/local-release-image source: registry.build01.ci.openshift.org/ci-op-1yt550tk/release That repository path causes trouble when the referenced images use a different repository, like: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/logs/openshift-kubernetes-1087-nightly-4.10-e2e-metal-ipi-upgrade-ovn-ipv6/1479101028317007872/artifacts/e2e-metal-ipi-upgrade-ovn-ipv6/gather-extra/artifacts/pods.json | jq -r '.items[] | select(.metadata.namespace == "openshift-etcd-operator").status.containerStatuses[].state' { "waiting": { "message": "Back-off pulling image \"registry.build01.ci.openshift.org/ci-op-1yt550tk/stable@sha256:b39240d76f42457511234d22b7d1dad72fe7917e7b55a25f520b41fd9dbbea53\"", "reason": "ImagePullBackOff" } } Compare ci-op-1yt550tk/stable with the ImageContentSourcePolicy's ci-op-1yt550tk/release. With this commit, we'll move to having entries like: source: registry.build01.ci.openshift.org From [2]: source: registry.redhat.io/openshift4 [3] ... [3]: You can configure a namespace inside a registry to use any image in that namespace. If you use a registry domain as a source, the ImageContentSourcePolicy resource is applied to all repositories from the registry. The %% business is POSIX parameter expansion [3]: ${parameter%%[word]} Remove Largest Suffix Pattern. The word shall be expanded to produce a pattern. The parameter expansion shall then result in parameter, with the largest portion of the suffix matched by the pattern deleted. stripping the first slash in the pullspec and everything after it. [1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/logs/openshift-kubernetes-1087-nightly-4.10-e2e-metal-ipi-upgrade-ovn-ipv6/1479101028317007872#1:build-log.txt%3A54 [2]: https://docs.openshift.com/container-platform/4.9/openshift_images/image-configuration.html#images-configuration-registry-mirror_image-configuration [3]: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02
wking
force-pushed
the
registry-scoped-imageContentSources
branch
from
2a1e3c4 to
4c8b1cf
Compare
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
|
Hmm, the upgrade job doesn't run here though. We should probably get that added anyway. |
|
No update jobs, but the CI that runs is green except for e2e-metal-ipi-serial-ovn-ipv6, which failed with: All of which seems unrelated to my change. |
|
/test e2e-metal-ipi-upgrade |
|
Thanks for the PR @wking.
I wonder the problem you mentioned in this PR is actually in mirroring release that will be upgraded to; https://github.com/openshift/release/blob/4e105dca61a0a7dc4f0cec55bae692b752cd4c4c/ci-operator/step-registry/baremetalds/e2e/test/baremetalds-e2e-test-commands.sh#L48. I've added ovn upgrade job in dev-scripts repo and whenever it is merged, we'll have a chance to test your PR. and one last question: this repo is used for all versions and do you think there won't be problem for <4.10 versions? |
|
/test e2e-metal-ipi-serial-ovn-ipv6 |
|
/test e2e-metal-ipi-upgrade-ovn-ipv6 |
|
@ardaguclu: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn 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/test-infra repository. |
|
/test e2e-metal-ipi-upgrade-ovn-ipv6 |
1 similar comment
|
/test e2e-metal-ipi-upgrade-ovn-ipv6 |
|
@ardaguclu: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn 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/test-infra repository. |
|
/test e2e-metal-ipi-upgrade-ovn-ipv6 |
|
@ardaguclu: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn 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/test-infra repository. |
|
/test e2e-metal-ipi-upgrade-ovn-ipv6 |
|
@ardaguclu: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn 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/test-infra repository. |
|
@andfasano: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn 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/test-infra repository. |
|
/test e2e-metal-ipi-upgrade-ovn-ipv6 |
|
Let's run /test e2e-metal-ipi-ovn-ipv6 |
|
OVN upgrade again got timeout. I think, indeed this is about the second mirroring which is for upgrade(https://github.com/openshift/release/blob/214ece4b1db7203a9f7f40a13c6fcf1e6c478800/ci-operator/step-registry/baremetalds/e2e/test/baremetalds-e2e-test-commands.sh#L49). But I could not figure out how it works in periodics and fails in presubmit jobs?, @wking do you have any suggestions about the problem? |
|
For the last failed one; All the pods using |
|
Periodics are working without any problem because they all use |
|
Digging into e2e-metal-ipi-upgrade-ovn-ipv6, here's the ImageContentSourcePolicy suggested by the initial-release mirror: Here's the config suggested for the target release mirror: $ curl -s https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift-metal3_dev-scripts/1333/pull-ci-openshift-metal3-dev-scripts-master-e2e-metal-ipi-upgrade-ovn-ipv6/1480475015475367936/artifacts/e2e-metal-ipi-upgrade-ovn-ipv6/baremetalds-e2e-test/build-log.txt | grep -A11 ImageContentSourcePolicy
To use the new mirrored repository for upgrades, use the following to create an ImageContentSourcePolicy:
apiVersion: operator.openshift.io/v1alpha1
kind: ImageContentSourcePolicy
metadata:
name: example
spec:
repositoryDigestMirrors:
- mirrors:
- virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
source: registry.build01.ci.openshift.org/ci-op-1nt9l6xk/release
- mirrors:
- virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
source: registry.build01.ci.openshift.org/ci-op-1nt9l6xk/stable
error: failed to retrieve cached signaturesAnd here's the config we actually get in the cluster (descended from the code I'm touching in this PR): $ curl https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift-metal3_dev-scripts/1333/pull-ci-openshift-metal3-dev-scripts-master-e2e-metal-ipi-upgrade-ovn-ipv6/1480475015475367936/artifacts/e2e-metal-ipi-upgrade-ovn-ipv6/gather-must-gather/artifacts/must-gather.tar >must-gather.tar.gz
$ tar tz <must-gather.tar.gz | grep imagecontentsourcepolicies/ | while read X; do echo "${X}"; tar xOz "${X}" <must-gather.tar.gz; done
registry-build01-ci-openshift-org-ci-op-1nt9l6xk-stable-initial-sha256-b50cd11185a18aeb653e8d46721332576d08645835cb7da8708374422c0eb106/cluster-scoped-resources/operator.openshift.io/imagecontentsourcepolicies/image-policy-0.yaml
---
apiVersion: operator.openshift.io/v1alpha1
kind: ImageContentSourcePolicy
metadata:
creationTimestamp: "2022-01-10T10:11:07Z"
generation: 1
name: image-policy-0
resourceVersion: "1583"
uid: 9b2b0c87-030f-47ee-9afe-134d5861264e
spec:
repositoryDigestMirrors:
- mirrors:
- virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
source: registry.build01.ci.openshift.org/ci-op-1nt9l6xk/release
registry-build01-ci-openshift-org-ci-op-1nt9l6xk-stable-initial-sha256-b50cd11185a18aeb653e8d46721332576d08645835cb7da8708374422c0eb106/cluster-scoped-resources/operator.openshift.io/imagecontentsourcepolicies/image-policy-1.yaml
---
apiVersion: operator.openshift.io/v1alpha1
kind: ImageContentSourcePolicy
metadata:
creationTimestamp: "2022-01-10T10:11:07Z"
generation: 1
name: image-policy-1
resourceVersion: "1585"
uid: 435776de-55b9-444a-8daa-4e9334101e9b
spec:
repositoryDigestMirrors:
- mirrors:
- virthost.ostest.test.metalkube.org:5000/localimages/local-release-image
source: registry.build01.ci.openshift.org/ci-op-1nt9l6xk/stable-initialI had been aiming for |
So we can debug why this doesn't seem to be working
|
@wking all tests passed and I think, we can move forward with the PR. /lgtm |
openshift-ci
bot
added
lgtm
|
/test e2e-metal-ipi-upgrade-ovn-ipv6 |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/test e2e-metal-ipi-serial-ipv4 |
cybertron
left a comment
cybertron
left a comment
There was a problem hiding this comment.
Unfortunately it doesn't look like this is going to fix the problem. Details inline.
| echo "${CANONICAL_REGISTRIES}" | while read REGISTRY; do | ||
| printf -- "- mirrors:\n - %s:\n source: %s\n" "${LOCAL_REGISTRY_DNS_NAME}:${LOCAL_REGISTRY_PORT}/localimages/local-release-image" "${REGISTRY}" | ||
| done | ||
| else |
There was a problem hiding this comment.
Shoot, I think I misread the script. At least in my local tests, what we're getting is this side of the if branch. As a result, these changes are not having any effect because we get whatever the mirror command recommends.
You can see it happening in this log snippet:
2022-01-10 11:32:45 ++(utils.sh:382): image_mirror_config(): [[ ! -s /opt/dev-scripts/registry/ostest-image_mirror-4.10.0-0.nightly-2022-01-10-101431.log ]]
2022-01-10 11:32:45 ++(utils.sh:393): image_mirror_config(): cat /opt/dev-scripts/registry/ostest-image_mirror-4.10.0-0.nightly-2022-01-10-101431.log
2022-01-10 11:32:45 ++(utils.sh:393): image_mirror_config(): sed -n '/To use the new mirrored repository to install/,/To use the new mirrored repository for upgrades/p'
2022-01-10 11:32:45 ++(utils.sh:394): image_mirror_config(): sed -e '/^$/d' -e '/To use the new mirrored repository/d'
2022-01-10 11:32:45 ++(utils.sh:396): image_mirror_config(): cat
So unless CI doesn't use the log file to get the image config (which the repeated failures suggest it is) this isn't going to fix the problem. :-/
In fact, you can see the same log output in the ci job toward the bottom of this file: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift-metal3_dev-scripts/1333/pull-ci-openshift-metal3-dev-scripts-master-e2e-metal-ipi-upgrade-ovn-ipv6/1480591379284365312/artifacts/e2e-metal-ipi-upgrade-ovn-ipv6/baremetalds-devscripts-setup/artifacts/root/dev-scripts/logs/05_create_install_config-2022-01-10-174239.log
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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/test-infra repository. I understand the commands that are listed here. |
|
Discussing this further with @wking, and maybe for upgrades we just want to do both mirroring operations up front instead of one in dev-scripts setup and one in the job steps? Ref: openshift/release#25123 That way we could put both ICS's in install-config and know they'll be correct when we run the upgrade. |
After working on this openshift/release#25130, I concluded that the same thing @cybertron. Even if mirroring for upgrade image is successfully completed, kube-apiserver-operator and cluster-etcd-operator still can not find the image. I think as you said, we should put ICS's in install-config or do you think that there is a way to add new ICS's after installation is completed? |
|
I think, if I create |
|
I did some testing for this patch in this #1334 (comment). Unfortunately, did not work and needs more changes in dev-scripts. According to the CI jobs results, this PR openshift/release#25130 solves problem. @wking if you don't mind, we can close this PR. |
|
For the cleaning up PRs I have subscribe, assigned, etc. /uncc |
4 participants
The previous configuration created entries which included the full repository path within the registry. The installer creates
ImageContentSourcePoliciesfrom the install-config entries, and they end up in the cluster like:That repository path causes trouble when the referenced images use a different repository, like:
Compare
ci-op-1yt550tk/stablewith the ImageContentSourcePolicy'sci-op-1yt550tk/release.With this commit, we'll move to having entries like:
From the OpenShift docs:
The
%%business is POSIX parameter expansion:stripping the first slash in the pullspec and everything after it.