You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@submarine.apache.org by GitBox <gi...@apache.org> on 2020/12/24 06:43:56 UTC
[GitHub] [submarine] charlie0220 opened a new pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
charlie0220 opened a new pull request #481:
URL: https://github.com/apache/submarine/pull/481
### What is this PR for?
Add 10GB persistent volume for each notebook which mounting path is /home/jovyan/workspace.
I use local persistent volume which mounting path of node is /mnt since Submarine do not use any storage class yet.
### What type of PR is it?
[Improvement]
### Todos
* [ ] - Task
### What is the Jira issue?
[SUBMARINE-685](https://issues.apache.org/jira/projects/SUBMARINE/issues/SUBMARINE-685?filter=allopenissues)
### How should this be tested?
[Travis-CI](https://travis-ci.com/github/charlie0220/submarine/builds/210378125)
### Screenshots (if appropriate)
### Questions:
* Does the licenses files need update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] lowc1012 commented on a change in pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
lowc1012 commented on a change in pull request #481:
URL: https://github.com/apache/submarine/pull/481#discussion_r548424475
##########
File path: submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -385,6 +414,112 @@ private IngressRouteSpec parseIngressRouteSpec(String namespace, String name) {
return spec;
}
+ private void createPersistentVolume(String name) {
Review comment:
Please move these functions into [NotebookUtils.java](https://github.com/apache/submarine/blob/master/submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/util/NotebookUtils.java)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] charlie0220 commented on a change in pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
charlie0220 commented on a change in pull request #481:
URL: https://github.com/apache/submarine/pull/481#discussion_r557008548
##########
File path: submarine-test/test-k8s/src/test/java/org/apache/submarine/rest/NotebookRestApiIT.java
##########
@@ -188,6 +189,7 @@ public void testListNotebooksWithUserId() throws Exception {
Assert.assertEquals(ENV_NAME, getEnvironment.getEnvironmentSpec().getName());
// create notebook instances
+ Thread.sleep(15000);
Review comment:
In this test we use different ways to create notebook with the same name and delete it in very short time, but persistent volume (pv) can not be deleted quickly. When previous pv haven't been deleted completely and we create new pv with the same name, the error will occur, so I choose to add Thread.sleep. I will add some comments in the file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] charlie0220 removed a comment on pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
charlie0220 removed a comment on pull request #481:
URL: https://github.com/apache/submarine/pull/481#issuecomment-754730652
> @charlie0220 Could you have time to fix the conflicts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] asfgit closed pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #481:
URL: https://github.com/apache/submarine/pull/481
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] pingsutw commented on a change in pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
pingsutw commented on a change in pull request #481:
URL: https://github.com/apache/submarine/pull/481#discussion_r555653679
##########
File path: submarine-test/test-k8s/src/test/java/org/apache/submarine/rest/NotebookRestApiIT.java
##########
@@ -224,6 +226,7 @@ public void testListNotebooksWithUserId() throws Exception {
private void runTest(String body, String contentType) throws Exception {
// create
+ Thread.sleep(15000);
Review comment:
Same here
##########
File path: submarine-test/test-k8s/src/test/java/org/apache/submarine/rest/NotebookRestApiIT.java
##########
@@ -188,6 +189,7 @@ public void testListNotebooksWithUserId() throws Exception {
Assert.assertEquals(ENV_NAME, getEnvironment.getEnvironmentSpec().getName());
// create notebook instances
+ Thread.sleep(15000);
Review comment:
Could you add some comments for `Thread.sleep(15000);`? Don't know why we should use it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] charlie0220 commented on pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
charlie0220 commented on pull request #481:
URL: https://github.com/apache/submarine/pull/481#issuecomment-762628069
@jiwq @pingsutw Could you help me to review this PR again? Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] charlie0220 commented on pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
charlie0220 commented on pull request #481:
URL: https://github.com/apache/submarine/pull/481#issuecomment-754730652
> @charlie0220 Could you have time to fix the conflicts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] lowc1012 commented on a change in pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
lowc1012 commented on a change in pull request #481:
URL: https://github.com/apache/submarine/pull/481#discussion_r548424475
##########
File path: submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -385,6 +414,112 @@ private IngressRouteSpec parseIngressRouteSpec(String namespace, String name) {
return spec;
}
+ private void createPersistentVolume(String name) {
Review comment:
Please move these functions into [NotebookUtils.java](https://github.com/apache/submarine/blob/master/submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/util/NotebookUtils.java)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] lowc1012 commented on pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
lowc1012 commented on pull request #481:
URL: https://github.com/apache/submarine/pull/481#issuecomment-751239160
@jiwq Could you please help to review this PR ? Thanks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] charlie0220 edited a comment on pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
charlie0220 edited a comment on pull request #481:
URL: https://github.com/apache/submarine/pull/481#issuecomment-754757578
> @charlie0220 Could you have time to fix the conflicts?
@jiwq yes i'm working on it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] lowc1012 commented on a change in pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
lowc1012 commented on a change in pull request #481:
URL: https://github.com/apache/submarine/pull/481#discussion_r548859687
##########
File path: submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -254,6 +267,19 @@ public Notebook createNotebook(NotebookSpec spec) throws SubmarineRuntimeExcepti
Map<String, String> labels = new HashMap<>();
labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, spec.getMeta().getOwnerId());
notebookCR.getMetadata().setLabels(labels);
+
+ // create persistent volume
+ createPersistentVolume(notebookCR.getMetadata().getName());
+
+ // create persistent volume claim
+ createPersistentVolumeClaim(notebookCR.getMetadata().getNamespace(),
+ notebookCR.getMetadata().getName());
+
+ // bind persistent volume claim
+ V1PersistentVolumeClaimVolumeSource pvc_source = new V1PersistentVolumeClaimVolumeSource()
Review comment:
Variable name should be in lower camel case.
Please correct the others in this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] charlie0220 commented on pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
charlie0220 commented on pull request #481:
URL: https://github.com/apache/submarine/pull/481#issuecomment-754757578
> @charlie0220 Could you have time to fix the conflicts?
yes i'm working on it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] jiwq commented on pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
jiwq commented on pull request #481:
URL: https://github.com/apache/submarine/pull/481#issuecomment-754722780
@charlie0220 Could you have time to fix the conflicts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] jiwq commented on a change in pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
jiwq commented on a change in pull request #481:
URL: https://github.com/apache/submarine/pull/481#discussion_r548973097
##########
File path: submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/parser/NotebookSpecParser.java
##########
@@ -132,9 +134,32 @@ private static V1PodTemplateSpec parseTemplateSpec(NotebookSpec notebookSpec) {
container.setResources(resources);
}
+ // Volume Mount
+ List<V1VolumeMount> mounts = new ArrayList<>();
+ V1VolumeMount mount = new V1VolumeMount();
+ mount.setMountPath("/home/jovyan/workspace");
Review comment:
Use the static variable named `DEFAULT_MOUNT_PATH` is better.
##########
File path: submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -385,6 +414,112 @@ private IngressRouteSpec parseIngressRouteSpec(String namespace, String name) {
return spec;
}
+ private void createPersistentVolume(String name) {
+ V1PersistentVolume pv = new V1PersistentVolume();
+ V1PersistentVolumeSpec pv_spec = new V1PersistentVolumeSpec();
+ V1ObjectMeta pv_metadata = new V1ObjectMeta();
+ List<String> accessMode = new ArrayList<>();
+ Map<String, Quantity> capacity = new HashMap<>();
+ V1LocalVolumeSource local = new V1LocalVolumeSource();
+ V1VolumeNodeAffinity affinity = new V1VolumeNodeAffinity();
+ V1NodeSelector required = new V1NodeSelector();
+ List<V1NodeSelectorTerm> terms = new ArrayList<>();
+ V1NodeSelectorTerm term = new V1NodeSelectorTerm();
+ List<V1NodeSelectorRequirement> expressions = new ArrayList<>();
+ List<String> value = new ArrayList<>();
+ value.add("linux");
+ V1NodeSelectorRequirement expression = new V1NodeSelectorRequirement()
+ .key("kubernetes.io/os")
+ .operator("In")
+ .values(value);
+
+ accessMode.add("ReadWriteOnce");
+ capacity.put("storage", new Quantity("10Gi"));
Review comment:
The quantity should passed from `NotebookSpec` rather than hard code.
##########
File path: submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/parser/NotebookSpecParser.java
##########
@@ -132,9 +134,32 @@ private static V1PodTemplateSpec parseTemplateSpec(NotebookSpec notebookSpec) {
container.setResources(resources);
}
+ // Volume Mount
+ List<V1VolumeMount> mounts = new ArrayList<>();
+ V1VolumeMount mount = new V1VolumeMount();
+ mount.setMountPath("/home/jovyan/workspace");
+ mount.setName("workspace-" + notebookSpec.getMeta().getName());
+ mounts.add(mount);
+ container.setVolumeMounts(mounts);
+
containers.add(container);
podSpec.setContainers(containers);
+ // Set Volumes
Review comment:
Should gave more comments about the logical for init the volume.
##########
File path: submarine-server/server-submitter/submitter-k8s/src/main/java/org/apache/submarine/server/submitter/k8s/K8sSubmitter.java
##########
@@ -254,6 +267,19 @@ public Notebook createNotebook(NotebookSpec spec) throws SubmarineRuntimeExcepti
Map<String, String> labels = new HashMap<>();
labels.put(NotebookCR.NOTEBOOK_OWNER_SELECTOR_KET, spec.getMeta().getOwnerId());
notebookCR.getMetadata().setLabels(labels);
+
+ // create persistent volume
+ createPersistentVolume(notebookCR.getMetadata().getName());
+
+ // create persistent volume claim
+ createPersistentVolumeClaim(notebookCR.getMetadata().getNamespace(),
+ notebookCR.getMetadata().getName());
+
+ // bind persistent volume claim
+ V1PersistentVolumeClaimVolumeSource pvc_source = new V1PersistentVolumeClaimVolumeSource()
+ .claimName(notebookCR.getMetadata().getName() + "-claim");
Review comment:
Should generate the claim name use the same method, so that avoid future mistakes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [submarine] charlie0220 closed pull request #481: SUBMARINE-685. Retain the data in notebook pod even if notebook server is destroyed
Posted by GitBox <gi...@apache.org>.
charlie0220 closed pull request #481:
URL: https://github.com/apache/submarine/pull/481
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org