Skip to content
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

chore: CRW-3429 - update docs for imagepuller #2493

Closed
wants to merge 3 commits into from
Closed

Conversation

nickboldt
Copy link
Contributor

@nickboldt nickboldt commented Nov 14, 2022

What does this PR do?

Draft: chore: fix docs to remove mention of OS 3.11; use new script for collecting related images from CSVs instead of vague instructions about parsing external_images.txt, which doesn't have all the images (missing traefik and DWO project-clone); update antora variables to DWO 0.16

Signed-off-by: Nick Boldt nboldt@redhat.com

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

https://issues.redhat.com/browse/CRW-3553

How to test this PR?

N/A

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@github-actions
Copy link

Click here to review and test in web IDE: Contribute

@github-actions
Copy link

Click here to review and test in web IDE: Contribute

@github-actions
Copy link

github-actions bot commented Nov 14, 2022

🎊 Navigate the preview: https://6374f8e98bed9521711bb8b4--eclipse-che-docs-pr.netlify.app 🎊

@che-bot che-bot added this to Triage in Reviews Nov 14, 2022
@nickboldt nickboldt changed the title chore: fix docs to remove mention of OS... chore: CRW-3429- fix docs to remove mention of OS 3.11; use new script for collecting related images from CSVs instead of vague instructions about parsing external_images.txt, which doesn't have all the images (missing traefik and DWO project-clone); update antora variables to DWO 0.16 Nov 14, 2022
@nickboldt nickboldt changed the title chore: CRW-3429- fix docs to remove mention of OS 3.11; use new script for collecting related images from CSVs instead of vague instructions about parsing external_images.txt, which doesn't have all the images (missing traefik and DWO project-clone); update antora variables to DWO 0.16 chore: CRW-3429 - update docs for imagepuller Nov 14, 2022
@nickboldt nickboldt requested a review from svor November 15, 2022 13:40
@max-cx max-cx self-assigned this Nov 15, 2022
@che-bot che-bot moved this from Triage to In progress in Reviews Nov 15, 2022
@nickboldt
Copy link
Contributor Author

@nickboldt nickboldt force-pushed the CRW-3429_ branch 2 times, most recently from 11d1157 to 2fcf645 Compare November 15, 2022 14:13
…ecting related images from CSVs instead of vague instructions about parsing external_images.txt, which doesn't have all the images (missing traefik and DWO project-clone); update antora variables to DWO 0.16

Signed-off-by: Nick Boldt <nboldt@redhat.com>

Max's suggestion, minus che-theia images, minus the duplicate footer line

Signed-off-by: Nick Boldt <nboldt@redhat.com>
Co-authored-by: Max Leonov <mleonov@redhat.com>
Copy link
Contributor

@svor svor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviews automation moved this from In progress to Approved Nov 15, 2022
@nickboldt nickboldt changed the title chore: CRW-3429 - update docs for imagepuller Draft: chore: CRW-3429 - update docs for imagepuller Nov 15, 2022
Comment on lines 7 to 8
[id="defining-the-list-of-images-to-pull"]
= Defining the list of images to pull
Copy link
Contributor

@max-cx max-cx Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[id="defining-the-list-of-images-to-pull"]
= Defining the list of images to pull
[id="configuring-image-puller-to-cache-images-for-faster-workspace-startup"]
= Configuring {image-puller-name-short} to cache images for faster workspace startup

Copy link
Contributor

@max-cx max-cx Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nick, if you change this, please don't forget to rename the file to configuring-image-puller-to-cache-images-for-faster-workspace-startup.adoc and update this file name also in nav.adoc.

@nickboldt
Copy link
Contributor Author

Don't merge yet, there's a discussion going on w/ Mario about maybe moving this functionality to chectl/dsc.

Suggestion is to implement these commands:

dsc server:imagelist --format:pairs

and

dsc server:imagelist --format:per-line

the first format being for use with KIP; the second being an easier-to-read list of images@sha256:digest (or tag)

-- https://coreos.slack.com/archives/CR87LL0PL/p1668523246810059?thread_ts=1668453951.417379&cid=CR87LL0PL

curl -sSLkO https://raw.githubusercontent.com/redhat-developer/devspaces/devspaces-3-rhel-8/product/getRelatedImages.sh && \
chmod +x getRelatedImages.sh && \
./getRelatedImages.sh -t {prod-ver-patch} -d {devworkspace-operator-version} --quiet
----

Copy link

@agiertli agiertli Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickboldt @max-cx The script does not seem to work on OS X:

./getRelatedImages.sh -t 3.2 -d 0.16


[INFO] Using local quay.io/devspaces/devspaces-operator-bundle:3.2 (a7b1881d3b13)...
Error: open /dev/stdout: permission denied
find: /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.VJs2Fa9N/quay.io-*-devspaces-operator-bundle-3.2-*/manifests/: No such file or directory
[INFO] Checking CSV:
Error: unknown shorthand flag: 'r' in -r
Usage:
  yq [flags]
  yq [command]

I have my doubts about replacing incomplete solution with a broken one. Can this be reconsidered?

Copy link
Contributor Author

@nickboldt nickboldt Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which yq do you have installed? If it's the mikefarah one, not the jq wrapper written in python, then you'll have a bad time. I can fluff up the script so it only works with the python based yq...

Copy link
Contributor Author

@nickboldt nickboldt Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but also +100, this was a simple prototype that would fetch images from CSVs that I tested on Fedora 36. I don't have an easy way to test on Apple hardware... but I could wrap the whole thing in a container ;)

The other suggested approach is to implement this functionality inside chectl, so that it's platform agnostic.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @nickboldt I reinstalled the yq with the python version but it still fails because of

Error: open /dev/stdout: permission denied

Which I assume it's related to
containers/podman#12402
I am using Podman 4.3

It now fails with:

bash -x ./getRelatedImages.sh -t 3.2 -d 0.16
+ QUIET=
+ [[ 4 -gt 0 ]]
+ case $1 in
+ VERSION=3.2
+ shift 1
+ shift 1
+ [[ 2 -gt 0 ]]
+ case $1 in
+ DWO_VERSION=0.16
+ shift 1
+ shift 1
+ [[ 0 -gt 0 ]]
+ [[ ! -n 3.2 ]]
+ [[ ! -n 0.16 ]]
++ mktemp -d
+ TMPDIR=/var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP
+ cd /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP
+ [[ ! -f /tmp/containerExtract.sh ]]
+ curl -sSLO https://raw.githubusercontent.com/redhat-developer/devspaces/devspaces-3-rhel-8/product/containerExtract.sh
+ chmod +x /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/containerExtract.sh
+ EXTRACT_CMD='/var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/containerExtract.sh  --tmpdir /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP --delete-before --delete-after'
+ [[ 3.2 == \7\.* ]]
+ [[ 3.2 == \8\.* ]]
+ [[ 3.2 == \3\.* ]]
+ rm -fr '/var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/quay.io-*-devspaces-operator-bundle-3.2-*'
+ /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/containerExtract.sh --tmpdir /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP --delete-before --delete-after quay.io/devspaces/devspaces-operator-bundle:3.2
[INFO] Using local quay.io/devspaces/devspaces-operator-bundle:3.2 (a7b1881d3b13)...
Error: open /dev/stdout: permission denied
++ find '/var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/quay.io-*-devspaces-operator-bundle-3.2-*/manifests/' -name '*csv.yaml'
find: /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/quay.io-*-devspaces-operator-bundle-3.2-*/manifests/: No such file or directory
+ CSV=
+ [[ '' == '' ]]
+ echo '[INFO] Checking CSV: '
[INFO] Checking CSV:
+ yq -r '.spec.relatedImages[].image' ''
+ sort -uV
usage: yq [-h] [--yaml-output] [--yaml-roundtrip]
          [--yaml-output-grammar-version {1.1,1.2}] [--width WIDTH]
          [--indentless-lists] [--in-place] [--version]
          [jq_filter] [files ...]
yq: error: argument files: can't open '': [Errno 2] No such file or directory: ''
+ [[ '' == '' ]]
+ echo

+ rm -fr '/var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/quay.io-*-devworkspace-operator-bundle-0.16'
+ /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/containerExtract.sh --tmpdir /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP --delete-before --delete-after quay.io/devworkspace/devworkspace-operator-bundle:0.16
[INFO] Using local quay.io/devworkspace/devworkspace-operator-bundle:0.16 (0cf26f525cb1)...
Error: open /dev/stdout: permission denied
++ find '/var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/quay.io-*-devworkspace-operator-bundle-0.16-*/manifests/' -name '*clusterserviceversion.yaml'
find: /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/quay.io-*-devworkspace-operator-bundle-0.16-*/manifests/: No such file or directory
+ CSV=
+ [[ '' == '' ]]
+ echo '[INFO] Checking CSV: '
[INFO] Checking CSV:
+ yq -r '.spec.relatedImages[].image' ''
+ sort -uV
usage: yq [-h] [--yaml-output] [--yaml-roundtrip]
          [--yaml-output-grammar-version {1.1,1.2}] [--width WIDTH]
          [--indentless-lists] [--in-place] [--version]
          [jq_filter] [files ...]
yq: error: argument files: can't open '': [Errno 2] No such file or directory: ''
+ [[ '' == '' ]]
+ echo

+ cd /tmp
+ rm -fr /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP

If the script already requires docker/podman I believe containerizing would make most sense as it completely removes the risk of running into env specific issue.

It would be also nice if it supported configuring container engine, i.e. ./script .sh-ce DOCKER

Copy link

@agiertli agiertli Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you also mentioned chectl, that would be even better alternative. I still think this should be more of a supplementary solution and the real one should be improving the default image list when image puller is enabled. For many use cases the default list would be a great starting point already, so I'd prefer not to manually configuring it - especially if I will simply reuse output of ./getRelatedImages.sh.

Copy link
Contributor Author

@nickboldt nickboldt Nov 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: podman, I use 4.3.0 too, but on fedora 36. I've also set it up to be rootless. https://github.com/containers/podman/blob/main/docs/tutorials/rootless_tutorial.md

you could also debug the containerExtract.sh script like this:

bash -x /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP/containerExtract.sh --tmpdir /var/folders/yh/dtn4z9nd0cgf_y0trh7znd6m0000gn/T/tmp.7QvlykqP --delete-before --delete-after quay.io/devspaces/devspaces-operator-bundle:3.2

Re: improving the default image list https://github.com/che-incubator/kubernetes-image-puller/blob/main/deploy/openshift/configmap.yaml#L27 yes, that would be the best option, especially if chectl/dsc can just configure the imagepuller for you with the correct image set.

HOWEVER at that point the assumption might be that you want ALL images pulled? What if you don't need theia and idea, and only want vscode?

Copy link

@agiertli agiertli Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since VSCode will be the default choice soon (with theia going away and idea being a TP) I'd simply make sure vscode is part of the default list.

Copy link
Contributor Author

@nickboldt nickboldt Nov 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed, vscode image should definitely be in the default list... my point was that the tool/solution needs some configuration to allow only pulling what you want (rather than pulling everything).

Here's another approach that @amisevsk wrote yesterday... haven't tested it yet. https://gist.github.com/amisevsk/9df020a46156b1824cd71a45785449aa

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be interesting to combine these approaches. i.e. run your bash script inside the container as shown above. It wouldn't need serviceAccount/RoleBindings, so the YAML would be quite minimal. And it would give you a chance to review (==and pick&choose) those images before applying those, as the approach above directly sets those, rather silently. And it also removes any environmental factor, as the script is now running into a fixed environment all the time.

…-to-pull.adoc

Co-authored-by: Max Leonov <mleonov@redhat.com>
@themr0c themr0c moved this from Approved to Drafts in Reviews Nov 28, 2022
@nickboldt nickboldt marked this pull request as draft November 28, 2022 14:38
@max-cx max-cx changed the title Draft: chore: CRW-3429 - update docs for imagepuller chore: CRW-3429 - update docs for imagepuller Nov 28, 2022
@nickboldt
Copy link
Contributor Author

Updated to link to new issue https://issues.redhat.com/browse/CRW-3553

@nickboldt
Copy link
Contributor Author

Closed as we haven't found a consensus on which way to go here with the tech, and therefore also not the doc. Watch for news in https://issues.redhat.com/browse/CRW-3553

@nickboldt nickboldt closed this Jan 26, 2023
Reviews automation moved this from Drafts to Done Jan 26, 2023
@max-cx
Copy link
Contributor

max-cx commented Jan 26, 2023

Because this PR is now closed, I'm closing my related PR review issue, https://issues.redhat.com/browse/RHDEVDOCS-4738.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Reviews
  
Done
5 participants