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

Provide machine logs volume and configuration of logs root path #8535

Merged
merged 3 commits into from
Feb 1, 2018

Conversation

akorneta
Copy link
Contributor

@akorneta akorneta commented Jan 31, 2018

What does this PR do?

Adds configuration of workspace logs root directory;
Provides environment variable for each OpenShift machine that contains value of logs root directory;
Adds logs volume into all OpenShift machines and make distinct subpaths for logs with machine name.
Modifies installation scripts of ws-agent and exec-agent with a logic that defines the logs folder for this agents and redirects bootstrapper logs to workspace logs root.

file structure on host for different strategies:

One PV will be used for storing logs of machines that are placed in the same pod.
unique:
unique_strategy

One PV will be used for storing logs and projects data of workspace with two machines.
common:
common_strategy

What issues does this PR fix or reference?

#7807
partially solves this issue(for OpenShift only): #7386

Docs PR

n/a

@akorneta akorneta 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 31, 2018
@akorneta akorneta self-assigned this Jan 31, 2018
@akorneta akorneta changed the title [WIP]: Provide machine logs volume and configuration of logs root path Provide machine logs volume and configuration of logs root path Jan 31, 2018
@akorneta akorneta 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 31, 2018
@akorneta
Copy link
Contributor Author

ci-build

@akorneta
Copy link
Contributor Author

ci-test-ocp

@riuvshin
Copy link
Contributor

will we have ws logs on docker?

@akorneta
Copy link
Contributor Author

akorneta commented Jan 31, 2018

@riuvshin maybe, but not in the scope of this pr

@riuvshin
Copy link
Contributor

riuvshin commented Jan 31, 2018

@akorneta sure, but we need ws logs on docker same as on openshift, so that should be considered I think in order to close ws logs issue

@akorneta
Copy link
Contributor Author

akorneta commented Jan 31, 2018

@riuvshin I understand that.
This PR will resolve this issue: #7807
and for OpenShift only, this one: #7386 (updated the pr summary)

@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Jan 31, 2018

@akorneta: isn't #7386 for logs of ws run on docker?

@akorneta
Copy link
Contributor Author

@dmytro-ndp no(see comment above), I'll not close that issue, just will comment that for OS infra it works.

@dmytro-ndp
Copy link
Contributor

@akorneta: I see, thank you.

# Defines the directory inside the machine where all the workspace logs are placed.
# The value of this folder should be provided into machine e.g. like environment variable
# so agents developers can use this directory for backup agents logs.
#CHE_WORKSPACE_LOGS_ROOT_DIR=/workspace_logs
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe default value should be set right here in che.env instead of defining in each agent installer script?
This way it will be more obvious to end user where are logs stored by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default value from che.properties will be used, and it equals to /workspace_logs(we won't define it in each script). This value is commented to be the same as all properties in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, thx

@codenvy-ci
Copy link

ci-test build report:
Build details
Test report
selenium tests report data
docker image: eclipseche/che-server:8535
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

@codenvy-ci
Copy link

Copy link
Member

@sleshchenko sleshchenko left a comment

Choose a reason for hiding this comment

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

LGTM.

Only one question: Did you consider configuring log dir for the terminal agent?

public class LogsRootEnvVariableProvider implements EnvVarProvider {

/** Environment variable that points to root folder of projects inside machine */
public static final String WORKSPACE_LOGS_ROOT_VARIABLE = "CHE_WORKSPACE_LOGS_ROOT_DIR";
Copy link
Member

Choose a reason for hiding this comment

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

I'd used ENV_VAR instead of VARIABLE

@akorneta
Copy link
Contributor Author

akorneta commented Feb 1, 2018

@sleshchenko
Yes, I thought about that. Actually, to be able to store terminal logs into the file we need to do some work on that because currently they are streamed to stdout and by the way, terminal logs do not contain any useful information(just start/end of the session) so, that's why I did not work on that.

@akorneta akorneta requested a review from l0rd February 1, 2018 09:18
@akorneta akorneta force-pushed the CHE-7807 branch 2 times, most recently from 74cbcbe to 87792d3 Compare February 1, 2018 14:03
#Defines the directory inside the machine where all the workspace logs are placed.
#The value of this folder should be provided into machine e.g. like environment variable
#so agents developers can use this directory for backup agents logs.
che.workspace.logs.root.dir=/workspace_logs

Choose a reason for hiding this comment

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

Consider usage of underscore between root and dir words

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, nice job!

@akorneta akorneta force-pushed the CHE-7807 branch 2 times, most recently from eb179d3 to fdb0dcc Compare February 1, 2018 15:51
@akorneta akorneta merged commit c21e9d8 into eclipse-che:master Feb 1, 2018
@akorneta akorneta deleted the CHE-7807 branch February 1, 2018 16:30
@l0rd
Copy link
Contributor

l0rd commented Feb 1, 2018

👍 thanks @akorneta

@l0rd
Copy link
Contributor

l0rd commented Feb 1, 2018

I have updated redhat-developer/rh-che#448

@benoitf benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Feb 5, 2018
@benoitf benoitf added this to the 6.1.0 milestone Feb 5, 2018
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.

9 participants