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

Create and use jetbrains' workspace images #5642

Closed
Tracked by #5831
atduarte opened this issue Sep 10, 2021 · 15 comments
Closed
Tracked by #5831

Create and use jetbrains' workspace images #5642

atduarte opened this issue Sep 10, 2021 · 15 comments

Comments

@atduarte
Copy link
Contributor

atduarte commented Sep 10, 2021

As defined in #5641, users will be able to select Jetbrains IDEs as their preferred IDE. When that is the case, starting (a new) workspace should make Gitpod install and run the appropriate Jetbrains IDE backend and lead the user to a page similar to the following:

The page should automatically redirect to the Jetbrains link (for now, a code with me link). And instead of "Jetbrains IDE" the button should mention the actual IDE name (e.g. Pycharm).

Questions & answers

How can we detect if the IDE backend is ready and fetch the link?

One option would be to monitor the stdout of the process, but the recommended option is to either run the status method (see internal discussion) or do an HTTP call (see here).

Is the VS code web supposed to be running simultaneously in the same workspace?

Yes.

@akosyakov
Copy link
Member

/schedule

@roboquat
Copy link
Contributor

@akosyakov: Issue scheduled in the IDE team (WIP: 0)

In response to this:

/schedule

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/test-infra repository.

@corneliusludmann
Copy link
Contributor

corneliusludmann commented Sep 23, 2021

Based on the discussion with @akosyakov, @atduarte, and @csweichel I would suppose the following solution.

Objectives

The goal is to allow us to add a second “IDE” to the workspace image. This second “IDE” is actually a backend service that allows to connect an external/remote IDE to the running workspace (e.g., a desktop IDE like IntellJ). If the remote IDE cannot connect for some reason or the user would like to start the web IDE instead, we still have a web IDE backend (e.g., VSCode) running in the workspace. The user should be able to open this web IDE with a click on a button (see screenshot above).

I personally prefer the term “remote IDE” over “desktop IDE” for the second IDE because technically, it's not limited to desktop IDEs but allows all kinds of IDEs that are not delivered by us to connect to the workspace, e.g. another web IDE hosted somewhere can connect to this workspace as well. However, if you are not convinced, we still can change it to use the term “desktop IDE” instead.

The implementation should be backward as well as forward compatible.

Implementation

Registry Facade

The remote IDE process that allows other (remote) IDEs to connect to this workspace should be implemented as an own image that is used in the registry facade as additional layers. For this we need to add the remote IDE image ref to the ImageSpec:

message ImageSpec {
// base_ref points to an image in another registry
string base_ref = 1;
// ide_ref points to an image denoting the IDE to use
string ide_ref = 2;
// content_layer describe the last few layers which provide the workspace's content
repeated ContentLayer content_layer = 3;
}

To be compatible with different versions, I would suggest that we don't touch the ide_ref attribute but just add another optional remote_ide_ref attribute like this:

 message ImageSpec { 
     // base_ref points to an image in another registry 
     string base_ref = 1; 
     // ide_ref points to an image denoting the IDE to use 
     string ide_ref = 2; 
     // content_layer describe the last few layers which provide the workspace's content 
     repeated ContentLayer content_layer = 3;
     // remote_ide_ref points to an image denoting the remote IDE to use 
     string remote_ide_ref = 4; 
 } 

The registry-facade needs to consider this as an additional layer source (and needs to make sure that it can handle a missing value if this workspace does not have a remote image):

ideRefSource := func(s *api.ImageSpec) (ref string, err error) {
return s.IdeRef, nil
}
ideLayerSource, err := NewSpecMappedImageSource(newResolver, ideRefSource)
if err != nil {
return nil, err
}
layerSources = append(layerSources, ideLayerSource)

Server

The additional remote image ref needs to be added to the WorkspaceInstanceConfiguration:

// WorkspaceInstanceConfiguration contains all per-instance configuration
export interface WorkspaceInstanceConfiguration {
// theiaVersion is the version of Theia this workspace instance uses
// @deprected: replaced with the ideImage field
theiaVersion?: string;
// feature flags are the lowercase feature-flag names as passed to ws-manager
featureFlags?: NamedWorkspaceFeatureFlag[];
// ideImage is the ref of the IDE image this instance uses.
ideImage: string;
}

Again, we can just add another attribute for the remote IDE image and leave the ide_image attribute as is.

and used here:

configuration.ideImage = mappedImage;

and here:

const spec = new StartWorkspaceSpec();
spec.setCheckoutLocation(checkoutLocation!);
await addExtensionsToEnvvarPromise;
await createGitpodTokenPromise;
spec.setEnvvarsList(envvars);
spec.setGit(this.createGitSpec(workspace, user));
spec.setPortsList(ports);
spec.setInitializer((await initializerPromise).initializer);
spec.setIdeImage(ideImage);
spec.setWorkspaceImage(instance.workspaceImage);
spec.setWorkspaceLocation(workspace.config.workspaceLocation || spec.getCheckoutLocation());
spec.setFeatureFlagsList(this.toWorkspaceFeatureFlags(featureFlags));
if (workspace.type === 'regular') {
spec.setTimeout(await userTimeoutPromise);
}
spec.setAdmission(admissionLevel);
return spec;

ws-manager

The additional remote image ref needs to be added to the WorkspaceSpec:

message WorkspaceSpec {
// workspace_image is the name of the Docker image this workspace runs
string workspace_image = 1;
// ide_image is the name of the Docker image used as IDE
string ide_image = 2;
// headless marks this workspace a headless one - headless workspaces are not intended for users but for automation
bool headless = 3;
// URL is the external URL of the workspace
string url = 4;
// exposed_ports lists all ports which this workspace has exposed to the outside world
repeated PortSpec exposed_ports = 5;
// workspace type denotes what kind of workspace this is, e.g. if it's user-facing, prebuilding content or probing the service
WorkspaceType type = 6;
// The intervals in which a heartbeat must be received for the workspace not to time out
string timeout = 7;
}

Again, we can just add another attribute for the remote IDE image and leave the ide_image attribute as is.

The remote IDE image needs to be set here for the registry facade:

spec := regapi.ImageSpec{
BaseRef: startContext.Request.Spec.WorkspaceImage,
IdeRef: startContext.Request.Spec.IdeImage,
}

And in the status.go:

if ispec, ok := wso.Pod.Annotations[workspaceImageSpecAnnotation]; ok {
spec, err := regapi.ImageSpecFromBase64(ispec)
if err != nil {
return nil, xerrors.Errorf("invalid iamge spec: %w", err)
}
wsImage = spec.BaseRef
ideImage = spec.IdeRef
}

Spec: &api.WorkspaceSpec{
Headless: wso.IsWorkspaceHeadless(),
WorkspaceImage: wsImage,
IdeImage: ideImage,
Url: wsurl,
Type: tpe,
Timeout: timeout,
},

Supervisor

The supervisor config needs to be extended so that we can add an additional IDEConfig for the remote IDE here:

type Config struct {
StaticConfig
IDEConfig
WorkspaceConfig
}

It makes sense to re-use the IDEConfig struct for the remote IDE config as well:

// IDEConfig is the IDE specific configuration
type IDEConfig struct {
// Entrypoint is the command that gets executed by supervisor to start
// the IDE process. If this command exits, supervisor will start it again.
// If this command exits right after it was started with a non-zero exit
// code the workspace is stopped.
Entrypoint string `json:"entrypoint"`
// LogRateLimit can be used to limit the log output of the IDE process.
// Any output that exceeds this limit is silently dropped.
// Expressed in kb/sec. Can be overriden by the workspace config (smallest value wins).
IDELogRateLimit int `json:"logRateLimit"`
// ReadinessProbe configures the probe used to serve the IDE status
ReadinessProbe struct {
// Type determines the type of readiness probe we'll use.
// Defaults to process.
Type ReadinessProbeType `json:"type"`
// HTTPProbe configures the HTTP readiness probe.
HTTPProbe struct {
// Path is the path to make requests to. Defaults to "/"
Path string `json:"path"`
} `json:"http"`
} `json:"readinessProbe"`
}

