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

Implemented recovery functionality for Kubernetes/OpenShift infrastructures #9301

Merged
merged 3 commits into from
Apr 12, 2018

Conversation

sleshchenko
Copy link
Member

@sleshchenko sleshchenko commented Mar 30, 2018

What does this PR do?

Implements recovery functionality for Kubernetes/OpenShift infrastructures.

It's done by usage of persistent cache for storing Kuberentes/OpenShift runtimes states including machine and servers information.

Jpa is used as persistent cache backend. To make it possible to have two Running Che Server (like during Rolling Update) it is needed to synchronize JPA L2 cache with JGroups, it is done in #8982.

What issues does this PR fix or reference?

#5919

Release Notes

Implemented recovery functionality for Kubernetes/OpenShift infrastructures.

Docs PR

N/A

@sleshchenko sleshchenko added kind/enhancement A feature request - must adhere to the feature request template. status/in-progress This issue has been taken by an engineer and is under active development. labels Mar 30, 2018
@sleshchenko sleshchenko self-assigned this Mar 30, 2018
@sleshchenko sleshchenko requested a review from a user March 30, 2018 12:21
@sleshchenko sleshchenko force-pushed the openshiftRuntimesCache branch 2 times, most recently from aca3dc1 to de5f41f Compare March 30, 2018 12:40
<dependency>
<groupId>org.eclipse.persistence</groupId>
<artifactId>javax.persistence</artifactId>
<scope>provided</scope>
Copy link
Contributor

Choose a reason for hiding this comment

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

remove provied if not needed.

PRIMARY KEY (workspace_id)
);
-- indexes
CREATE UNIQUE INDEX index_k8s_runtime_workspace_id ON che_k8s_runtime (workspace_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

You already have an index on workspace_id. Instead, you need foreign kay on workspace table.


-- Runtime ---------------------------------------------------------------------
CREATE TABLE che_k8s_runtime (
workspace_id VARCHAR(255) NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

workspace_id uniuque

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a primary key. Are you sure that I must specify unique here? I checked other places like workspace, stack tables, id field is not marked as unique.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

machine_name VARCHAR(255),
attribute_key VARCHAR(255),
attribute VARCHAR(255)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

index on workspace_id, machine_name

Copy link
Member Author

@sleshchenko sleshchenko Mar 30, 2018

Choose a reason for hiding this comment

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

I planned to do it a bit later ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

server_name VARCHAR(255),
attribute_key VARCHAR(255),
attribute VARCHAR(255)
);
Copy link
Contributor

Choose a reason for hiding this comment

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

index workspace_id, machine_name, server_name

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@sleshchenko sleshchenko force-pushed the openshiftRuntimesCache branch 14 times, most recently from 422cf79 to e6e35cf Compare April 4, 2018 08:44
@sleshchenko
Copy link
Member Author

ci-test

@codenvy-ci
Copy link

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

@codenvy-ci
Copy link

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

@sleshchenko sleshchenko changed the title [WIP] Implemented recovery functionality for Kubernetes/OpenShift infrastructures Implemented recovery functionality for Kubernetes/OpenShift infrastructures Apr 10, 2018
@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 Apr 10, 2018
@sleshchenko sleshchenko force-pushed the openshiftRuntimesCache branch 2 times, most recently from 756253b to 276e1d5 Compare April 10, 2018 13:40
try {
return managerProvider
.get()
.createQuery("SELECT r FROM KubernetesRuntime r", KubernetesRuntimeState.class)
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use named query here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Replaced with a named query.

return managerProvider
.get()
.createQuery(
"SELECT m FROM KubernetesMachine m WHERE m.machineId.workspaceId = :workspaceId",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use named query here?

@sleshchenko sleshchenko force-pushed the openshiftRuntimesCache branch 4 times, most recently from 48d8874 to 25a8bb7 Compare April 11, 2018 13:31
@sleshchenko
Copy link
Member Author

ci-test

Copy link
Contributor

@akorneta akorneta left a comment

Choose a reason for hiding this comment

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

good job 👍

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.

From what I understand looks good. But PR is too big to provide a really thoughtful review.
BTW I was not sure about adding WS servers readiness probes on each Che master but @akorneta said that it is not supposed to be used on a two working Che masters, but rather during the swap of Che master instances.

}

public OpenShiftProject(
OpenShiftClientFactory clientFactory, String name, String workspaceId, boolean doPrepare)

Choose a reason for hiding this comment

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

Can you add a description to the doPrepare parameter and constructors?

Copy link
Member Author

Choose a reason for hiding this comment

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

Replaced doPrepare parameter with package private method that should be invoked by project factory.

@@ -44,4 +44,9 @@ public OpenShiftProject create(String workspaceId) throws InfrastructureExceptio
final String projectName = isNullOrEmpty(this.projectName) ? workspaceId : this.projectName;
return new OpenShiftProject(clientFactory, projectName, workspaceId);
}

public OpenShiftProject create(String workspaceId, String projectName)

Choose a reason for hiding this comment

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

Can you add javadocs with clarification why 2 methods exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

@codenvy-ci
Copy link

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

@sleshchenko
Copy link
Member Author

BTW I was not sure about adding WS servers readiness probes on each Che master but @akorneta said that it is not supposed to be used on a two working Che masters, but rather during the swap of Che master instances.

@garagatyi I have thought about avoiding WS servers readiness probes on each Che master and there is not an easy way to do that. Since original issue that should be solved now is Hot Update, I decided that it's OK for now to leave it. So, when we will have to solve the issue with multiple Che Master (replications) then this issue definitely should be investigated more.

@sleshchenko
Copy link
Member Author

ci-test

@codenvy-ci
Copy link

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

@sleshchenko sleshchenko merged commit 7b4adc6 into eclipse-che:master Apr 12, 2018
@sleshchenko sleshchenko deleted the openshiftRuntimesCache branch April 12, 2018 15:35
@sleshchenko sleshchenko removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Apr 12, 2018
@benoitf benoitf added this to the 6.4.0 milestone Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement A feature request - must adhere to the feature request template.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants