Workspace creation fails if a workspace with the same name already exist

[sunix]

Description

With latest chectl, creating a workspace now using the new devfile api, but it seems that this does not support the creation of multiple workspace with the same name. Previously that was working and suffixing the workspace name with _1, _2, etc ...

Reproduction Steps

Try to run chectl workspace:start twice:

 $  chectl workspace:start -f https://raw.githubusercontent.com/eclipse/che-theia/readme-contribute/devfiles/che-theia-all.devfile.yaml
  ✔ Retrieving Che Server URL...http://che-che.192.168.39.13.nip.io
  ✔ Verify if Che server is running
  ✔ Create workspace from Devfile https://raw.githubusercontent.com/eclipse/che-theia/readme-contribute/devfiles/che-theia-all.de
vfile.yaml

Workspace IDE URL:
http://che-che.192.168.39.13.nip.io/dashboard/#/ide/che/che-theia-all

$ chectl workspace:start -f https://raw.githubusercontent.com/eclipse/che-theia/readme-contribute/devfiles/che-theia-all.devfile.yaml
  ✔ Retrieving Che Server URL...http://che-che.192.168.39.13.nip.io
  ✔ Verify if Che server is running
  ✖ Create workspace from Devfile https://raw.githubusercontent.com/eclipse/che-theia/readme-contribute/devfiles/che-theia-all.de
vfile.yaml
    → E_CHE_API_UNKNOWN_ERROR - Endpoint: http://che-che.192.168.39.13.nip.io/api/workspace/devfile -Status: 409
Error: E_CHE_API_UNKNOWN_ERROR - Endpoint: http://che-che.192.168.39.13.nip.io/api/workspace/devfile -Status: 409
    at CheHelper.getCheApiError (/snapshot/chectl/lib/api/che.js:0:0)
    at CheHelper.<anonymous> (/snapshot/chectl/lib/api/che.js:0:0)
    at Generator.throw (<anonymous>)
    at rejected (/snapshot/chectl/node_modules/tslib/tslib.js:108:69)

OS and version:

Diagnostics:

[benoitf]

it was handled by the server side API

[sunix]

maybe the same issue: #13683

[l0rd]

@skabashnyuk please review if we should close this one as duplicate of #13683 or if makes sense to keep both open. Anyway I have included both in milestone 7.1.0.

[skabashnyuk]

@l0rd I would like to keep them separate since they have to be fixed in different places

• Unable to accept factory with workspace, which name would collide with existing workspace name #13683 factory flow.
• Workspace creation fails if a workspace with the same name already exist #13794 chectl

[benoitf]

@skabashnyuk if it's handled server-side then there is only one place to fix no ?

[skabashnyuk]

Yeh. However, that make our API inconsistent. For example, when you asking to create user for example with name "sergii" you don't expect to have it with name "sergii_77" in case if "sergii" already exists. Previous behavior was more oriented for the factory use case where it makes sense.

[benoitf]

I don't see why it's inconsistent. It depends on the nature of the resource of we want to create and the relationship order, 1..1, 1..n, etc.

If I want to create a workspace from a devfile, I expect that it will always create a new workspace because I can have several workspaces from the same devfile while when I create a user from an email address it's 1..1 so I can't create more if already taken.

[skabashnyuk]

I see your point. That looks like a point of enhancement. However, I didn't see that in our API before and I'm not sure it's a good time to a discussion about the pros and cons before GA. We may think about that in 7.x

[sunix]

There is no debate about the fact that it is a regression for the end user flow I want to create a workspace. It was working before and now it is not working now.

Debating can happen on where the fix is to be done and to me, it makes sense that is would be done server side as it has always been done in the past.

[metlos]

If I want to create a workspace from a devfile, I expect that it will always create a new workspace because I can have several workspaces from the same devfile while when I create a user from an email address it's 1..1 so I can't create more if already taken.

Could you explain why as a user you would like to have 2 workspaces with the same project?

[metlos]

here is no debate about the fact that it is a regression for the end user flow I want to create a workspace. It was working before and now it is not working now.

Not true. It was working for factories, not workspace creation per se. The beta endpoint for devfile adopted the approach of the factory but later we realized our mistake and adopted the approach of workspace. You could never create 2 workspaces with the same name.

I am more than open to change that logic again once someone proposes an important usecase where creating 2 workspaces from a single devfile is unavoidable (by a single user), though I don't see such usecase myself.

[benoitf]

@metlos I could work on different tasks like two different issues to fix, then I will use the same devfile and implement feature1 in workspace1, feature2 in workspace2
I think it's a basic scenario to work on the same project I don't know how you are working else

[rhopp]

I was thinking in the same lines as @benoitf. It's not "unavoidable", as @metlos requested, but it's big timesaver (no need to checkout&rebuild different branches... Just work in different "browser windows")

[sunix]

@metlos This one of the greatest thing with che: you can work on the same project on different workspaces. The usecase is about creating a workspace each time you start to work on a new task.
1 workspace for 1 task gives you the ability to easily switch from 1 task to another and solve the task/context switching hell: for instance waiting for feedback from a PR, don't have to deal with branches and built binaries issue, etc ...

My current Che workspaces, some are based on the same devfile:

I don't know what is true or not. But since the begining, chectl was creating workspaces and adding _1 _2 when a workspace with the same name already exist. Devfile is taking the idea the factory had. But instead of storing workspace meta in the database, we have that in a yaml file that we can more easily share.

So people who were using factory will use devfile for the same kind of usecase: creating new workspaces from a kind of template. In that scenario it is a regression. I have been using Che daily since version 4 and it was behaving this way.

Last, it is a very bad UX to fail, if the workspace is created with sergii_1, user could just change it or just keep it like it is if user doesn't mind with the name. At least he can continue and work right away. Failing will force the user to retry again and make him eventually unhappy.

[metlos]

I do see the appeal for having a workspace per feature but IMHO doing it this way - duplicating the checkout, builds and more importantly all the sidecar machinery is a huge waste of resources.

Also, it can be argued that creating a workspace that already exists is also a bad UX. This just depends on what the user expectations are and, as Stevan said, this boils down to asking the user or letting them configure the behavior they prefer.

Also, I'd imagine that the users will run their workspaces in a constrained environment so they might easily trigger quota breaches, etc, if the workspaces are created anew all the time.

I'm happy with changing the default behavior so that devfile behaves like a project template or a factory, I just want to point out that it is not all rosy ☺️

[gorkem]

@metlos Creating a workspace and running a workspace are 2 different actions. A workspace is just a record on the DB so there are no real resources involved with the creation of the workspace. The resources are created when somebody actually uses the workspace and we do have other ways to limit the number of running workspaces and putting timeouts etc to better utilize resources.

[skabashnyuk]

We were able to discuss this and #13683 with @l0rd and @slemeur.

There is no debate that use-cases a legitimate. However, the ways how different clients (chectl, factory acceptance) react to a fact that workspace may exist may/should/must be rethought. However, at this moment we don't want to spend a lot of time discussing alternative behavior and the goal of this task should be - to restore original behavior: if workspace already exists then new workspace would be created with a name wsname-xxxxx where xxxxx is a number.

About the implementation: taking into account that behavior might be changed I, @metlos @sparkoo and rest of the che platform team though that the most reasonable place to fix it is in workspace API clients: chectl and dashboard. Che Platform team is going to start working on that.

[benoitf]

I'm not in favor in handling it client side

[skabashnyuk]

@benoitf I see that. And that makes me upset during wring this comment #13794 (comment). Do you see a compromise solution where both sides disagree? I'm open to considering some alternatives. Especially I'm interested in two conditions.

• clients behavior might be multiple times changed
• API compatibility should be handled.

[benoitf]

It's a corner case (only workspace already exists) and API compatibility is easy as it can be provided by the client like https://developer.github.com/v3/media/#request-specific-version

[benoitf]

This solution is perfectly fine for me

[skabashnyuk]

CC @davidfestal @l0rd . Please take a look at #13794 (comment)

[l0rd]

@sparkoo this solution makes sense

[sparkoo]

@sunix @benoitf @l0rd @skabashnyuk do we want to use generateName in all our devfiles in devfile-registry?

[benoitf]

I would say yes as I may start two workspaces from the same devfile

[skabashnyuk]

I would say yes too.

[sunix]

I am fine if the workspace creation doesn't fail if there is a name and generate-name are both defined and the name already exist.

[sunix]

To me, using generate-name or not ... i don't mind but this flow should work:

1. User is creating a workspace from a devfile
2. User is adding a che plugin from che-theia to the created workspace (it is adding it to the devfile associated to a workspace)
3. User is restarting the workspace: it works
4. User is getting the current devfile sources and recreating a new workspace from it without any modification.
5. The new workspace should be created without any errors.

Whatever the implementation, could we have that flow working properly ?

[skabashnyuk]

@sunix I don't see any issues in this plan, do you?

[sunix]

@skabashnyuk if there is no issue then it is fine but I have asked some questions during sprint review demos: what if we have name and generate-name at the same in the devfile when generating a new workspace and the answer was that name would be used, not generate-name. It means that the previous flow would not work. But if it works, it is fine.

[skabashnyuk]

I think you/we are mixing together two things devfile as a file/template and devfile object as part of workspace object. I still don't see any issues in my interpretation of your flow, however, it can be different from yours. That is why I can suggest two ways.

• You can test the proposed pr.
• We can merge pr as is and create a new issue after to adjust behavior if needed.

[sunix]

This issue Workspace creation fails if a workspace with the same name already exist is about devfile as a file/template to create a new workspace. But the devfile object as a workspace object could be reuse as a file/template.

https://github.com/eclipse/che/pull/14157/files#r313852917 that is not fine and would not work in the previous flow. Creation would still fail.

[sparkoo]

how do you get devfile file/template from existing workspace? We can tune that place and create generateName there.

[skabashnyuk]

@sunix would it #13794 (comment) work for you?

[sunix]

What would work for me is a way so that workspace creation never fails because of name conflict. Whatever the devfile you take.

[sunix]

How is this kubernetes rule important compared to the value we provide to the user in not failing?
https://github.com/eclipse/che/pull/14157/files#r313858328

[sparkoo]

Then you have option to use generateName and workspace creation will not fail.
Whole devfile is inspired by kubernetes and is following it where possible. Users of Che are developers and possibly know kubernetes so we'd like to keep it as close as possible so user will come to familiar environment. I don't see why having both options here is bad. If you have issue with getting devfile from existing workspace having name, we can fix it there.

[sparkoo]

@l0rd @benoitf @davidfestal WDYT?

[sparkoo]

@slemeur ^^^

[l0rd]

@sunix if, in your devfile, you use generateName instead of name the workspaces creation will always work right? That looks like a good compromise. What's the issue with it?

[sunix]

@l0rd

Yes, I would use generateName yes and the generated workspaces would have name AND generateName. At this point, in the newly created workspace, you could choose to clone a new git project, add few commands and so on. The devfile assiciated with the workspace would be synchronized with these new commands or projects.

You may then want to create a new workspace from that enhanced devfile. But if you do that it would fail ... because you would have name already taken and even if you have generateName.

We would have this kind of use case also if we want to create a factory from a existing devfile. At some point, to have that things working as the user would like to, we will ask him to copy the content of the devfile in a gist, ask him to remove the name field from the devfile and start the a new workspace from it ?

I fill like this rule will be painful for the end user but if you want to do then no worries.

[metlos]

I really hate to beat the dead horse here, but I think the n-th cycle we're in on this issue is due to the different understanding of what devfile is.

I apologize for this to be a rather long comment but I find it necessary to paint a complete(ish) picture before I try to come to some conclusion.

On one hand, @sunix, @benoitf and others want to understand the devfile as a workspace template of sorts, e.g. use 1 devfile to create N workspaces. This is completely valid and IMHO no one argues about the utility of it being possible to create several workspaces using a single devfile.
Let's call this understanding "workspace template".

On the other hand some other people, including me, understand the devfile as the workspace itself. There is no such thing as a "devfile object" from which a number of workspaces has been created. No, there is just a bunch of workspaces, each defined using a "recipe" that we happen to call devfile. Let's call this understanding "workspace definition".

Now let me spend some time explaining the "workspace definition" approach in more detail. First, because that is what I believe is a more rational understanding (while fully agreeing about the need to support the workflow that @sunix suggests) and second because I believe many of the arguments were not well taken by the "opposing party". So let me try one more time to explain why I think this is a reasonable understanding of the devfile.

It all boils down to Kubernetes. In Kubernetes, there are objects of different kinds, each identified by its name (in given namespace). There can be no 2 objects of the same name and kind in a single namespace. If devfile was a Kubernetes object (which is something I think we're aiming for), I believe its kind should be "Workspace". From this, all the reasoning about the behavior of "name" and "generateName" that we argued for, follows rather directly - we're trying to adhere to "the principle of the least astonishment" and mimic the behavior of other Kubernetes objects (name taking precedence over generateName is indeed the behavior in Kubernetes).

On the other hand the "workspace template" approach requires a different understanding of the objects involved. As alluded to already by @benoitf, it would be a logical approach if we were storing objects of kind "Devfile" and somehow from them created objects of kind "Workspace". IMHO, this indirection doesn't bring any value because there is no difference between "Devfile" object and "Workspace" object apart from the fact that the workspace object can be started/stopped. But what would the "Devfile" object be for?

So now, I'm going to assume that we agree on there being no need to store "Devfile" objects and that we want to only work with "Workspace"s (and please bring forward any usecases for "Devfile" objects). We want to follow the Kubernetes conventions, which dictate the name to be unique (within the kind and namespace). If we want to follow Kubernetes conventions we have no option but to require name to be unique and fail workspace creation if it isn't or use generateName. If we encounter devfile with both name and generateName we have to use the name. This is the behavior that we actually implemented.

So now, I said that I wanted to support the workflows that @sunix advertizes in this issue but the above doesn't exactly do that (and I understand the objections @sunix has had, because, really, we didn't do what he asked for).

It is my belief that the "workspace template" workflow is best done using the workspace factory, which basically says "I'm going to create a workspace like that one", as opposed to "I'm going to create this workspace" which the workspace endpoint does. I.e. /factory = "workspace template", /workspace = "workspace definition". This btw. means that che server currently doesn't work like that yet - #13683 is still open.

An ideal solution, in my mind, is done on the client side (as already pointed out by several others) and that's because, in the end, it is the user's decision what should happen with the workspace being created using the devfile - questions like "you already have a workspace called this name. Do you really want to create another one?", "what should be the name of the new workspace?". While the server side can implement logic to override fields of the devfile with values from query parameters for example, it is the client side that needs to inform the users about the state of the workspaces and ask for guidance.

IMHO, the attitude of "I want this to work like that because it always has" is not correct - we weren't based on Kubernetes prior to Che 7.

What I would like to do is to do this properly - not introduce historical behavior that is going to be hard/unintuitive to revert in the future.

So the plan for me would be this:

• Add generateName so that we support the same naming logic as Kubernetes
• factory should never fail to create workspace due to name collisions - Unable to accept factory with workspace, which name would collide with existing workspace name #13683
• Implement overrides of devfile properties using query parameters during workspace creation
• teach chectl to use the devfile overrides during workspace creation
• (optional) make the workspace creation using chectl interactive

WDYT?

[sparkoo]

2nd point in @metlos's plan would mean that currently proposed PR doesn't fix the #13683. Factory behaves the same as /workspace/devfile, so it will fail when passing devfile with name that already exist. To fix that, we would need to fix the factory, before it gets into the WorkspaceManager.

[sparkoo]

@sunix @benoitf @l0rd @slemeur @davidfestal can someone of you please react to @metlos's comment? We would like to merge the PR and close the issue. We can continue with creating new issues by @metlos's plan and discuss details there, but I currently can't see any unresolved/unexplained issues blocking current PR to be merged. This is taking too long and we'd like to move it forward.

[l0rd]

@sparkoo the PR is approved and I don't think you need to wait any comment here to merge it.

My point of view is that #13683 is the real P1 and need to be fixed.

Making the devfile a kind of a template (a la OpenShift Templates) is a nice to have for me.