However, since the config is a struct that merges StaticConfig, IDEConfig, and WorkspaceConfig into one single struct we cannot just add another IDEConfig to this struct. We have 2 options:

  1. Leave IDEConfig as is and add remote IDE as an attribute like this:
type Config struct {
	StaticConfig
	IDEConfig
	RemoteIDE *IDEConfig
	WorkspaceConfig
}

That has the advantage that the references to IDEConfig do not need to be touched and the disadvantage that RemoteIDE does not fit well into this struct and has a special role.

  1. Move IDEConfig to an attribute like this:
type Config struct {
	StaticConfig
	IDE IDEConfig
	RemoteIDE *IDEConfig
	WorkspaceConfig
}

With this change, we need to change all references to the IDEConfig, e.g. cfg.Entrypointcfg.IDE.Entrypoint. The 2 big advantages are:
a) It is more clear that it is the IDE entrypoint we are talking about.
b) Re-using functions would be easier: like pepareIDELaunch(cfg *Config) *exec.Cmd would change to pepareIDELaunch(cfg *IDEConfig) *exec.Cmd which would it make easier to re-use them for the remote IDE configuration.

Thus, I think that option 2 would be preferable.

Besides this, there are a lot of changes in the supervisor that have to be made, e.g.:

  • launch the entrypoint command for the remote IDE in supervisor.go
  • extend the status endpoint to respect the remote IDE as well (when given)
  • support a ready check that looks for a specific string in the logs to indicate the IDE is ready
  • introduce an endpoint in the supervisor API that returns the join URL for the remote IDE that will be shown in the frontend
  • show a frontend page with the join link

Testing

We still need to make a plan on how to test this.

@csweichel
Copy link
Contributor

@corneliusludmann Thank you for the detailed analysis and write-up. A few comments:

  • remote_ide_ref in registry facade: I originally thought we could combine this with the content layer, but the ordering of things would break. It makes sense to maintain this as a separate field. To make the SpecMappedImage source handle an empty reference, you could return an empty ImageLayerSource here when the ref is empty.
  • for the ws-manager-api I'd rather make the ide_image field composite as outlined below. The reason is that we hope to move the image-builder to the workspace side in not too far a future, at which point the workspace_image field will become composite as well.
    message WorkspaceSpec { 
        // IDEImage configures the IDE images a workspace will use
        message IDEImage struct {
            // web_ref is a reference to an OCI image used for serving the web-based IDE
            string web_ref = 1;
            // remote_ref is an optional reference to an OCI image used for serving remote IDEs
            string remote_ref = 2;
        }
    
        // workspace_image is the name of the Docker image this workspace runs 
        string workspace_image = 1; 
    
        // deprecated_ide_image is a field present for backwards compatibility and the same
        // as IDEImage.web_ref. If both fields are present, IDEImage.web_ref takes precedence.
        string deprecated_ide_image = 2;
    
        // ide_image is the name of the Docker image used as IDE 
        IDEImage ide_image = 8; 
    
        ...
    } 
  • regarding the supervisor config, personally I'd favour option 2 because it's more explicit.
  • it would be great if you could detail the change to the IDE status endpoint, considering it's consumed by ws-manager as well
  • we should break this issue into smaller issues that we can work on bit by bit

@corneliusludmann
Copy link
Contributor

One option would be to monitor the stdout of the process, but the recommended option is to run the status method (see internal discussion).

@atduarte Could you add a link to the internal discussion? How can I call the status method?

@atduarte
Copy link
Contributor Author

atduarte commented Sep 24, 2021

@corneliusludmann done.

remote IDE sounds a bit confusing as the "remote" concept depends on context (the origin). For example, the actual "remote IDE" uses the "local app" to establish the connection—both run in same device and one is called remote and the other local. I suggest using local IDE.

To be compatible with different versions, I would suggest that we don't touch the ide_ref attribute

Unless something weird is being done, values in protobuf messages are identified by their index and the name of the attribute is not present. As such compatibility should not be affected, right?

@akosyakov
Copy link
Member

akosyakov commented Sep 27, 2021

I think we should rename remote to desktop. Both web and desktop IDEs are remote meaning that they are running backends in Gitpod workspaces. The only difference is a ui kind: web meaning using browser, and desktop meaning some native desktop app.

@corneliusludmann
Copy link
Contributor

Regarding the term that we use for the second kind of IDE images

remote IDE sounds a bit confusing as the "remote" concept depends on context (the origin). For example, the actual "remote IDE" uses the "local app" to establish the connection—both run in same device and one is called remote and the other local. I suggest using local IDE.

Indeed. I personally find “local IDE” confusing for the same reason—it's a matter of the origin. However, because we already have the “local app” it would make sense to call it “local IDE” as well. Then we are aligned with the terms. 👍

Another option would call it “external IDE”. Perhaps that would be even more accurate?

I think we should rename remote to dashboard.

You mean rename remote to desktop (not dashboard), right?

At the end of the day, it's just a name and I don't want to start an unnecessary discussion here. If you're more comfortable with “desktop IDE”, I can live with that. However, please let me explain why I don't prefer this.

Both web and desktop IDEs are remote meaning that they are running backends in Gitpod workspaces. The only difference is a ui kind: web meaning using browser, and desktop meaning some native desktop app.

In my opinion, that's not 100 % true. The actual difference between these 2 IDEs is that we serve the first one ourselves (the “web IDE”) and the second one is served by someone else. Currently, the second kind of IDEs is always a desktop IDE but technically JetBrains could also serve their IDEs as a web IDE on their servers and connect to our workspaces. And with Electron & Co, the boundaries between web and desktop are blurred anyway. That's why I think the term “desktop IDE” is too narrow.

In fact, the “IDE image” for the desktop/remote/local IDE (however we call this) is actually not what I would call an “IDE” it's more like a gateway that allows external IDEs to connect to our workspace. IMO, it would be even more accurate if we call it something like external_ide_gateway instead.

@corneliusludmann
Copy link
Contributor

To be compatible with different versions, I would suggest that we don't touch the ide_ref attribute

Unless something weird is being done, values in protobuf messages are identified by their index and the name of the attribute is not present. As such compatibility should not be affected, right?

I mean the change from

message FooBar { 
  	ide_ref string = 1;
}

to

message FooBar { 
	message IDE {
		ide_ref string = 1;
		remote_ide_ref string = 2;
	}
	ide_refs IDE = 2;
}

would be incompatible or at least need to be handled. Just renaming is not a bug problem (but you still need to change the code occurrences that refer to the attributes of the generated code).

@akosyakov
Copy link
Member

akosyakov commented Sep 27, 2021

remote IDE sounds a bit confusing as the "remote" concept depends on context (the origin). For example, the actual "remote IDE" uses the "local app" to establish the connection—both run in same device and one is called remote and the other local. I suggest using local IDE.

From user perspective:

  • Remote IDE means that it is running on a remote machine, like anything remote. We don't generally speak though about remote IDEs because there are 2 applications at least: client and backend. And only backend is always running in Gitpod workspace. On user machine we only run IDE frontend. When we speak local IDE, we mean that a user uses some local IDE app as a frontend to IDE running on remote machine, but not browser. When we speak web IDE, we mean that a user uses browser as a frontend.
  • The local app is running locally to establish transport to a remote Gitpod workspace and allow integrations with localhost.

I don't like remote because ide_ref is also remote. You can actually use local VS Code to connect to out Gitpod Code Server. Local is better. It seems to be used interchangeable with desktop by us. Although other products using VS Code Desktop and so on, they don't call it local VS Code. I liked desktop because it was natural how we will explain to users, and as soon us JetBrains has good web client we don't need second ide ref anymore.

@corneliusludmann
Copy link
Contributor

corneliusludmann commented Sep 27, 2021

Just to make it clear: I don't have any problems with speaking about Desktop IDEs to our users in our docs etc. I just think that we should call it in the code more open and technically accurate. Technically, there is no reason that an external IDE that connects to the workspace is a Desktop IDE. I just want to prevent that we call it desktop_ide everywhere in our code and in the future we use this function e.g. to let other cloud IDE frontends connect to our workspace and everyone is asking “why is it called ‘desktop_ide’?” and we say: “Well, it's for historical reasons. As we developed this, we had only Desktop IDEs that have connected to our workspaces and nobody thought about it that we can technically let other kinds of IDE frontends with this feature connect to our workspaces.”

I see that remote can be confusing. local would be probably better. Currently, I would even prefer external as I proposed above.

But hey, when I'm the only one who thinks so and cannot convince you with my point of view, then let's stick with desktop_ide. It's just a name in the code … 😆

@csweichel
Copy link
Contributor

csweichel commented Sep 28, 2021

message FooBar { 
	message IDE {
		ide_ref string = 1;
		remote_ide_ref string = 2;
	}
	ide_ref IDE = 2;
}

would indeed be incompatible with the current API. However,

message WorkspaceSpec { 
    // IDEImage configures the IDE images a workspace will use
    message IDEImage struct {
        // web_ref is a reference to an OCI image used for serving the web-based IDE
        string web_ref = 1;
        // remote_ref is an optional reference to an OCI image used for serving remote IDEs
        string remote_ref = 2;
    }

    // workspace_image is the name of the Docker image this workspace runs 
    string workspace_image = 1; 

    // deprecated_ide_image is a field present for backwards compatibility and the same
    // as IDEImage.web_ref. If both fields are present, IDEImage.web_ref takes precedence.
    string deprecated_ide_image = 2;

    // ide_image is the name of the Docker image used as IDE 
    IDEImage ide_image = 8; 

    ...
} 

is compatible because the new message uses a new index, and the field number 2 remains a string.

@csweichel
Copy link
Contributor

Also +1 for desktop rather than remote IDE

@corneliusludmann
Copy link
Contributor

Also +1 for desktop rather than remote IDE

Since there were other alternatives like local or external IDE, I read this as you think the term desktop IDE is still the most appropriate despite that we probably enable other non-desktop IDEs with this feature, right? 🤣 Probably this is simply the term that most people here can relate to. 👍 Then let's stick with desktop_ide! 🚀

is compatible because the new message uses a new index, and the field number 2 remains a string.

Sure. We are on the same page here. Just wanted to stress that this needs code changes to reflect the changed field (what makes it some kind of incompatible on another layer).

  • for the ws-manager-api I'd rather make the ide_image field composite as outlined below. The reason is that we hope to move the image-builder to the workspace side in not too far a future, at which point the workspace_image field will become composite as well.

While we're on this topic: I actually don't see the benefit of making this field composite. It would produce additional code to make this change backwards compatible that we need for a very short transition phase only. But I read from it that it has advantages for the move of the image-builder to the workspace side when we make it composite, right?

@corneliusludmann
Copy link
Contributor

corneliusludmann commented Sep 28, 2021

Here is a first draft: https://github.com/gitpod-io/gitpod/compare/clu/intellij-ide

Still with a lot of rough edges and not everything has been considered but working prototype:

Screenshots

User preferences:

Peek 2021-09-28 11-53

Workspace start:

Peek 2021-09-28 11-59


Smaller Issues / PRs

we should break this issue into smaller issues that we can work on bit by bit

💯 Having smaller issues / PRs is always a good thing! However, I don't know yet how to divide this in a meaningful way to have changes that are viable on their own. Especially the API changes are all somehow connected. I'll think about it some more …

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants