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

Switch async images to UBI #11

Merged
merged 2 commits into from
Jan 15, 2021
Merged

Conversation

tomgeorge
Copy link
Contributor

Switch the alpine images to UBI, #10

Signed-off-by: Tom George [email protected]

@ibuziuk ibuziuk requested review from vparfonov, sleshchenko and ibuziuk and removed request for vparfonov December 21, 2020 10:02
Copy link
Contributor

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

Comment on lines 27 to 21
# Add user that will be able to start watcher binary but nothing more
# the result will be propagated then into scratch image
# See https://stackoverflow.com/a/55757473/12429735RUN
Copy link
Contributor

Choose a reason for hiding this comment

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

probably the comment should be udpated

Copy link

@benoitf benoitf left a comment

Choose a reason for hiding this comment

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

AFAIK ubi and non-ubi are separated Dockerfiles

like Dockerfile with alpine and Dockerfile.rhel with ubi8 from

@ibuziuk
Copy link
Contributor

ibuziuk commented Dec 21, 2020

@benoitf we do not plan to have non-ubi dockerfiles

@benoitf
Copy link

benoitf commented Dec 21, 2020

@ibuziuk why ? all images of che are not ubi based by default

@ibuziuk
Copy link
Contributor

ibuziuk commented Dec 21, 2020

@benoitf I believe it is up to plugin / component developers to decide ;-)
image puller is UBI based by default, eventhough we have centos image, it is not used anywhere (with the recent centos stream announcement it is doomed to be deprecated in general). In order to lower the support burden, I would opt for ubi / ubi-minimal only for async-storage

@benoitf
Copy link

benoitf commented Dec 21, 2020

@ibuziuk

quay.io/eclipse/che-workspace-data-sync-storage   latest               7b0123559264   48 seconds ago       251MB
quay.io/eclipse/che-sidecar-workspace-data-sync   latest               565bce0cc468   About a minute ago   248MB
quay.io/eclipse/che-workspace-data-sync-storage   alpine               66abd0279cd9   3 minutes ago        12.3MB
quay.io/eclipse/che-sidecar-workspace-data-sync   alpine               b615ce49c122   3 minutes ago        39.1MB

it's a diff of more than 200MB per image for nothing
Images should be as small as possible for upstream che.

@ibuziuk
Copy link
Contributor

ibuziuk commented Dec 21, 2020

@tomgeorge maybe it is possible to use - https://catalog.redhat.com/software/containers/ubi8/ubi-minimal/5c359a62bed8bd75a2c3fba8
@benoitf we can provide UBI & non-UBI versions. However, managing extra images creates an additional support burden for the teams and I believe we need to find an alternative to alpine that would be also suitable for product

Signed-off-by: Tom George <[email protected]>
@tomgeorge
Copy link
Contributor Author

tomgeorge commented Jan 8, 2021

I have working ubi-minimal images but they are still pretty big:

quay.io/eclipse/che-sidecar-workspace-data-sync                                    latest         155288cd5b71   11 minutes ago      173MB
quay.io/eclipse/che-workspace-data-sync-storage                                    latest         4f1236fc9180   About an hour ago   163MB

Copy link
Contributor

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

@tomgeorge based on the discussion with @benoitf let's have separate ubi and alpine images for the product and project respectively

@tomgeorge
Copy link
Contributor Author

Added the alpine Dockerfiles back and named the ubi dockerfiles ubi.Dockerfile

@benoitf benoitf dismissed their stale review January 11, 2021 10:42

ubi images are not replacing alpine one

@ibuziuk
Copy link
Contributor

ibuziuk commented Jan 14, 2021

@benoitf could you please review?

#
# SPDX-License-Identifier: EPL-2.0

FROM registry.redhat.io/ubi8/ubi-minimal:8.3
Copy link

@benoitf benoitf Jan 14, 2021

Choose a reason for hiding this comment

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

Please add a comment line

# https://access.redhat.com/containers/?tab=tags#/registry.access.redhat.com/ubi8-minimal

Helping to see new images

Also, please stick to a given version like FROM registry.access.redhat.com/ubi8-minimal:8.3-230

@@ -0,0 +1,67 @@
#!/bin/bash
Copy link

Choose a reason for hiding this comment

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

I think it's a copy/error typo of the header (Dockerfile is not a bash script)

# See https://stackoverflow.com/a/55757473/12429735RUN
#
RUN microdnf update -y \
&& microdnf install -y \
Copy link

Choose a reason for hiding this comment

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

please follow convention of && at end of lines or beginning of lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do have && at beginning of lines, it's the same as dockerfiles/agent/Dockerfile?

Copy link

Choose a reason for hiding this comment

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

Ok maybe I read the only wrong line

&& mkdir  /var/run/sshd && \

#
# SPDX-License-Identifier: EPL-2.0

FROM registry.redhat.io/ubi8/ubi-minimal:8.3
Copy link

Choose a reason for hiding this comment

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

same remarks than for the other Dockerfile (bash header, comment to see new updates and stick to a fine grain version tag)

@ibuziuk ibuziuk merged commit 35ee985 into che-incubator:master Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants