Make brokers pod use a unique name to avoid collisions #12518
Conversation
Signed-off-by: Oleksandr Garagatyi <ogaragat@redhat.com>
che-bot
added
status/code-review
| @@ -196,7 +196,7 @@ private Container newContainer( | |||
| private Pod newPod() { | |||
| return new PodBuilder() | |||
| .withNewMetadata() | |||
| .withName(BROKERS_POD_NAME) | |||
| .withName(generateUniqueName(BROKERS_POD_NAME)) | |||
There was a problem hiding this comment.
I think it would be better to prefix BrokerPodName with workspace id if it is possible
There was a problem hiding this comment.
Looks like you changed code line and no tests affected. I do not think that it is critical functionality that must be tested by still it would be nice to have some check in tests.
There was a problem hiding this comment.
Workspace ID would not help to make it unique. Do you mean another purpose?
There was a problem hiding this comment.
How to test that something was randomly generated? Should I just test that it starts from something? I do not think such tests are really needed.
There was a problem hiding this comment.
As far as I know, there should be one Plugin Broker pod for one starting workspace. Could you elaborate why prefixing with workspace id won't help prevent failure of starting several workspaces concurrently?
There was a problem hiding this comment.
Well, I see that name should be prefixed by environmentProvisioner here https://github.com/eclipse/che/blob/a4f8410e6e3bc1797baa10142079f71675596766/infrastructures/kubernetes/src/main/java/org/eclipse/che/workspace/infrastructure/kubernetes/wsplugins/PluginBrokerManager.java#L104.
But wonder why there are any issues with starting several Che 7 workspaces concurrently in the same kubernetes namespace.
There was a problem hiding this comment.
Because if we start several workspaces we start several brokers pods
There was a problem hiding this comment.
But they are named workspace2hedhs4g-plugin-broker, workspace90rfhjcgg-plugin-broker, etc. And they are not conflicting. Did I miss something?
There was a problem hiding this comment.
You are right. Closing this PR since it is not needed
|
@rkratky @dneary @slemeur People still fill in |
|
@sleshchenko these changes are just part of the changes needed to allow starting several workspaces in the same namespace. |
|
ci-test |
@garagatyi Do you mean that detailed |
|
@sleshchenko This PR doesn't provide a complete solution, so it should not go to the release notes. I'll remove it from the PR description |
|
Results of automated E2E tests of Eclipse Che Multiuser on OCP: |
What does this PR do?
Make brokers pod use a unique name to avoid collisions. This prevents failure of starting several workspaces concurrently.
What issues does this PR fix or reference?
Related to #12238
Docs PR