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

OCPBUGS-50559: v2: disable spinners when not running in a tty #1049

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

r4f4
Copy link
Contributor

@r4f4 r4f4 commented Jan 29, 2025

Description

If oc-mirror is run behind a CI/CD pipeline or if output is redirected to a file/pipe, the spinners' output is not shown and it looks like oc-mirror is not making any progress. By using a regular log line instead, we can still show some progress logs in those cases.

Github / Jira issue:

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code Improvements (Refactoring, Performance, CI upgrades, etc)
  • Internal repo assets (diagrams / docs on github repo)
  • This change requires a documentation update on openshift docs

How Has This Been Tested?

m2d with | grep '' to force a non-tty output.

Expected Outcome

Log lines are printed as images are copied (or fail to copy).

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 29, 2025
@r4f4
Copy link
Contributor Author

r4f4 commented Jan 29, 2025

Example run:

 $ ./bin/oc-mirror --v2 -c ~/Downloads/oc-mirror-issue/isc2.yaml file:///$HOME/Downloads/oc-mirror-issue/ocpbugs-45059-v2 | grep ''

Are we running from a terminal? false
2025/01/29 18:20:26  [INFO]   : 👋 Hello, welcome to oc-mirror
2025/01/29 18:20:26  [INFO]   : ⚙  setting up the environment for you...
2025/01/29 18:20:26  [INFO]   : 🔀 workflow mode: mirrorToDisk
2025/01/29 18:20:27  [INFO]   : 🕵  going to discover the necessary images...
2025/01/29 18:20:27  [INFO]   : 🔍 collecting release images...
2025/01/29 18:20:27  [INFO]   : 🔍 collecting operator images...
2025/01/29 18:20:27  [INFO]   : Collecting catalog oci://$HOME/Downloads/redhat-operator-index
2025/01/29 18:20:27  [INFO]   : 🔍 collecting additional images...
2025/01/29 18:20:27  [INFO]   : 🔍 collecting helm images...
2025/01/29 18:20:27  [INFO]   : 🔂 rebuilding catalogs
2025/01/29 18:20:27  [INFO]   : Rebuilding catalog oci://$HOME/Downloads/redhat-operator-index
2025/01/29 18:20:27  [INFO]   : 🚀 Start copying the images...
2025/01/29 18:20:27  [INFO]   : 📌 images to copy 7
2025/01/29 18:20:27  [INFO]   : Batch copying docker://registry.redhat.io/openshift4/ose-cluster-kube-descheduler-operator@sha256:ba0b71ff2a30a069b4a8a8f3c1e0898aaadc6db112e4cc12aff7c77ced7a0405 to docker://localhost:55000/openshift4/ose-cluster-kube-descheduler-operator:sha256-ba0b71ff2a30a069b4a8a8f3c1e0898aaadc6db112e4cc12aff7c77ced7a0405
2025/01/29 18:20:27  [INFO]   : Batch copying docker://registry.redhat.io/openshift4/ose-descheduler@sha256:45dc69ad93ab50bdf9ce1bb79f6d98f849e320db68af30475b10b7f5497a1b13 to docker://localhost:55000/openshift4/ose-descheduler:sha256-45dc69ad93ab50bdf9ce1bb79f6d98f849e320db68af30475b10b7f5497a1b13
2025/01/29 18:20:27  [INFO]   : Batch copying docker://registry.redhat.io/kube-descheduler-operator/descheduler-rhel9@sha256:7ba3f46cbbec2e47edcc255fae405bdbf2b9df3a232fc6809ce65fb1827a3b45 to docker://localhost:55000/kube-descheduler-operator/descheduler-rhel9:sha256-7ba3f46cbbec2e47edcc255fae405bdbf2b9df3a232fc6809ce65fb1827a3b45
2025/01/29 18:20:27  [INFO]   : Batch copying docker://registry.redhat.io/kube-descheduler-operator/kube-descheduler-rhel9-operator@sha256:03404d74da227fdf5805f61657272af2c915ce0014d6a902ef384b7a70bd10e2 to docker://localhost:55000/kube-descheduler-operator/kube-descheduler-rhel9-operator:sha256-03404d74da227fdf5805f61657272af2c915ce0014d6a902ef384b7a70bd10e2
2025/01/29 18:20:27  [INFO]   : Batch copying docker://registry.redhat.io/openshift4/ose-cluster-kube-descheduler-operator-bundle@sha256:b473fba287414d3ccb09aaabc64f463af2c912c322ca2c41723020b216d98d14 to docker://localhost:55000/openshift4/ose-cluster-kube-descheduler-operator-bundle:sha256-b473fba287414d3ccb09aaabc64f463af2c912c322ca2c41723020b216d98d14
2025/01/29 18:20:27  [INFO]   : Batch copying docker://registry.redhat.io/kube-descheduler-operator/kube-descheduler-operator-bundle@sha256:72c2aeb630281a636cad334fbbf0e67b70afba26c61a1b25a2b93277765e5ac7 to docker://localhost:55000/kube-descheduler-operator/kube-descheduler-operator-bundle:sha256-72c2aeb630281a636cad334fbbf0e67b70afba26c61a1b25a2b93277765e5ac7
2025/01/29 18:20:27  [INFO]   : Batch copying oci:///$HOME/Downloads/redhat-operator-index to docker://localhost:55000/ocicatalog73452:v16
2025/01/29 18:20:31  [INFO]   : === Results ===
2025/01/29 18:20:31  [INFO]   :  ✓  7 / 7 operator images mirrored successfully
2025/01/29 18:20:31  [INFO]   : 📦 Preparing the tarball archive...
2025/01/29 18:20:48  [INFO]   : mirror time     : 21.588107601s
2025/01/29 18:20:48  [INFO]   : 👋 Goodbye, thank you for using oc-mirror

@lmzuccarelli
Copy link
Contributor

@r4f4 - verified this locally and its working as expected. I like the pipe to grep '' (to simulate a no tty)

@lmzuccarelli
Copy link
Contributor

I'm happy to merge this.

@r4f4 - Do you think we need to document this ?

@r4f4 r4f4 force-pushed the disable-spinners-non-tty branch from ec33f84 to f820c5a Compare January 30, 2025 10:13
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2025
@r4f4 r4f4 force-pushed the disable-spinners-non-tty branch from f820c5a to 4ec6db5 Compare January 30, 2025 10:17
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2025
@r4f4
Copy link
Contributor Author

r4f4 commented Jan 30, 2025

I'm happy to merge this.

I still need to test the failure path. Not sure yet how to do that.

@r4f4 - Do you think we need to document this ?

Good question. I don't think we make any guarantees about our output so, in theory, no. But if we treat this as a bug (in case we want to backport to 4.18), then it can be documented as part of the bug fix notes.

@r4f4 r4f4 force-pushed the disable-spinners-non-tty branch 2 times, most recently from 1438c14 to be4bc01 Compare January 30, 2025 14:25
@r4f4
Copy link
Contributor Author

r4f4 commented Jan 30, 2025

Example output in case of failure:

$ ./bin/oc-mirror --v2 -c ~/Downloads/oc-mirror-issue/isc2.yaml file://$HOME/Downloads/oc-mirror-issue/ocpbugs-45059-v2 | grep ''

