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

VOLUME declaration prevents this image working on OpenShift Online #30

Closed
bparees opened this issue Oct 5, 2017 · 17 comments
Closed

VOLUME declaration prevents this image working on OpenShift Online #30

bparees opened this issue Oct 5, 2017 · 17 comments

Comments

@bparees
Copy link
Contributor

bparees commented Oct 5, 2017

This image declares a VOLUME which means it can't be run as an s2i builder because it is impossible to run images on openshift online if they declare a volume and you do not map that volume to a mount point.

For running an image it's fine because the user can map the volume.

But when the image is run as a builder via s2i, there is no opportunity for the user to map a mount to the VOLUME and openshift online fails the build because the volume can't be written to.

I think the VOLUME declaration should be removed from the image, it's not like users can't still map a volume if they want to anyway.

@hhorak @sspeiche fyi.

@omron93
Copy link

omron93 commented Oct 9, 2017

This image declares a VOLUME which means it can't be run as an s2i builder because it is impossible to run images on openshift online if they declare a volume and you do not map that volume to a mount point.

@bparees What is a reason of this?
It means that database images (which are meant to be also extendable using s2i) can't be used as builder images too in OpenShift online, right?

I think the VOLUME declaration should be removed from the image, it's not like users can't still map a volume if they want to anyway.

I don't know how OpenShift exactly runs images. But with Docker this causes performance degradation - because if mount point is not specified "volume" is stored on layered file system (which has lower performance).

@bparees
Copy link
Contributor Author

bparees commented Oct 9, 2017

@bparees What is a reason of this?

the reason is that we can't manage/monitor disk usage that is allocated by docker in this way, so the only way to prevent abuse currently is to not allow these volumes to be created by docker at all.

It means that database images (which are meant to be also extendable using s2i) can't be used as builder images too in OpenShift online, right?

correct. it's a problem for any image that declares a VOLUME, but this is the first case we've had of an image that's likely to be used as a builder image, that also declares a VOLUME. (Technically the Jenkins image also has this problem, but it seems no one has run into it yet, but we should consider fixing that one too).

I don't know how OpenShift exactly runs images. But with Docker this causes performance degradation - because if mount point is not specified "volume" is stored on layered file system (which has lower performance).

I'm not sure what you're saying.... if the image did not declare a VOLUME then there would be two modes for the image to run under:

  1. no mounts specified, everything is in the container filesystem. no issue.
  2. user explicitly creates an openshift volume and mounts it to the container. no issue.

What we are explicitly trying to avoid is the third mode which is:
3) image declares a volume. user does not mount anything to that volume. docker creates a local volume and mounts it. Which I think is the case you're saying performs badly anyway (and in online, it fails completely).

@hhorak
Copy link
Member

hhorak commented Oct 9, 2017

@bparees Is there some documentation for the behaviour of OpenShift, when VOLUME is specified? If we removed all VOLUMES (or maybe better commented-out for documentation purposes), we should clearly link to the reasoning.

@bparees
Copy link
Contributor Author

bparees commented Oct 9, 2017

Is there some documentation for the behaviour of OpenShift, when VOLUME is specified?

no, and it's not openshift behavior, it's docker behavior.

If we removed all VOLUMES

i'm not suggesting you remove all of them. I'm not even necessarily suggesting you permanently remove this one(once online can tolerate running containers with unmapped VOLUMEs, you could reintroduce the declaration), but for now if you want this image to work as an s2i builder in openshift online, you need to remove the VOLUME.

@omron93
Copy link

omron93 commented Oct 10, 2017

I'm not sure what you're saying.... if the image did not declare a VOLUME then there would be two modes for the image to run under:

I think I've read it somewhere. But now I can't find it again... so take this note as no such serious :-)

  1. no mounts specified, everything is in the container filesystem. no issue.

This is the default. Lets take and example of database which uses one directory for storing data. This directory is heavily used for writing, other parts of image are mainly read-only. So in this case everything in that directory is stored in container writable layer (so in background using devicemapper, loop-devicemapper, overlayfs,... filesystem).

What we are explicitly trying to avoid is the third mode which is:
3) image declares a volume. user does not mount anything to that volume. docker creates a local volume and mounts it. Which I think is the case you're saying performs badly anyway (and in online, it fails completely).

So as I understand it, this is the good option. Even if mount directory on host is generated by docker somewhere in /var/lib/docker/volumes. Heavily writable directory is stored in host filesystem which is probably more performant because it don't have to union several layers each time it is used.

But this could change with new docker supported filesystems or something like this.

@pkubatrh
Copy link
Member

Makes sense to me to remove it temporarily if Openshift is unable to work with them when the images are used as s2i builders.
But it should really only be temporary and eventually be reintroduced when Openshift is fixed. It would be nice to have some issue against Openshift to track the state of the implementation there.

@bparees
Copy link
Contributor Author

bparees commented Oct 10, 2017

It would be nice to have some issue against Openshift to track the state of the implementation there.

there are open bugs for it. @abhgupta can point you to them.

and again to be clear, this is only a limitation in openshift online (unless an openshift administrator intentionally sets up their docker storage in the same we we've configured the online nodes).

@luciddreamz
Copy link

Related question - why is this so different than the s2i-php-container which also runs httpd?

hhorak added a commit to hhorak/httpd-container that referenced this issue Dec 7, 2017
It's rationalized here:
sclorg#30

Ideally this should be reverted once OpenShift Online is fixed.
omron93 pushed a commit that referenced this issue Dec 8, 2017
It's rationalized here:
#30

Ideally this should be reverted once OpenShift Online is fixed.
hhorak added a commit to hhorak/nginx-container that referenced this issue Dec 14, 2017
It's rationalized in sclorg/httpd-container#30

Ideally this should be reverted once OpenShift Online is fixed.
hhorak added a commit to sclorg/nginx-container that referenced this issue Dec 15, 2017
It's rationalized in sclorg/httpd-container#30

Ideally this should be reverted once OpenShift Online is fixed.
@hhorak
Copy link
Member

hhorak commented Dec 15, 2017

The VOLUMES are commented out now, although I'd keep this issue opened to track that it's not perfect now and once https://bugzilla.redhat.com/show_bug.cgi?id=1481918 is fixed, we can re-enable VOLUMES again.

@hhorak
Copy link
Member

hhorak commented Apr 23, 2018

This was already fixed via #41, closing.

@hhorak hhorak closed this as completed Apr 23, 2018
@hhorak
Copy link
Member

hhorak commented Apr 23, 2018

The VOLUMES are commented out now, although I'd keep this issue opened to track that it's not perfect now and once https://bugzilla.redhat.com/show_bug.cgi?id=1481918 is fixed, we can re-enable VOLUMES again.

I've forgot about this ^^, so let's keep it opened.

@pkubatrh
Copy link
Member

pkubatrh commented Mar 1, 2019

Hi @bparees I see the bz (https://bugzilla.redhat.com/show_bug.cgi?id=1481918) is now closed. Has the underlying issue in Openshift Online been dealt with (and we can enable VOLUMEs in Dockerfile again)? Or was the BZ closed because s2i builds no longer fail due to VOLUME commands being commented in the Dockerfile?

@bparees
Copy link
Contributor Author

bparees commented Mar 1, 2019

@pkubatrh i think it was fixed by updating online to use an httpd image that doesn't declare a volume:

https://github.com/sclorg/httpd-container/blob/master/2.4/Dockerfile#L69
0591134

so i guess this can be closed

@pkubatrh
Copy link
Member

pkubatrh commented Mar 5, 2019

@bparees Thanks. That means the underlying issue of online not being able to run s2i builds with images that declare volumes in their Dockerfile is still there, so lets keep this open.

@abhgupta From the older comments I see that you should know where the issue in online is tracked. Can we have some links to the bugs so that we do not have to continue polling once in a while?

@abhgupta
Copy link

IIRC, fixing this required Online to transition to using buildah as S2I builds would also have had to deal with optionally provision emptyDir instead of docker volumes. That transition was not done and currently we are focused on 4.x and not adding new features to Online. So, I don't have a tracker card for getting this addressed in Online.

@phracek
Copy link
Member

phracek commented Feb 19, 2024

@abhgupta @bparees Is this still valid issue even in OpenShift 4? if not please close this issue. Thanks.

@bparees
Copy link
Contributor Author

bparees commented Feb 19, 2024

i think we can close this. i don't believe it's an issue anymore and if it is a new bug can be opened.

@bparees bparees closed this as completed Feb 19, 2024
phracek added a commit to sclorg/nginx-container that referenced this issue Feb 19, 2024
We can safely remove these comments.

Signed-off-by: Petr "Stone" Hracek <[email protected]>
phracek added a commit to sclorg/nginx-container that referenced this issue Feb 21, 2024
* remove .exclude-rhel8 because we have devel-repos
* Add Dockerfile.rhel9 for testing on RHEL9 host
* Add CentOS Stream 8 Dockerfile for Nginx-1.24
* Build and push CentOS Stream 8,9 Nginx-1.24 images
* Fix typo in build-and-push.yml. It is nginx-124-c{8,9}s
* Based on the issue sclorg/httpd-container#30

Signed-off-by: Petr "Stone" Hracek <[email protected]>
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

No branches or pull requests

7 participants