-
Notifications
You must be signed in to change notification settings - Fork 1.9k
WIP - Disconnected round two, with file system #17896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP - Disconnected round two, with file system #17896
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to not use bastion because I think with disconnected there is no bastion, so terminology is too vague. I tried a couple, 'mirror host' sounds reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the earlier [id="installing-preparing-bastion"] -> [id="installing-preparing-mirror"]? Or maybe inclide [id="..."] for both forms for backwards-compat?
38a208b to
06facb6
Compare
|
@kalexand-rh have you incorporated these text changes already in another PR? Or do I need to resurrect this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If no TLS on this image server, podman in bootkube.sh will complain with that, isn't it?
# podman pull --help --tls-verify Require HTTPS and verify certificates when contacting registries (default true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
podman would, but you don't need podman for any of these steps. Digest pulls are safe because you can't lie about what the content is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In https://github.com/openshift/installer/blob/master/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L13, podman will pull and run image against a secure registry, which is expecting a https registry, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, do you know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That step is install-time only, and even then we should start setting whatever Podman needs to allow HTTP. As Clayton points out, the installer's by-digest reference is safe over HTTP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run this command, get such error.
$ oc adm release mirror -a ~/mirror_pullsecret_config.json --from=registry.svc.ci.openshift.org/ocp/release:4.2.0-0.nightly-2020-01-13-060909 --to=file://test
<--snip-->
uccess
Update image: test:4.2.0-0.nightly-2020-01-13-060909
Mirror prefix: file://test
error: Error creating mirror usage instructions: Unable to parse image reference 'file://test': invalid reference format
Do I miss anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The command has changed. it's --to-dir and --from-dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While testing these instructions I found a bug in 4.3 onwards that will get fixed and back ported that prevented them from working properly.
|
@kalexand-rh bump, does this need closing? |
|
This and #16894 should be merged together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: What is "it" in "push it"? Probably "content" and not "the host". Is this introducing "how to create a mirror host" (that's how I'd read it before)? Is this introducing "how to push content to an already-existing mirror host"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per https://bugzilla.redhat.com/show_bug.cgi?id=1806779#c11, we should update this command via 'oc image mirror', which is being suggested on the output when mirror release image to disk file.
This is part of decoupling our docs from the assumption that a bastion will exist. For installation, we just care that there is a registry with the mirrored content that the cluster can pull from. Having that registry on a bastion makes creating the mirrored images easier, but as our existing docs hint, there are other ways you can get the mirrored images in to your mirror repository. This commit borrows its motivation and some of its wording from Clayton's [1]. Also push the port information into <mirror_registry>, because we want to make it clear that we don't require a port for these entries. The previous content was not consistent about whether placeholder were referenced with or without their wrapping <>. In my new text, I've included the <> to make it more clear that they are part of the placeholder, and not literal characters that should remain after placeholder substitution. [1]: openshift#17896 (comment)
This is part of decoupling our docs from the assumption that a bastion will exist. For installation, we just care that there is a registry with the mirrored content that the cluster can pull from. Having that registry on a bastion makes creating the mirrored images easier, but as our existing docs hint, there are other ways you can get the mirrored images in to your mirror repository. This commit borrows its motivation and some of its wording from Clayton's [1]. Also push the port information into <mirror_registry>, because we want to make it clear that we don't require a port for these entries. The previous content was not consistent about whether placeholder were referenced with or without their wrapping <>. In my new text, I've included the <> to make it more clear that they are part of the placeholder, and not literal characters that should remain after placeholder substitution. [1]: openshift#17896 (comment)
This is part of decoupling our docs from the assumption that a bastion will exist. For installation, we just care that there is a registry with the mirrored content that the cluster can pull from. Having that registry on a bastion makes creating the mirrored images easier, but as our existing docs hint, there are other ways you can get the mirrored images in to your mirror repository. This commit borrows its motivation and some of its wording from Clayton's [1]. Also push the port information into <mirror_registry>, because we want to make it clear that we don't require a port for these entries. The previous content was not consistent about whether placeholder were referenced with or without their wrapping <>. In my new text, I've included the <> to make it more clear that they are part of the placeholder, and not literal characters that should remain after placeholder substitution. [1]: openshift#17896 (comment)
This is part of decoupling our docs from the assumption that a bastion will exist. For installation, we just care that there is a registry with the mirrored content that the cluster can pull from. Having that registry on a bastion makes creating the mirrored images easier, but as our existing docs hint, there are other ways you can get the mirrored images in to your mirror repository. This commit borrows its motivation and some of its wording from Clayton's [1]. Also push the port information into <mirror_registry>, because we want to make it clear that we don't require a port for these entries. The previous content was not consistent about whether placeholder were referenced with or without their wrapping <>. In my new text, I've included the <> to make it more clear that they are part of the placeholder, and not literal characters that should remain after placeholder substitution. [1]: openshift#17896 (comment)
This is part of decoupling our docs from the assumption that a bastion will exist. For installation, we just care that there is a registry with the mirrored content that the cluster can pull from. Having that registry on a bastion makes creating the mirrored images easier, but as our existing docs hint, there are other ways you can get the mirrored images in to your mirror repository. This commit borrows its motivation and some of its wording from Clayton's [1]. Also push the port information into <mirror_registry>, because we want to make it clear that we don't require a port for these entries. The previous content was not consistent about whether placeholder were referenced with or without their wrapping <>. In my new text, I've included the <> to make it more clear that they are part of the placeholder, and not literal characters that should remain after placeholder substitution. [1]: openshift#17896 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton, do you have the process to add this secret to the machines or a contact who would?
06facb6 to
bcaaf79
Compare
|
@jianlinliu, will you PTAL? I've rebased and edited the PR into modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here it is 'file://openshift/release:<product_version>*', missing the ending ' * '.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, here is a huge gap to get this process working, I never testes this before.
- Here is my trial steps:
`
$ oc adm release mirror <product_version> --to file://test
<--snip-->
Success
Update image: test:4.3.0-0.nightly-2020-06-07-045523
Mirror prefix: file://test
To upload local images to a registry, run:
oc image mirror --from-dir=/home/installer1/mnt/2020-06-08-07-57-52/jialiu432_2020-06-08-07-57-52/data file://test:4.3.0-0.nightly-2020-06-07-045523* REGISTRY/REPOSITORY
$ oc image serve
error: --dir must point to a directory: stat : no such file or directory
$ oc image serve --dir=./data --listen=:5001
I0628 06:09:47.819085 21665 serve.go:119] Serving at :5001 ...
$ podman pull upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523 --tls-verify=false --log-level debug
DEBU[0000] Initializing boltdb state at /var/lib/containers/storage/libpod/bolt_state.db
DEBU[0000] Using graph driver overlay
DEBU[0000] Using graph root /var/lib/containers/storage
DEBU[0000] Using run root /var/run/containers/storage
DEBU[0000] Using static dir /var/lib/containers/storage/libpod
DEBU[0000] Using tmp dir /var/run/libpod
DEBU[0000] Using volume path /var/lib/containers/storage/volumes
DEBU[0000] Set libpod namespace to ""
DEBU[0000] [graphdriver] trying provided driver "overlay"
DEBU[0000] cached value indicated that overlay is supported
DEBU[0000] cached value indicated that metacopy is not being used
DEBU[0000] cached value indicated that native-diff is usable
DEBU[0000] backingFs=xfs, projectQuotaSupported=false, useNativeDiff=true, usingMetacopy=false
DEBU[0000] Initializing event backend journald
INFO[0000] Found CNI network crio-bridge (type=bridge) at /etc/cni/net.d/100-crio-bridge.conf
INFO[0000] Found CNI network 200-loopback.conf (type=loopback) at /etc/cni/net.d/200-loopback.conf
INFO[0000] Found CNI network podman (type=bridge) at /etc/cni/net.d/87-podman-bridge.conflist
DEBU[0000] parsed reference into "[overlay@/var/lib/containers/storage+/var/run/containers/storage]upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523"
Trying to pull upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523...DEBU[0000] reference rewritten from 'upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523' to 'upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523'
DEBU[0000] Trying to pull "upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523"
DEBU[0000] Using registries.d directory /etc/containers/registries.d for sigstore configuration
DEBU[0000] Using "default-docker" configuration
DEBU[0000] No signature storage configuration found for upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523
DEBU[0000] Looking for TLS certificates and private keys in /etc/docker/certs.d/upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001
DEBU[0000] GET https://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/v2/
DEBU[0000] Ping https://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/v2/ err Get https://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/v2/: http: server gave HTTP response to HTTPS client (&url.Error{Op:"Get", URL:"https://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/v2/", Err:(*errors.errorString)(0xc420462090)})
DEBU[0000] GET http://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/v2/
DEBU[0000] Ping http://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/v2/ status 200
DEBU[0000] GET http://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/v2/file/manifests/4.3.0-0.nightly-2020-06-07-045523
ERRO[0000] Error pulling image ref //upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523: Error initializing source docker://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523: Error reading manifest 4.3.0-0.nightly-2020-06-07-045523 in upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file: error parsing HTTP 404 response body: invalid character 'p' after top-level value: "404 page not found\n"
Failed
ERRO[0000] error pulling image "upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523": unable to pull upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523: unable to pull image: Error initializing source docker://upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file:4.3.0-0.nightly-2020-06-07-045523: Error reading manifest 4.3.0-0.nightly-2020-06-07-045523 in upshift-nointernet.mirror-registry.qe.devcluster.openshift.com:5001/file: error parsing HTTP 404 response body: invalid character 'p' after top-level value: "404 page not found\n"
`
Even I applied some workaround, still no chance. What is next steps to consume this mirrored release image?
- In cluster install process. podman always use https by default to pull image, such as:
[1]: https://github.com/openshift/installer/blob/master/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L13
[2]: https://github.com/openshift/installer/blob/master/data/data/bootstrap/files/usr/local/bin/release-image-download.sh.template#L15
In this section, these steps start a non-insecure registry, that definitely does not work with openshift v4 (all image-pull places reqire HTTPS). Another potential issue this even does not need authentication, how to help user set pull secret in install-config to allow customer to follow the next intsall section to get a successfull install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking, what's your opinion on including the non-production mirror steps? Are you ok with making an existing registry a requirement, or is there a way to make this process work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine punting as much as we can so we can land the non-contentious bits :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From user perspective, What is "ICS/ICSP"? How to consume it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imageContentSources and ImageContentSourcePolicy. We should probably spell them out and not abbreviate.
|
@kalexand-rh Is there pre-review web page, so that I can review this more easily. |
1c7e855 to
dc1c851
Compare
@jianlinliu, here's a built version: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oc registry sounds like not validating user:password, directly saving credentials. It is better to have a warning message to prompt user to input correct user:password.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, "Mirroring the OpenShift Container Platform image repository" is mirror-to-registry scenario, "Mirroring the content" is mirror-to-disk scenario. They are should be at the same level. So can we re-arrange the doc structure to make the two scenarios as some kind of list for easy reading? Such as:
Mirroring the OpenShift Container Platform image repository:
-
mirror release image to local registry directly
-
mirror release image to disk files, then upload files to local registry
BTW, is it possible to make 'product_version' and 'mirror_repository' be more specific, just like what we did in "Mirroring the OpenShift Container Platform image repository" section to introduce environment variables, so that make mirror-to-registry and mirror-to-disk in a consistent style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianlinliu, if I make the introduction cover both use cases, can we just use this module instead? https://github.com/openshift/openshift-docs/pull/21993/files#diff-ac4ad58a22744e32fc45f760386799bbR1
The last commit shows what that might look like. I updated the preview to show what this might look like: http://file.rdu.redhat.com/kalexand/062920/disconnected_changes/installing/install_config/installing-restricted-networks-preparations.html#installing-restricted-networks-preparations-mirroring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/openshift/openshift-docs/pull/21993/files#diff-ac4ad58a22744e32fc45f760386799bbR1 focus on upgrade, but not install. To some content, whatever upgrade or fresh install, the mirror commands may be the same. But there is sill some difference in the whole process. For example, install need a detailed steps about how to record imageContentSources, and inject it into install-config.yaml; install section should never have "--apply-release-image-signature" option when mirror image to local registry, which is only applicable for upgrade scenario.
I went through the preview doc, I am okay with its layout, especially in "3. Mirror the version images to the internal container registry". But maybe need more polish for new "mirror-to-disk" scenario with detailed steps, especially we should try our best to avoid new issues for existing "mirror-to-reigstry" scenario (so far its steps is working perfectly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianlinliu, will you please take another look? I had a chat with Trevor about trying to get the imageContentSources information for the "mirror-to-disk" scenario, and there's not an automatic way to do it. He said that the "mirror-to-disk" commands that are in the reworked module (and the refreshed preview build) should work. I took the "mirror-to-registry" commands from before and reformatted it all to follow the same format as the upgrade steps.
82b813f to
6d82e4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step should belong to mirror-to-disk scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for a fresh install, so there should be no "the cluster are connected to the mirror host" case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About how to capture imageContentSources in mirror-to-disk scenario, I already got a resolution from https://bugzilla.redhat.com/show_bug.cgi?id=1806779#c9, And QE's automation script is following such way to get it working well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jianlinliu, can you be more specific? Trevor said that the enhancement to output the imageContentSources was rejected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take an example here.
Firstly run mirror-to-registry command with '--run-dry' option to capture the imageContentSources output.
$ oc adm -a ${LOCAL_SECRET_JSON} release mirror
--from=quay.io/${PRODUCT_REPO}/${RELEASE_NAME}:${OCP_RELEASE}-${ARCHITECTURE}
--to=${LOCAL_REGISTRY}/${LOCAL_REPOSITORY}
--to-release-image=${LOCAL_REGISTRY}/${LOCAL_REPOSITORY}:${OCP_RELEASE}-${ARCHITECTURE} --run-dry
Then run mirror-to-disk command to upload the real contents to registry, but have to be the same target repository like the above command.
$ oc image mirror -a ${LOCAL_SECRET_JSON} --from-dir=${REMOVABLE_MEDIA_PATH}/mirror 'file://openshift/release:${OCP_RELEASE}*' ${LOCAL_REGISTRY}/${LOCAL_REPOSITORY}
|
@jianlinliu, will you please take another look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command is for mirror-to-registry case, should be:
$ oc adm release mirror -a ${LOCAL_SECRET_JSON} --to-dir=${REMOVABLE_MEDIA_PATH}/mirror quay.io/${PRODUCT_REPO}/${RELEASE_NAME}:${OCP_RELEASE}-${ARCHITECTURE}
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No explanation and description for <8>
|
@jianlinliu, thank you. Is there anything else amiss? And will you please confirm the versions - is this still valid in 4.3+ or is it 4.4+, like the upgrade PR? |
4.3 contains changes that simplify working with credentials and true disconnected. Updating the docs (draft) to capture the differences and deemphasize the custom bastion, and encourage using your own. More details about what is required from a mirror registry. Try to split out the bits of steps for disconnected and connected mirroring. There are connected mirror use cases.
7cc0edf to
b1dd043
Compare
|
LGTM.
This should be valid in 4.3+, but 4.3 still need has one more fix - https://bugzilla.redhat.com/show_bug.cgi?id=1806782. I can confirm 4.4+ is ready. |
|
@jianlinliu, thank you so much for working with me on getting this finished. I'm going to merge this change to 4.4 and 4.5 now and check back with @wking when that backport is merged so that the docs go live in 4.3 at the right time. @vikram-redhat, FYI |
|
/cherrypick enterprise-4.5 |
|
/cherrypick enterprise-4.4 |
|
@kalexand-rh: #17896 failed to apply on top of branch "enterprise-4.4": 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. |
|
@kalexand-rh: new pull request created: #23586 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. |
4.3 contains changes that simplify working with credentials and true
disconnected. Updating the docs (draft) to capture the differences
and deemphasize the custom bastion, and encourage using your own.
More details about what is required from a mirror registry.
Try to split out the bits of steps for disconnected and connected
mirroring. There are connected mirror use cases.
This is still heavily draft, and has no catalog changes, but a
vehicle for discussion.