2025/01/30 14:14:02  [INFO]   : 👋 Hello, welcome to oc-mirror
2025/01/30 14:14:02  [INFO]   : ⚙  setting up the environment for you...
2025/01/30 14:14:02  [INFO]   : 🔀 workflow mode: mirrorToDisk
2025/01/30 14:14:03  [INFO]   : 🕵  going to discover the necessary images...
2025/01/30 14:14:03  [INFO]   : 🔍 collecting release images...
2025/01/30 14:14:03  [INFO]   : 🔍 collecting operator images...
2025/01/30 14:14:03  [INFO]   : Collected catalog oci://$HOME/Downloads/redhat-operator-index
2025/01/30 14:14:03  [INFO]   : 🔍 collecting additional images...
2025/01/30 14:14:03  [INFO]   : 🔍 collecting helm images...
2025/01/30 14:14:03  [INFO]   : 🔂 rebuilding catalogs
2025/01/30 14:14:03  [INFO]   : Rebuilding catalog oci://$HOME/Downloads/redhat-operator-index
2025/01/30 14:14:03  [INFO]   : 🚀 Start copying the images...
2025/01/30 14:14:03  [INFO]   : 📌 images to copy 8
2025/01/30 14:14:04  [INFO]   : Success copying oci://$HOME/Downloads/redhat-operator-index
2025/01/30 14:14:05  [ERROR]  : Failed copying image registry.redhat.io/invalid/invalid:latest
2025/01/30 14:14:06  [INFO]   : Success copying docker://registry.redhat.io/kube-descheduler-operator/kube-descheduler-operator-bundle@sha256:72c2aeb630281a636cad334fbbf0e67b70afba26c61a1b25a2b93277765e5ac7
2025/01/30 14:14:07  [INFO]   : Success copying docker://registry.redhat.io/openshift4/ose-cluster-kube-descheduler-operator-bundle@sha256:b473fba287414d3ccb09aaabc64f463af2c912c322ca2c41723020b216d98d14
2025/01/30 14:14:10  [INFO]   : Success copying docker://registry.redhat.io/openshift4/ose-descheduler@sha256:45dc69ad93ab50bdf9ce1bb79f6d98f849e320db68af30475b10b7f5497a1b13
2025/01/30 14:14:10  [INFO]   : Success copying docker://registry.redhat.io/kube-descheduler-operator/kube-descheduler-rhel9-operator@sha256:03404d74da227fdf5805f61657272af2c915ce0014d6a902ef384b7a70bd10e2
2025/01/30 14:14:11  [INFO]   : Success copying docker://registry.redhat.io/openshift4/ose-cluster-kube-descheduler-operator@sha256:ba0b71ff2a30a069b4a8a8f3c1e0898aaadc6db112e4cc12aff7c77ced7a0405
2025/01/30 14:14:12  [INFO]   : Success copying docker://registry.redhat.io/kube-descheduler-operator/descheduler-rhel9@sha256:7ba3f46cbbec2e47edcc255fae405bdbf2b9df3a232fc6809ce65fb1827a3b45
2025/01/30 14:14:12  [INFO]   : === Results ===
2025/01/30 14:14:12  [INFO]   :  ✓  7 / 7 operator images mirrored successfully
2025/01/30 14:14:12  [INFO]   :  ✗  0 / 1 additional images mirrored: Some additional images failed to be mirrored - please check the logs
2025/01/30 14:14:12  [ERROR]  : [Worker] error mirroring image registry.redhat.io/invalid/invalid:latest error: initializing source docker://registry.redhat.io/invalid/invalid:latest: reading manifest latest in registry.redhat.io/invalid/invalid: unauthorized: access to the requested resource is not authorized
2025/01/30 14:14:12  [INFO]   : 📦 Preparing the tarball archive...
2025/01/30 14:14:34  [INFO]   : mirror time     : 31.13926561s
2025/01/30 14:14:34  [WARN]   : [Worker] some errors occurred during the mirroring.
         Please review $HOME/Downloads/oc-mirror-issue/ocpbugs-45059-v2/working-dir/logs/mirroring_errors_20250130_141412.txt for a list of mirroring errors.
         You may consider:
         * removing images or operators that cause the error from the image set config, and retrying
         * keeping the image set config (images are mandatory for you), and retrying
         * mirroring the failing images manually, if retries also fail.
2025/01/30 14:14:34  [INFO]   : 👋 Goodbye, thank you for using oc-mirror

@r4f4
Copy link
Contributor Author

r4f4 commented Jan 30, 2025

/retitle v2: disable spinners when not running in a tty

@openshift-ci openshift-ci bot changed the title WIP: disable spinners when not running in a tty v2: disable spinners when not running in a tty Jan 30, 2025
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 30, 2025
@r4f4
Copy link
Contributor Author

r4f4 commented Jan 31, 2025

/hold

I want to try out more failure scenarios.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2025
@aguidirh
Copy link
Contributor

/label acknowledge-critical-fixes-only

@openshift-ci openshift-ci bot added the acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. label Jan 31, 2025
@aguidirh
Copy link
Contributor

/approve

Copy link

openshift-ci bot commented Jan 31, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aguidirh, r4f4

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2025
@aguidirh
Copy link
Contributor

@r4f4 feel free to merge when you get the tests from QE done.

@soukron
Copy link

soukron commented Feb 3, 2025

FYI: Mirroring the stable-2.4 channel of the automation platform operator (aap-operator) with all the images full: true should fail.

@kasturinarra
Copy link

Pre-merge tested the PR below are some of the observations:

  1. I see output is different when running oc-mirror commands with and with out interactive terminal as discussed in thread
  2. Do not see any log for delete, it just says deleteing 191 images then after sometime outputs deleted images successfully
  3. command used to delete is ./oc-mirror delete --delete-yaml-file newissue/working-dir/delete/delete-images.yaml docker://localhost:5000 --v2 --dest-tls-verify=false > outputdelete1.log 2>&1
[cloud-user@preserve-oc-mirror-vm openshift-tests-private]$ cat outputdelete1.log 

2025/02/03 06:55:26  [INFO]   : 👋 Hello, welcome to oc-mirror
2025/02/03 06:55:26  [INFO]   : ⚙️  setting up the environment for you...
2025/02/03 06:55:26  [INFO]   : 🔀 workflow mode: diskToMirror / delete
2025/02/03 06:55:27  [INFO]   : 👀 Reading delete file...
2025/02/03 06:55:27  [INFO]   : 🚀 Start deleting the images...
2025/02/03 06:55:27  [INFO]   : 📌 images to delete 191 
2025/02/03 06:55:29  [INFO]   : === Results ===
2025/02/03 06:55:29  [INFO]   :  ✓  191 / 191 release images deleted successfully
2025/02/03 06:55:29  [INFO]   : delete time     : 2.413832604s
2025/02/03 06:55:29  [INFO]   : 📝 Remember to execute a garbage collect (or similar) on your remote repository
2025/02/03 06:55:29  [INFO]   : 👋 Goodbye, thank you for using oc-mirror

@r4f4 r4f4 force-pushed the disable-spinners-non-tty branch from be4bc01 to b2a0353 Compare February 3, 2025 14:55
@r4f4
Copy link
Contributor Author

r4f4 commented Feb 3, 2025

Update: following QE feedback, I've made the following changes:

  • Fixed the terminal detection for the delete command
  • reorganized the code a bit
  • changed the output to be more consistent with the spinner's:
# m2d
2025/02/03 15:40:31  [INFO]   : Success copying quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:9156750af139aaaf5eef82b90e0e888b91e9e2cf86a1d5a76257e5f16b928d0e ➡ cache
# d2m
2025/02/03 15:47:23  [INFO]   : Success copying quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:06c93951661e3bdad3964a8485291b97e93f6675c25acfc8399c35d3b59b95fa ➡ localhost:5000/mirror-data/openshift
# delete
2025/02/03 16:10:26  [INFO]   : Success deleting docker://localhost:55000/openshift/graph-image:latest ➡ localhost:5000/mirror-data/openshift/
2025/02/03 16:10:26  [INFO]   : Success deleting docker://localhost:55000/openshift/graph-image:latest ➡ cache

@r4f4
Copy link
Contributor Author

r4f4 commented Feb 3, 2025

Example of delete failure:

2025/02/03 16:10:26  [ERROR]  : Failed to delete operatorRelatedImage docker://registry.redhat.io/noo/node-observability-agent-rhel8@sha256:a8e60d9f63e2ed0cf1e27309aac54014e3f1efc947b51ca0a99376338d220875
2025/02/03 16:10:26  [ERROR]  : Failed to delete operatorBundle docker://registry.redhat.io/noo/node-observability-operator-bundle-rhel8@sha256:0fbbb0b32adadc5746251103d25bba6bd365b2ff60e289e4694061fc92c3ce6e
2025/02/03 16:10:26  [ERROR]  : Failed to delete operatorRelatedImage docker://registry.redhat.io/noo/node-observability-rhel8-operator@sha256:53ba77f648154ac8aca1ae6c83815078dfde211dba1e42f597e69844e4954c01
2025/02/03 16:10:26  [ERROR]  : Failed to delete operatorBundle docker://registry.redhat.io/noo/node-observability-operator-bundle-rhel8@sha256:0fbbb0b32adadc5746251103d25bba6bd365b2ff60e289e4694061fc92c3ce6e
2025/02/03 16:10:26  [ERROR]  : Failed to delete operatorRelatedImage docker://registry.redhat.io/noo/node-observability-agent-rhel8@sha256:a8e60d9f63e2ed0cf1e27309aac54014e3f1efc947b51ca0a99376338d220875
2025/02/03 16:10:26  [ERROR]  : Failed to delete operatorRelatedImage docker://registry.redhat.io/noo/node-observability-rhel8-operator@sha256:53ba77f648154ac8aca1ae6c83815078dfde211dba1e42f597e69844e4954c01

@r4f4 r4f4 force-pushed the disable-spinners-non-tty branch from b2a0353 to 6c207f0 Compare February 4, 2025 15:50
@r4f4
Copy link
Contributor Author

r4f4 commented Feb 4, 2025

Update: rebased on top of main to fix merge conflicts.

@r4f4
Copy link
Contributor Author

r4f4 commented Feb 4, 2025

/test sanity

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2025
If oc-mirror is run behind a CI/CD pipeline or if output is redirected
to a file/pipe, the spinners' output is not shown and it looks like
oc-mirror is not making any progress. By using a regular log line
instead, we can still show some progress logs in those cases.
@r4f4
Copy link
Contributor Author

r4f4 commented Feb 11, 2025

/retitle OCPBUGS-50559: v2: disable spinners when not running in a tty

@openshift-ci openshift-ci bot changed the title v2: disable spinners when not running in a tty OCPBUGS-50559: v2: disable spinners when not running in a tty Feb 11, 2025
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Feb 11, 2025
@openshift-ci-robot
Copy link

@r4f4: This pull request references Jira Issue OCPBUGS-50559, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (4.19.0) matches configured target version for branch (4.19.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @zhouying7780

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Description

If oc-mirror is run behind a CI/CD pipeline or if output is redirected to a file/pipe, the spinners' output is not shown and it looks like oc-mirror is not making any progress. By using a regular log line instead, we can still show some progress logs in those cases.

Github / Jira issue:

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Code Improvements (Refactoring, Performance, CI upgrades, etc)
  • Internal repo assets (diagrams / docs on github repo)
  • This change requires a documentation update on openshift docs

How Has This Been Tested?

m2d with | grep '' to force a non-tty output.

Expected Outcome

Log lines are printed as images are copied (or fail to copy).

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 openshift-ci bot requested a review from zhouying7780 February 11, 2025 09:49
@r4f4 r4f4 force-pushed the disable-spinners-non-tty branch from 6c207f0 to b5475a2 Compare February 11, 2025 09:50
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2025
@r4f4
Copy link
Contributor Author

r4f4 commented Feb 11, 2025

Update: rebased to fix merge conflicts.

@r4f4
Copy link
Contributor Author

r4f4 commented Feb 11, 2025

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2025
Copy link

openshift-ci bot commented Feb 11, 2025

@r4f4: all tests passed!

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.

@lmzuccarelli
Copy link
Contributor

@r4f4 - I'll add the correct labels once QE has verified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
acknowledge-critical-fixes-only Indicates if the issuer of the label is OK with the policy. approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants