-
Notifications
You must be signed in to change notification settings - Fork 13
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
mustgather: Dockerfile: use ocp CLI base image #1063
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shajmakh 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 |
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.
conceptually fine, but let's narrow down the image
1c1a002
to
5c873d4
Compare
Initially, must-gather for NROP was built by grabbing the latest openshift-ose-must-gather image, copying the required binaries for running must gather and override the collection scripts and base that on rhel-minimal. While this works, there is no need for extra dependencies on images when all we need is the openshift clients to gather the must-gather collection which is available in the CLI image. Signed-off-by: Shereen Haj <[email protected]>
5c873d4
to
6937670
Compare
@shajmakh: The following test 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. |
# Save original gather script | ||
COPY --from=builder /usr/bin/gather* /usr/bin/ | ||
RUN mv /usr/bin/gather /usr/bin/gather_original | ||
FROM quay.io/openshift/origin-cli:4.18.0 |
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.
ok, this is now me nitpicking, but still: is this tag pointing to the latest build of 4.18.z or to the latest build of 4.18.0?
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 addition, can you please link how the origin-must-gather
image is built (its Dockerfile)?
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.
4.X and 4.X.0 have the same content. there are no 4.X.z images in quay:
https://quay.io/repository/openshift/origin-cli?tab=tags&tag=latest
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 main must-gather (=origin-must-gather) uses the origin-cli base too:
https://github.com/openshift/must-gather/blob/master/Dockerfile.ocp
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.
thanks, this looks good enough!
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.
so can we use origin-cli:4.18
like the linked Dockerfile does?
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.
Yes that's the equivalent u/s quay image
@@ -10,7 +10,7 @@ resources+=(nodes machineconfigs machineconfigpools featuregates kubeletconfigs) | |||
|
|||
# run the collection of resources using must-gather | |||
for resource in ${resources[@]}; do | |||
/usr/bin/oc adm inspect --dest-dir must-gather --all-namespaces ${resource} | |||
oc adm inspect --dest-dir must-gather --all-namespaces ${resource} |
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.
please keep the abspath (may need some amends)
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 is no longer explicitly copied because it exists in the new CLI base
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.
fine, but still we can use the absolute path (abspath) in the invocation, can't we?
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.
we can but might cause issues d/s. why do you think we should keep it?
A midstream MR depends on this actually, since there we use another dockerfile in terms of the base we aim to no longer copy the oc binary. I still need to see how m/s will end up. we can use an env var for the binary if that helps
MG test passes with some adjustments, I'm post a fix in a different PR. |
Initially, must-gather for NROP was built by grabbing the latest openshift-ose-must-gather image, copying the required binaries for running must gather and override the collection scripts and base that on rhel-minimal.
While this works, there is no need for extra dependencies on images when all we need is the openshift clients to gather the must-gather collection which is available in the CLI image.