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

To start VS Code, use post-start rather than overriding the entrypoint #21879

Closed
Tracked by #21864 ...
l0rd opened this issue Dec 12, 2022 · 26 comments
Closed
Tracked by #21864 ...

To start VS Code, use post-start rather than overriding the entrypoint #21879

l0rd opened this issue Dec 12, 2022 · 26 comments
Assignees
Labels
area/editor/vscode Issues related to the Code OSS editor of Che kind/enhancement A feature request - must adhere to the feature request template. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system. sprint/current status/release-notes-review-needed Issues that needs to be reviewed by the doc team for the Release Notes wording
Milestone

Comments

@l0rd
Copy link
Contributor

l0rd commented Dec 12, 2022

Is your enhancement related to a problem? Please describe

VS Code che-code-runtime-description component contributes the container command to the tooling container:

(...)
  - schemaVersion: 2.1.0
    metadata:
      name: che-incubator/che-code/insiders
    (...)
    components:
      - name: che-code-runtime-description
        attributes:
          controller.devfile.io/container-contribution: true 
        container:
          (...)
          command:
            - /checode/entrypoint-volume.sh

As a result, the script /checode/entrypoint-volume.sh is executed at workspace startup, but the tooling container entrypoint (for example UDI's) is not.

This is problematic as the tooling container entrypoint may be used to initialise development tools and runtimes.

Describe the solution you'd like

We should use a post-start event rather than the command to start VS Code.

That should be tested though and may require some tweaks to the VS Code startup script (or not).

This is currently VS Code specific but will JetBrains and Theia may be affected too if this feature is implemented in the meantime.

@l0rd l0rd added the kind/enhancement A feature request - must adhere to the feature request template. label Dec 12, 2022
@che-bot che-bot added the status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. label Dec 12, 2022
@l0rd l0rd changed the title To start VS Code, use a post-start command rather than the tooling container entrypoint To start VS Code, use post-start rather than overriding the entrypoint Dec 12, 2022
@ibuziuk ibuziuk added area/editors severity/P1 Has a major impact to usage or development of the system. and removed status/need-triage An issue that needs to be prioritized by the curator responsible for the triage. See https://github. labels Dec 12, 2022
@l0rd l0rd mentioned this issue Dec 13, 2022
82 tasks
@azatsarynnyy azatsarynnyy added this to the 7.59 milestone Dec 13, 2022
@l0rd l0rd modified the milestones: 7.59, 7.60 Jan 5, 2023
@azatsarynnyy azatsarynnyy mentioned this issue Jan 10, 2023
60 tasks
@azatsarynnyy azatsarynnyy mentioned this issue Jan 20, 2023
32 tasks
@RomanNikitenko RomanNikitenko self-assigned this Jan 24, 2023
@azatsarynnyy azatsarynnyy modified the milestones: 7.60, 7.61 Jan 24, 2023
@azatsarynnyy azatsarynnyy added area/editor/vscode Issues related to the Code OSS editor of Che and removed area/editors labels Jan 25, 2023
@l0rd
Copy link
Contributor Author

l0rd commented Jan 26, 2023

@RomanNikitenko I have been testing:

1. With this editor definition that uses args instead of command (link to start it on dogfooding instance) and, after I set secure: false and protocol: https in the main endpoint, I was able to start the workspace

2. With this editor definition that uses a poststart event (link to start it on dogfooding instance) but it's failing with message

Error processing devfile: failed to process postStart event inject-and-start-vscode: container component with name che-code-runtime-description not found

and I noticed that the DWT doesn't have the inject-and-start-vscode component indeed (the dashboard filters it out)

@RomanNikitenko
Copy link
Member

thank you, Mario, for sharing your experience related to the problem!

Unfortunately using args instead of command doesn't work for me.
Please see the video.

args_entrypoint.mp4

image

@RomanNikitenko
Copy link
Member

RomanNikitenko commented Jan 30, 2023

About the second option.

Yes, before our meeting I tried to set the command for the che-code-runtime-description component and faced with the same error.

Then I went forward and tried to set the command for the tools container (just for testing). That time there was no the error container component with name che-code-runtime-description not found, but, as I mentioned on our meeting, the workspace hung on the Waiting for workspace to start step.
After the meeting I noticed that it was because of

image

I continue to investigate the problem...

@RomanNikitenko
Copy link
Member

Update for #21879 (comment)

It looks like the entrypoint is not executed when args is used in the yaml file.
For such use case the workspace has started successfully when I manually run the entrypoint from the terminal:

entrypoint_from_terminal.mp4

@RomanNikitenko
Copy link
Member

depends on #21971

@RomanNikitenko
Copy link
Member

RomanNikitenko commented Jan 31, 2023

Today I've tested on the dogfooding instance creation of the workspace using Create DevWorkspace form of the openshift console.

image

The result is: the error Error processing devfile: failed to process postStart event init-che-code-command: container component with name che-code-runtime-description not found

image

So, I guess, some changes are required on the devworkspace-operator side to have possibility to define a command for the che-code-runtime-description component and use it for the postStart event.

@amisevsk
Copy link
Contributor

amisevsk commented Feb 3, 2023

DWO issue devfile/devworkspace-operator#1029 has an impact on this. I've opened a PR to fix this in upstream DWO: devfile/devworkspace-operator#1033

@amisevsk
Copy link
Contributor

Historically, it was (maybe still is) necessary to create entries in /etc/passwd and /etc/group, as OpenShift clusters run pods with semi-random UIDs. This can result in various things/tools breaking -- see e.g. this old issue

If I remember right when I configured a web-terminal-tooling image to use zsh, it was possible to set the shell by exporting the SHELL env var in the dockerfile, but it has been a while so I could be wrong here.

@l0rd
Copy link
Contributor Author

l0rd commented Feb 15, 2023

If I remember right when I configured a web-terminal-tooling image to use zsh, it was possible to set the shell by exporting the SHELL env var in the dockerfile, but it has been a while so I could be wrong here.

This would be great. The last time I looked I found that cri-o is patching automatically /etc/passwd and /etc/group ( honoring the image User and the env variable HOME) but the shell is always set to /sbin/nologin. Forcing us to do the patch anyway.

@RomanNikitenko
Copy link
Member

RomanNikitenko commented Feb 16, 2023

Update:

  • I prepared a UDI image with a custom entrypoint
  • tested creation of workspace for the VS Code editor without Dashboard usage
  • I can confirm that the UDI entrypoint is not overridden anymore(and it's executed) if I use the following changes for the description of the editor.

At the same time there is still a problem when I create a workspace from the dashboard. So - I guess:

  • logic on the DWO side works correctly
  • we still need some changes on the dashboard side (or required changes for the dashboard didn't come to the dogfooding instance yet...)

I'm going to test similar changes for the JetBrains editor.

@azatsarynnyy azatsarynnyy modified the milestones: 7.61, 7.62 Feb 16, 2023
@RomanNikitenko
Copy link
Member

RomanNikitenko commented Feb 16, 2023

About changes on the dashboard side (please see my previous comment) - I think resolving this issue should fix the problem.

@amisevsk
Copy link
Contributor

amisevsk commented Feb 16, 2023

Looking into using alternative shells, che-code, (current latest version) respects the $SHELL environment variable, even with existing /etc/passwd functionality. I've built the following images that are useful for testing:

  • quay.io/amisevsk/web-terminal-tooling:zsh: Improved WTO tooling image, with zsh installed (other shells are not available in default repos, so just bash and zsh for now)
  • quay.io/amisevsk/che-code:test: Version of che-code init container that does not modify /etc/passwd on start to add an entry for the current user (in my testing, this does not result in an entry being added by cri-o, but that's maybe just the dogfooding cluster)

To test setting SHELL, you can use this DevWorkspace. It should be applied directly to the cluster and assumes Che/Dev Spaces is installed (i.e. not through the dashboard):

kubectl apply -f https://gist.githubusercontent.com/amisevsk/986b628d26a61427fd2f679ddd414ee1/raw/8ee302f8ee4cece8787809e82229b289257c4521/shell-test.devworkspace.yaml

To summarize, as far as I can tell: If $SHELL env var is set in "dev" container, value of $SHELL is used for in-editor terminal. Otherwise, login shell from /etc/passwd is used. If neither are set, fall back to sh.

In addition, $SHELL is ignored by both the OpenShift console terminal tab (in pod details) and the Web Terminal.

@l0rd
Copy link
Contributor Author

l0rd commented Feb 17, 2023

Thank you @amisevsk this is great news. We should get rid of the patches to the UDI /etc/passwd now! I will create a separate issue.

@amisevsk
Copy link
Contributor

amisevsk commented Feb 17, 2023

When I tested it against OpenShift 4.11/4.12, I didn't get the cri-o injected entry in /etc/passwd, so before removing the functionality entirely, it may be worth verifying that we don't run into the other class of issues with not having the entry (configured $HOME directory in containers that do not set $HOME env var, issues with not having a whoami entry, etc.).

However, if UDI images installed alternate shells (e.g. zsh), we could enable switching shells by just setting an environment variable in the devfile, which would be a nice bonus. I'll create an issue to follow up here, as we're getting off-topic for this issue.

Edit: Created issues to move discussion out of this thread:

@RomanNikitenko
Copy link
Member

RomanNikitenko commented Mar 8, 2023

I've faced with a new problem that the plugin-registry is ignoring post-start event for an editor, please see details here.
I'm investigating that problem, will update my PR ASAP...

The issue: #22071

@azatsarynnyy
Copy link
Member

I noticed one small side-effect of adding the command of exec type in the editor definition:

image
It's listed in the Tasks of the devfile type.

The best option for hiding it - filter in Che-Code by the well-known ID init-che-code-command.

@RomanNikitenko WDYT about ^^?

I used the test project provided in the PR eclipse-che/che-plugin-registry#1566.

@RomanNikitenko
Copy link
Member

RomanNikitenko commented Mar 23, 2023

@azatsarynnyy
thank you for reporting the problem!

About filtering by ID - I don't like hardcoding in general:

  • filtering by init-che-code-command will work for our yaml files
  • but we never know what ID user defines in the .che/che-editor.yaml file
  • even within our devfiles/yaml files there were differences, like: all editors had a command with init-container-command ID, but one of them had a command with init-remote-runtime-injector ID: eclipse-che/che-plugin-registry@619e7e3

From architecture point of view, I would say, we should have some mechanism/approach how to filter out devfile commands which are not expected to be converted to VS Code tasks.

In this case the command was defined in the .che/che-editor.yaml file, not in the devfile.yaml of the project. My question is: is it expected to convert commands from .che/che-editor.yaml file to VS Code tasks?

@azatsarynnyy
Copy link
Member

azatsarynnyy commented Mar 24, 2023

@RomanNikitenko
Theoretically, the user may want to provide an exec command with its custom VS Code editor Devfile. However, I have never seen that in practice. Moreover, I don't see any value in such an approach. So, I'd hide all such editor-specific exec commands.

We can filter them out by the controller.devfile.io/imported-by: editor attribute.
See my suggestion in the PR che-incubator/che-code#192.
Once the bot builds the images, we can test it on our dogfooding cluster where the plugin registry already contains your latest changes.

@azatsarynnyy
Copy link
Member

The editor-contributed Devfile Commands are filtered out now - che-incubator/che-code#192

@amisevsk
Copy link
Contributor

I'm concerned the approach to filtering editor-contributed commands is potentially fragile, as the editor name comes from the devfile registry instead of some other source (as far as I can tell, at least). I left a comment on the PR here

@azatsarynnyy
Copy link
Member

Thanks @amisevsk! I implemented your suggestion in che-incubator/che-code#194. Now, it doesn't depend on the controller.devfile.io/imported-by attribute's value.

@l0rd l0rd added new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes status/release-notes-review-needed Issues that needs to be reviewed by the doc team for the Release Notes wording labels Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/editor/vscode Issues related to the Code OSS editor of Che kind/enhancement A feature request - must adhere to the feature request template. new&noteworthy For new and/or noteworthy issues that deserve a blog post, new docs, or emphasis in release notes severity/P1 Has a major impact to usage or development of the system. sprint/current status/release-notes-review-needed Issues that needs to be reviewed by the doc team for the Release Notes wording
Projects
None yet
Development

No branches or pull requests

8 participants