-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Implemented recovery functionality for Kubernetes/OpenShift infrastructures #9301
Conversation
aca3dc1
to
de5f41f
Compare
infrastructures/kubernetes/pom.xml
Outdated
<dependency> | ||
<groupId>org.eclipse.persistence</groupId> | ||
<artifactId>javax.persistence</artifactId> | ||
<scope>provided</scope> |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workspace_id uniuque
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) | ||
); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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) | ||
); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
422cf79
to
e6e35cf
Compare
ci-test |
ci-test build report: |
b23fc8a
to
27a3dca
Compare
ci-test build report: |
989af77
to
7f46175
Compare
756253b
to
276e1d5
Compare
try { | ||
return managerProvider | ||
.get() | ||
.createQuery("SELECT r FROM KubernetesRuntime r", KubernetesRuntimeState.class) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
b415c03
to
08bc586
Compare
48d8874
to
25a8bb7
Compare
ci-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good job 👍
There was a problem hiding this 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
ci-test build report: |
25a8bb7
to
9c82289
Compare
@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. |
ci-test |
048dd86
to
8bcef4e
Compare
ci-test build report: |
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