Che: 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: https://github.com/eclipse/che/issues/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

• https://github.com/eclipse/che/issues/13683 factory flow.
  
  
  
  • https://github.com/eclipse/che/issues/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 https://github.com/eclipse/che/issues/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 https://github.com/eclipse/che/issues/13794#issuecomment-517296544. 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

[skabashnyuk]

I didn't get the idea that you proposed in this case. Can you be more concrete?

[benoitf]

https://restfulapi.net/versioning/

[skabashnyuk]

https://restfulapi.net/versioning/

The idea is great. Do you see a practical way of how we can easily implement with java-based che-server? Because I don't see that realistically possible.

[benoitf]

JAX-RS is letting you to do this

I found old articles as there is nothing new there:
http://codebias.blogspot.com/2014/03/versioning-rest-apis-with-custom-accept.html
https://allegro.tech/2015/01/Content-headers-or-how-to-version-api.html
https://www.maxenglander.com/2013/04/23/basic-restful-api-versioning-in-jersey.html

[skabashnyuk]

http://codebias.blogspot.com/2014/03/versioning-rest-apis-with-custom-accept.html
https://allegro.tech/2015/01/Content-headers-or-how-to-version-api.html
https://www.maxenglander.com/2013/04/23/basic-restful-api-versioning-in-jersey.html

Yeh, that is interesting reading. Personally, if we ever consider implementing such thing I would take a look in GraphQL https://developer.github.com/v4/ . Even if these concepts are interesting to read I don't see that we can easily adapt it in close feature in our JAVA-based che-server in a reasonable time. Taking to account:

• API compatibility should be handled.
• clients behavior might be multiple times changed. We didn't give any promise that chectl and factory flow will never be changed in the future (in terms of a workspace name collision).

I still don't see another way to implement this and #13683 issue except that I've described here https://github.com/eclipse/che/issues/13794#issuecomment-517296544

[vparfonov]

My two cents in this discussion, i think provide solution on client side more reasonable and flexible. For example, chectl got Already exists exception can be proposed several choices for user:

• recreate with same name
• start existed as is
• create new one with new name

Devfile definition is part of our API and if it have workspace name definition, API should respect it and don't modify it without user interaction

[sleshchenko]

API should respect it and don't modify it without user interaction

It's my concern as well. If we take a look on K8s API, then we can see different fields: name and generateName https://docs.openshift.com/container-platform/3.5/rest_api/kubernetes_v1.html#v1-objectmeta
So, the user is able to specify the exact name or prefix for a generated name if the client is sure that exact name does not matter for him.

If we talk about workspace per feature/issue then che-1. che-2, che-3 does not make sense for me. It makes more sense for user to specify name manually, like che-tls, che-devfile-api, che-npe-fix, ...

[benoitf]

If I call API to create a workspace from a devfile, it should always create a new workspace
the workspace name should be automatically generated.

If I want to start an existing workpace, I start it by its name.

Optionally, the client can specify a name and then only in that case the server could report that this workspace name is already taken.

[sleshchenko]

If I call API to create a workspace from a devfile, it should always create a new workspace
the workspace name should be automatically generated.

From the workspace API perspective, workspace name is the same as devfile name.
I mean there is no a dedicate field for workspace name and devfile name, it's one field.

[benoitf]

From the workspace API perspective if I give only a devfile to create a workspace there is no workspace name.
Only If I provide a name then from this perspective a name is there.

[vparfonov]

From the workspace API perspective if I give only a devfile to create a workspace there is no workspace name.
Only If I provide a name then from this perspective a name is there.

https://redhat-developer.github.io/devfile/devfile#metadata

[benoitf]

@vparfonov it's the name of the devfile not the name of the workspace

I don't give a workpace name when I create a workspace from a devfile

WorkspaceService implementation can use that name as a prefix or not, I don't care as a user. I just need to have a workspace created at the end.

[vparfonov]

@vparfonov it's the name of the devfile not the name of the workspace

I don't give a workpace name when I create a workspace from a devfile

Devfile Name
The name of the devfile. Workspaces created from devfile, will inherit this name.

WorkspaceService implementation can use that name as a prefix or not, I don't care as a user. I just need to have a workspace created at the end.

Yes and chectl is perfect tool todo it. Chectl can help user with it and we don't need change API for minute wishes.

[benoitf]

relationship between a devfile and a workspace is:

devfile 1 ... n workspaces

so workspace name can't be devfile name

[benoitf]

@vparfonov the relationship implemented is wrong, it's not a matter of what a client should do.

Che is creating workspace on demands, if you ask 5 times Che to create a workspace from a given devfile it should create 5 workspaces, that's all. That is what API should provide. It's the default.

if you want that chectl notice user you that a workspace might already exist for a given devfile it's a total different story.

[sunix]

Hello, could we move forward and choose the simplest way to implement and maintain ? -> server side.

[skabashnyuk]

Hello, could we move forward and choose the simplest way to implement and maintain ? -> server side.

Personally, I can't. You are completely ignoring mine on a couple of other contributors arguments.
This two-point is never been addressed realistically in "server-side fix"

• API compatibility should be handled.
• clients behavior might be multiple times changed. We didn't give any promise that chectl and factory flow will never be changed in the future (in terms of a workspace name collision).

[benoitf]

@skabashnyuk
I could argue that API compatibility is not really an issue as for now it's a bug and regression to not be able to create several workspaces from the same devfile. So all clients aimed to follow that.

workspace name collision can't occur on server side. If the generated name already exists, server side can pick up a new one and iterate again.

So basically I see no APi compatibility issues in providing auto-generated workspace name (that can use devfile name as prefix) and client behavior as they're expecting right now that a new workspace is always created.

[skabashnyuk]

workspace name collision can't occur on server side. If the generated name already exists, server side can pick up a new one and iterate again.

ok. @l0rd @slemeur can you confirm that this is desired behavior and if we decided to change it then backward compatibility will not be required.

[sunix]

Personally, I can't. You are completely ignoring mine on a couple of other contributors arguments.

@skabashnyuk
Sorry, we are not ignoring but keeping explaining you ... in this very long thread ... why your arguments don't work.
This is going too far: we are spending too much time debating than fixing that simple bug.
If in the futur we figure out that is not the right way to do, we'll change it but in my opinion we should follow the simplest way to implement that and maintain that at the moment. And it is server side, not in the various clients that would call the API.

[skabashnyuk]

Ok. However, the team does not agree with you we will do whatever you've asked in this task.

[skabashnyuk]

Does anybody see any difference if instead of "workspace-XXX" where XXX is sequential number would be "workspace-YYYYY" where YYYYY - is a random letter/numbers (same as a new workspace name on the dashboard)? Cause searching for next XXX quite a heavy lifting
process for backend

[benoitf]

I think it's nice to have the devfile name part of workspace name to better see what is inside the workspace but if workspaces have all random name suffixes like -ml62 (even the first one) I'm fine as well
BTW this is what dashboard is using for newly workspaces.

But it would be nice if API allow to specify the workspace name as well (and if already taken throw error in that case)

[sparkoo]

Although pull request is already open (#14129), our team does not much like the solution and discussion now directs to generate random suffix. We would like to propose this solution:
Inspired by Kubernetes (https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.14/#objectmeta-v1-meta), we would like to add a new field to the devfile-metadata - generateName. This field will be applied if and only if name is not defined. In that case, final workspace name will be generated like [generateName]-YYYYY, where Y is random [0-9a-z] character. In case that name is also defined, generateName will be ignored and name will be used and must be unique in the namespace.
So devfile will look like this:

---
apiVersion: 1.0.0
metadata:
  generateName: python-
projects:
  -
    name: python-hello-world
    source:
      type: git
      location: "https://github.com/che-samples/python-hello-world.git"
components:
  -
    type: chePlugin
    id: ms-python/python/latest
    memoryLimit: 512Mi
  -
    type: dockerimage
    alias: python
    image: quay.io/eclipse/che-python-3.6:nightly
    memoryLimit: 512Mi
    mountSources: true
commands:
  -
    name: run
    actions:
      -
        type: exec
        component: python
        command: "python hello-world.py"
        workdir: ${CHE_PROJECTS_ROOT}/python-hello-world

(taken from https://github.com/eclipse/che-devfile-registry/blob/master/devfiles/python/devfile.yaml only changed in metadata-name/generateName field)
and workspaces created from this devfile might look like python-123ab, python-1a2b3, ...

We believe, that this solution is much more cleaner, explicit, predictable and future-proof. We would also follow Kubernetes pattern, so users that already know Kubernetes will come to familiar environment. Of course it will be more work as we would have to update our devfile specification, devfiles in devfile registry, and wsmaster code changes will be more heavyweight. However, we think that it's worth the effort.
Thoughts?

[benoitf]

This solution is perfectly fine for me

[skabashnyuk]

CC @davidfestal @l0rd . Please take a look at https://github.com/eclipse/che/issues/13794#issuecomment-518549557

[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 https://github.com/eclipse/che/issues/13794#issuecomment-521235907 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 - https://github.com/eclipse/che/issues/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:

• [x] Add generateName so that we support the same naming logic as Kubernetes
• [ ] factory should never fail to create workspace due to name collisions - https://github.com/eclipse/che/issues/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.