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

feat: Support running VS Code in ubi9-based containers #324

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

RomanNikitenko
Copy link
Contributor

@RomanNikitenko RomanNikitenko commented Jan 17, 2024

What does this PR do?

Use:

  • Node.js provided by registry.access.redhat.com/ubi9/nodejs-18
  • libbrotli library from the same image
  • LD_LIBRARY_PATH env variable to define additional shared libs place

to run VS Code in a user's ubi9-based container.

What issues does this PR fix?

eclipse-che/che#21778

How to test this PR?

Image Workspace starts Devfile registry link Link to start a workspace
registry.access.redhat.com/ubi8 + - click here
registry.access.redhat.com/ubi9 + - click here
registry.access.redhat.com/ubi9/ubi:9.3-1476 + - click here
registry.access.redhat.com/ubi9/ubi-minimal:9.3-1475 + - click here
registry.access.redhat.com/ubi9/podman:9.3-8 + - click here
registry.access.redhat.com/ubi9/perl-532:1-121 + - click here
registry.access.redhat.com/ubi9/php-81:1-43 + - click here
registry.access.redhat.com/ubi9/ruby-30:1-138 + - click here
registry.access.redhat.com/ubi9/openjdk-21-runtime + - click here
registry.access.redhat.com/ubi9/nodejs-16:1-143 + - click here
registry.access.redhat.com/ubi9/nodejs-18:1-84 + - click here
registry.access.redhat.com/ubi9/nodejs-20:1-20 + - click here
registry.access.redhat.com/ubi9/openjdk-11:1.17-1.1705602311 + - click here
registry.access.redhat.com/ubi9/openjdk-17:1.17-1 + Basic Spring Boot click here
registry.access.redhat.com/ubi9/go-toolset:1.19.13-4.1697647145 + Go Runtime click here
registry.access.redhat.com/ubi8/nodejs-16:latest + Basic Node.js click here
registry.access.redhat.com/ubi9/python-39:1-161 + Python-Django click here
registry.access.redhat.com/ubi9/python-39:1-161 + Basic Python click here
registry.access.redhat.com/ubi8/openjdk-17:1.18-2 + Quarkus Java click here
quay.io/eclipse/che-java11-maven:7.37.2 No
the cause of the problem
Vert.x Java click here
quay.io/jaegertracing/all-in-one:1.50 No
the cause of the problem
WildFly Bootable Jar click here
docker.io/alpine:3.19.0 No - click here
docker.io/node:18.18.2-alpine3.18 + - click here

Copy link

github-actions bot commented Jan 17, 2024

Click here to review and test in web IDE: Contribute

Copy link

Pull Request Dev image published:
👉 quay.io/che-incubator-pull-requests/che-code-dev:pr-324-dev-amd64

1 similar comment
Copy link

Pull Request Dev image published:
👉 quay.io/che-incubator-pull-requests/che-code-dev:pr-324-dev-amd64

Copy link

Pull Request Che-Code image published:
👉 quay.io/che-incubator-pull-requests/che-code:pr-324-amd64

Copy link

Pull Request Dev image published:
👉 quay.io/che-incubator-pull-requests/che-code-dev:pr-324-dev-amd64

Copy link

Pull Request Che-Code image published:
👉 quay.io/che-incubator-pull-requests/che-code:pr-324-amd64

@RomanNikitenko RomanNikitenko changed the title Ubi9 test feat: Support running VS Code in ubi9-based containers Jan 23, 2024
Copy link
Contributor

@l0rd l0rd left a comment

Choose a reason for hiding this comment

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

I have tested this PR vscode image on workspaces using alpine, busybox, docker, fedora, golang, openjdk, ubi8, ubi9 and ubuntu base images and if worked on most of them (docker.io/alpine:3.19.0, docker.io/busybox:1.36.1, docker.io/docker:24.0.6-dind are still failing). This is an important improvement compared to what we get trying current vs code image.

Some improvments (for later PRs) could be:

  • support recent alpine with new version of musl (that should fix quay.io/jaegertracing/all-in-one:1.50 too)
  • support images that don't have neither openssl nor rpm (looking for /usr/lib/libcrypto* for example)
  • libc-ubi8 and libc-ubi9 checode should be renamed libc-v1 and libc-v3 as they run on any image using libc, not just on UBI.
  • default che-code should be libc-ubi9/libc-v3 instead of the libc-ubi8 as new images will all have the new version of libc.

@RomanNikitenko
Copy link
Contributor Author

@l0rd
thank you very much for testing!

I would like to clarify some points for myself:

support recent alpine with new version of musl (that should fix quay.io/jaegertracing/all-in-one:1.50 too)

It's possible to apply the improvement for the che-code, but there is a problem for the downstream.
AFAIK - at the moment we don't have alpine-based assembly for the downstream as there is no base image that we can use for the productization process.
So, there is a problem for images, like:

  • quay.io/jaegertracing/all-in-one:1.50
  • docker.io/alpine:3.19.0
  • docker.io/docker:24.0.6-dind

=====================

default che-code should be libc-ubi9/libc-v3 instead of the libc-ubi8 as new images will all have the new version of libc.

I'm afraid I didn't get what do you mean here:

  • as far as I understand the editor-image contains both: ubi8 and ubi9-based assemblies
  • at starting a workspace we just use one of them - it depends on the user's container for running
  • so - what default che-code should be libc-ubi9/libc-v3 means

=====================

libc-ubi8 and libc-ubi9 checode should be renamed libc-v1 and libc-v3 as they run on any image using libc, not just on UBI.

I'm not sure about they run on any image using libc.
For example, we build ubi8- based assembly that relies on (GNU libc) 2.28, but user's container contains (GNU libc) 2.17.
or
Node.js relies on /lib64/libstdc++.so.6, the file exists in the user's container, but user gets something like: /lib64/libstdc++.so.6: version GLIBCXX_3.4.20 not found - I'm not sure that LD_LIBRARY_PATH can resolve this problem.

So, I consider ubi8 and ubi9-based assemblies as a set of libraries that are required for running Node.js and some VS Code dependencies in a user's ubi8/ubi9-based container.
Like:

  • we build VS Code in the ubi8 container - then it should work in another ubi8-based container as both of them have similar shared libs(version of libs)

@l0rd
Copy link
Contributor

l0rd commented Jan 24, 2024

It's possible to apply the improvement for the che-code, but there is a problem for the downstream.

Right. There is no request to support alpine downstream. It's an upstream only issue.

it depends on the user's container for running. so - what default che-code should be libc-ubi9/libc-v3 means?

What I mean is the default when the openssl version is unknown. When the bash script fails to figure out the openssl version (that happens on the ubuntu and busybox images for example) then it currently defaults to the ubi8 assembly. If we change that, and use ubi9 in case of unknown openssl version, any recent ubuntu-based image will work out of the box.

we build VS Code in the ubi8 container - then it should work in another ubi8-based container as both of them have similar shared libs(version of libs)

Does it matter if the assembly has been built in a ubi8, a fedora or an ubuntu container? che-code built on ubi8 works on images that have openssl-v1 (e.g. openjdk) and che-code built on ubi9 works on any image with openssl-v3 (ubuntu, golang, fedora).

@RomanNikitenko
Copy link
Contributor Author

What I mean is the default when the openssl version is unknown. When the bash script fails to figure out the openssl version (that happens on the ubuntu and busybox images for example) then it currently defaults to the ubi8 assembly. If we change that, and use ubi9 in case of unknown openssl version, any recent ubuntu-based image will work out of the box.

Maybe it's true for ubuntu, but not for the mentioned above busybox.
I just tested docker.io/busybox:1.36.1 for both: ubi8 and ubi9 assembly.

There is the same error:
image

image

So, for the docker.io/busybox:1.36.1 it doesn't matter - ubi8 or ubi9 - we should provide not only brotli, but also libz.
All libs required by Node.js must be present in a user's container:
image

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Jan 24, 2024

Does it matter if the assembly has been built in a ubi8, a fedora or an ubuntu container? che-code built on ubi8 works on images that have openssl-v1 (e.g. openjdk) and che-code built on ubi9 works on any image with openssl-v3 (ubuntu, golang, fedora).

I was not able to run Node.js in the quay.io/wildfly/wildfly-centos7:26.1 using both: ubi8 and ubi9-based assemblies:

Screenshot 2024-01-24 at 11 18 51

So, I'm not sure we can guarantee that Che-Code can start in any user's container.
It really depends on set of shared libs and their versions in a user's container.
That's why I would start from support for ubi8 and ubi9-based containers.

@RomanNikitenko
Copy link
Contributor Author

RomanNikitenko commented Jan 24, 2024

@l0rd thank you for the feedback! It's really helpful!

@l0rd @ibuziuk
So, does it make sense to improve the following within the current PR:

  • add libz related files to both: ubi8 and ubi9 assemblies
  • add libbrotli files to ubi8 assembly (the same way as it's done for ubi9 in the current PR)
  • use ubi9 based assembly when the openssl version is unknown

The changes look simple, after applying them I could test it for my table in the PR description.
But I'm not aware what set of images was tested by Mario for the current PR - it makes sense to test them again after updating my PR...

@ibuziuk
Copy link
Contributor

ibuziuk commented Jan 24, 2024

That's why I would start from support for ubi8 and ubi9-based containers.

I believe it makes sense to start with supporting ubi8 and ubi 9 and merge this PR, but we need to create follow up issue -
for cases that @l0rd described in #324 (review)

The ultimate goal / story - As a User I want to specify any popular image in the devfile and use it in my CDE

Also, please create separate issues for 2 devfile.io devfiles that are currently failing. In the next sprint it would be great to finalize the following epic - eclipse-che/che#20251

But I'm not aware what set of images was tested by Mario for the current PR - it makes sense to test them again after updating my PR...

I think the scripts for testing various images are part of https://github.com/l0rd/cloud-dev-images

@l0rd
Copy link
Contributor

l0rd commented Jan 24, 2024

@RomanNikitenko I approved this PR so from my point of view that's ok to merge as it is.

At some point we should support ubuntu-based images (that looks easy, it's just changing the script to use the opensslv3 assembly rather than the opensslv1 when the openssl version is unknown) and alpine-based images (that seams a little bit harder: introduce a new musl assembly that supports openssl v3).

busybox is NOT an ubuntu-based or alpine-based image and it's not a priority at all so you can ignore it.

Copy link

Pull Request Dev image published:
👉 quay.io/che-incubator-pull-requests/che-code-dev:pr-324-dev-amd64

Copy link

Pull Request Che-Code image published:
👉 quay.io/che-incubator-pull-requests/che-code:pr-324-amd64

@RomanNikitenko RomanNikitenko marked this pull request as ready for review January 24, 2024 13:50
Signed-off-by: Roman Nikitenko <rnikiten@redhat.com>
Copy link

github-actions bot commented Feb 9, 2024

Pull Request Dev image published:
👉 quay.io/che-incubator-pull-requests/che-code-dev:pr-324-dev-amd64

Copy link

github-actions bot commented Feb 9, 2024

Pull Request Che-Code image published:
👉 quay.io/che-incubator-pull-requests/che-code:pr-324-amd64

@RomanNikitenko
Copy link
Contributor Author

image

@azatsarynnyy
it looks like I'm not able to merge the current PR due to Required status of the build (libc, amd64) check.
so, could you update the repo settings?

@azatsarynnyy
Copy link
Member

@azatsarynnyy it looks like I'm not able to merge the current PR due to Required status of the build (libc, amd64) check. so, could you update the repo settings?

@RomanNikitenko done

@RomanNikitenko RomanNikitenko merged commit 64ac009 into main Feb 13, 2024
7 checks passed
@RomanNikitenko RomanNikitenko deleted the ubi9_test branch February 13, 2024 12:34
@devstudio-release
Copy link

Build 3.13 :: code_3.x/1285: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.12 :: code_3.12/25: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

This was referenced Feb 13, 2024
@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.12 :: get-sources-rhpkg-container-build_3.12/104: FAILURE

code : 3.12 :: Failed in 58907335 : code-rhel8-3.12-61
FAILURE: 1925588 - osbs.osbs_http - DEBUG - cleaning up==> 58907335:DEFAULT:osbs-client.log: <==; copied to quay

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.13 :: update-digests_3.x/5869: UNSTABLE

No new images detected: nothing to do!

@devstudio-release
Copy link

Build 3.13 :: code_3.x/1286: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

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.

5 participants