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

Use meaningful value for plugin broker container name instead of generated one #12372

Merged
merged 1 commit into from
Jan 25, 2019

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Jan 10, 2019

What does this PR do?

During manual testing of PVCs supporting in K8s/OS recipes, I discovered that Che 7 workspace acquires new folder for each workspace start. So, each Che 7 workspace start increases folders number in common PV:

pv0069
`-- workspacewsgi9hoklsfdwp7x
    |-- che-logs
    |   |-- che-plugin-broker
    |   |   |-- broker3zeff1
    |   |   |-- broker8iylgg
    |   |   |-- brokergeky82
    |   |   |-- brokerr7l031
    |   |   |-- brokerrmt1lb
    |   |   `-- brokery19p82
    |   |   ...
    |   `-- ws
    |       |-- che-hello
    |       |-- che-machine-exec
    |       |-- dev
    |       |-- theia-dev
    |       `-- theia-ide
    |-- plugins
    |   `-- che_ui_service_plugin.theia
    `-- projects

Note that it unique PVC strategy does not have the described issue (but I did investigated why):

----
pv0041
`-- workspacewsgi9hoklsfdwp7x
    `-- che-logs-che-plugin-broker
----
pv0061
`-- workspacewsgi9hoklsfdwp7x
    `-- plugins
        `-- che_ui_service_plugin.theia
----
pv0079
`-- workspacewsgi9hoklsfdwp7x
    `-- projects
----
pv0065
`-- workspacewsgi9hoklsfdwp7x
    `-- che-logs-ws

Since Plugin Brokers do not write any logs in Logs volume it makes sense to deprecate usage of logs volume at all. The corresponding issue #11806.
But while there is no final decision whether it should be removed or not, it makes sense to use meaningful names for plugins brokers machines.
It also could help during issues investigation, like an example if for some reason plugin broker container is invalid - we will see error Container plugin-broker/broker1n71g0 is not valid, after aplying these changes we will see Container plugin-broker/eclipse-plugin-broker-che is not valid `

After these changes PV of common PVC has the following folders structure:

pv0027
|-- workspacefgr3e973rcupbt2x
|   |-- che-logs
|   |   |-- che-plugin-broker
|   |   |   |-- eclipse-che-init-plugin-broker-v0-7-0
|   |   |   `-- eclipse-che-plugin-broker-v0-7-0
|   |   `-- ws
|   |       |-- che-machine-exec
|   |       |-- dev
|   |       `-- theia-ide
|   |-- plugins
|   `-- projects

What issues does this PR fix or reference?

N/A

Release Notes

N/A

Docs PR

N/A

@sleshchenko sleshchenko added status/in-progress This issue has been taken by an engineer and is under active development. kind/task Internal things, technical debt, and to-do tasks to be performed. labels Jan 10, 2019
@sleshchenko sleshchenko self-assigned this Jan 10, 2019
Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Do you plan to add tests?

@sleshchenko sleshchenko changed the title [WIP] Use meaningful value for plugin broker container name instead of generated one Use meaningful value for plugin broker container name instead of generated one Jan 16, 2019
@sleshchenko
Copy link
Member Author

ci-test

@sleshchenko sleshchenko added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels Jan 16, 2019
@sleshchenko sleshchenko changed the title Use meaningful value for plugin broker container name instead of generated one [WIP] Use meaningful value for plugin broker container name instead of generated one Jan 16, 2019
@sleshchenko sleshchenko added status/in-progress This issue has been taken by an engineer and is under active development. and removed status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Jan 16, 2019
@sleshchenko
Copy link
Member Author

Previosly, I tried to implement using plugin type as container name but it can not be implemented since one broker container may be used for several plugin types, like for Che Plugin and Che Editor is used the same broker image.

So, we may we container image after replacing all non allowed character to - (image.toLowerCase().replaceAll("[^\\d\\w-]", "-")).
Then we will have

pv0027
|-- workspacefgr3e973rcupbt2x
|   |-- che-logs
|   |   |-- che-plugin-broker
|   |   |   |-- eclipse-che-init-plugin-broker-v0-7-0
|   |   |   `-- eclipse-che-plugin-broker-v0-7-0
|   |   `-- ws
|   |       |-- che-machine-exec
|   |       |-- dev
|   |       `-- theia-ide
|   |-- plugins
|   `-- projects

@garagatyi WDYT? It is better than we have now?
Maybe it makes sense to use image without version like eclipse-che-init-plugin-broker, eclipse-che-plugin-broker. Or just close this PR?

@eclipse-che eclipse-che deleted a comment from che-bot Jan 16, 2019
@garagatyi
Copy link

I'm OK with the solution you are suggesting.
But to be honest I would remove usage of logs volume in Che 7 at all (after moving Che 6 out of Che 7 sources). All the Che 7 components ignore it as far as I understand. And instead, they produce output to the container stdout. Which allows us to collect logs by logs collection stack if needed.

@sleshchenko sleshchenko changed the title [WIP] Use meaningful value for plugin broker container name instead of generated one Use meaningful value for plugin broker container name instead of generated one Jan 24, 2019
@sleshchenko
Copy link
Member Author

ci-test

1 similar comment
@sleshchenko
Copy link
Member Author

ci-test

@sleshchenko sleshchenko added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels Jan 25, 2019
@garagatyi
Copy link

It can spoil running several workspaces at the same time. See #12395

Copy link

@garagatyi garagatyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with these improvements

…rated one

Signed-off-by: Sergii Leshchenko <sleshche@redhat.com>
@garagatyi
Copy link

It can spoil running several workspaces at the same time. See #12395

My bad - it is not a problem

@che-bot
Copy link
Contributor

che-bot commented Jan 25, 2019

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12372
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@sleshchenko sleshchenko merged commit c3e0c2e into eclipse-che:master Jan 25, 2019
@sleshchenko sleshchenko deleted the pluginBrokerMachineName branch January 25, 2019 13:14
@che-bot che-bot removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Jan 25, 2019
@che-bot che-bot added this to the 6.18.0 milestone Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/task Internal things, technical debt, and to-do tasks to be performed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants