Delete Suggestion deployment after Experiment is finished #1150

sperlingxx · 2020-04-16T08:29:09Z

This PR responds to the #1061 issue.
I'd like to implement suggestion eviction at first. Then work on how to persist the status of suggestion #1062 (maybe through pv?).
@andreyvelich @johnugeorge @richardsliu @hougangliu @gaocegege

kubeflow-bot · 2020-04-16T08:29:15Z

This change is

andreyvelich

Thank you for doing this!
I left few comments.

andreyvelich · 2020-04-16T14:59:59Z

pkg/apis/controller/experiments/v1alpha3/experiment_types.go

@@ -49,9 +49,11 @@ type ExperimentSpec struct {

 	NasConfig *NasConfig `json:"nasConfig,omitempty"`

+	// If false/true, which means delete/resume Suggestion after experiment is finished
+	ResumeExperiment bool `json:"resumeExperiment,omitempty"`


Can we use this parameter in the future PRs also for #1062?
For example:

ResumeExperiment: "never"- Always delete Suggestion deployment after Experiment is finished and never restart it.
ResumeExperiment: "alwaysRunning" - Suggestion deployment is in always running mode.
ResumeExperiment: "fromPersistentVolume" - Suggestion deployment is deleted after Experiment is finished and we can restore it from PV.

This is just my thought, we can think about better naming.
/cc @gaocegege @johnugeorge @richardsliu

@andreyvelich Roughly, this design sounds to me. There are a few things I want to double check:

According to bottom comment, when ResumeExperiment: "never" , suggestion instance itself will still be alive with terminated status after experiment who owns it finished. We only evict deployment and service.

If ResumeExperiment: "fromPersistentVolume" , we will try to retrieve suggestion status from PV when initializing suggestion instance. And we will preserve the suggestion instance into PV after the termination of experiment.

@sperlingxx Yeah, it is correct.
As I mentioned bellow, should Suggestion CR be always running? What do you think?
Even if we want to delete deployment after Experiment is finished, Suggestion CR still has useful information for the user.

@andreyvelich I agree on keeping Suggestion CR. It may be useful, and it will be deleted after the remove of experiment CR who owns it.

andreyvelich · 2020-04-16T15:00:12Z

pkg/apis/controller/experiments/v1alpha3/experiment_types.go

@@ -49,9 +49,11 @@ type ExperimentSpec struct {

 	NasConfig *NasConfig `json:"nasConfig,omitempty"`

+	// If false/true, which means delete/resume Suggestion after experiment is finished
+	ResumeExperiment bool `json:"resumeExperiment,omitempty"`


Can we use this parameter in the future PRs also for #1062?
For example:

ResumeExperiment: "never"- Always delete Suggestion deployment after Experiment is finished and never restart it.
ResumeExperiment: "alwaysRunning" - Suggestion deployment is in always running mode.
ResumeExperiment: "fromPersistentVolume" - Suggestion deployment is deleted after Experiment is finished and we can restore it from PV.

This is just my thought, we can think about better naming.
What do you think @sperlingxx ?
/cc @gaocegege @johnugeorge @richardsliu

I think it is already updated.

pkg/controller.v1alpha3/experiment/experiment_controller.go

pkg/controller.v1alpha3/experiment/experiment_util.go

test/e2e/v1alpha3/resume-e2e-experiment.go

test/e2e/v1alpha3/run-e2e-experiment.go

gaocegege · 2020-04-17T10:09:58Z

@andreyvelich request another review, thanks.

andreyvelich

Thank you for this change. I left few comments

andreyvelich · 2020-04-17T10:37:16Z

pkg/apis/controller/experiments/v1alpha3/experiment_types.go

+
+const (
+	NeverResume ResumePolicyType = "Never"
+	LongRunning ResumePolicyType = "LongRunning"


I like the name ResumePolicy.
What do you think about the name LongRunning? Is it better than AlwaysRunning?
/cc @johnugeorge @gaocegege @richardsliu

@gaocegege @johnugeorge @richardsliu Do you have any ideas about the naming?

To make it more consistent, how about keeping options to "Never" and "Always"

@johnugeorge As which proposed in #1062 , there will be the third policy type representing "resuming experiment from persistent format"

I agree with @sperlingxx.
In case when Experiment is not deleted and always running, ResumeExperiment: Always sounds not very obviously.

SGTM. I think we can use LongRunning

pkg/controller.v1alpha3/suggestion/suggestion_controller.go

pkg/controller.v1alpha3/suggestion/suggestion_controller_util.go

pkg/controller.v1alpha3/experiment/experiment_util.go

pkg/apis/controller/experiments/v1alpha3/constants.go

gaocegege · 2020-04-27T02:15:35Z

@johnugeorge @andreyvelich Any other suggestion?

gaocegege

/lgtm

johnugeorge · 2020-04-28T07:33:11Z

Sorry for being late.
One minor change.
@sperlingxx Can you add resume policy to NeverResume to one or more of our examples so that it is tested with our presubmit tests? Since Resume policy is not specified in our tests, only default of LongRunning will be tested currently.

sperlingxx · 2020-04-29T02:25:55Z

Sorry for being late.
One minor change.
@sperlingxx Can you add resume policy to NeverResume to one or more of our examples so that it is tested with our presubmit tests? Since Resume policy is not specified in our tests, only default of LongRunning will be tested currently.

Good idea! But I'm busy with other business these days, I think I can offer the test case in early (maybe mid) May.

andreyvelich · 2020-04-29T14:30:51Z

I think we can merge this PR and add e2e test in the future PR.
What do you think @gaocegege @johnugeorge ?

gaocegege · 2020-04-30T01:57:05Z

Of course. I think we can.

/cc @johnugeorge

johnugeorge · 2020-04-30T09:39:28Z

Sounds good
/approve

k8s-ci-robot · 2020-04-30T09:39:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnugeorge

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [johnugeorge]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

) * Delete Suggestion deployment after Experiment is finished * fix * update * update openapi * fix validator test * refine * add debug info * try to fix * try to fix * try to fix * keep API style consistent * rebase master

k8s-ci-robot requested review from Akado2009 and garganubhav April 16, 2020 08:29

k8s-ci-robot added the size/M label Apr 16, 2020

sperlingxx changed the title ~~[solve #1061] Delete Suggestion deployment after Experiment is finished~~ Delete Suggestion deployment after Experiment is finished Apr 16, 2020

andreyvelich reviewed Apr 16, 2020

View reviewed changes

k8s-ci-robot requested review from gaocegege, johnugeorge and richardsliu April 16, 2020 15:03

k8s-ci-robot added size/L and removed size/M labels Apr 17, 2020

gaocegege reviewed Apr 17, 2020

View reviewed changes

test/e2e/v1alpha3/resume-e2e-experiment.go Outdated Show resolved Hide resolved

test/e2e/v1alpha3/run-e2e-experiment.go Show resolved Hide resolved

andreyvelich reviewed Apr 17, 2020

View reviewed changes

k8s-ci-robot requested a review from gaocegege April 17, 2020 10:50

andreyvelich reviewed Apr 17, 2020

View reviewed changes

pkg/controller.v1alpha3/experiment/experiment_util.go Outdated Show resolved Hide resolved

andreyvelich reviewed Apr 17, 2020

View reviewed changes

pkg/controller.v1alpha3/experiment/experiment_util.go Outdated Show resolved Hide resolved

andreyvelich reviewed Apr 17, 2020

View reviewed changes

pkg/apis/controller/experiments/v1alpha3/constants.go Outdated Show resolved Hide resolved

sperlingxx added 12 commits April 18, 2020 22:28

Delete Suggestion deployment after Experiment is finished

d8f8814

fix

3e12bda

update

b5c3335

update openapi

dbf6e47

fix validator test

ec70bca

refine

37980da

add debug info

c596a3d

try to fix

a615ffa

try to fix

d7f4a34

try to fix

d6ffa59

keep API style consistent

aa20c1b

rebase master

590deaf

sperlingxx force-pushed the delete_suggestion_after_finished branch from 4d35ba6 to 590deaf Compare April 18, 2020 14:30

gaocegege reviewed Apr 27, 2020

View reviewed changes

k8s-ci-robot assigned gaocegege Apr 27, 2020

k8s-ci-robot added the lgtm label Apr 27, 2020

johnugeorge mentioned this pull request Apr 28, 2020

Persist meta data of experiments #1128

Closed

k8s-ci-robot added the approved label Apr 30, 2020

k8s-ci-robot merged commit 58e0764 into kubeflow:master Apr 30, 2020

andreyvelich mentioned this pull request Jun 8, 2020

Katib cut release for Kubeflow 1.1 #1211

Closed

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete Suggestion deployment after Experiment is finished #1150

Delete Suggestion deployment after Experiment is finished #1150

sperlingxx commented Apr 16, 2020 •

edited

Loading

kubeflow-bot commented Apr 16, 2020

andreyvelich left a comment

andreyvelich Apr 16, 2020

sperlingxx Apr 17, 2020

andreyvelich Apr 17, 2020

sperlingxx Apr 17, 2020

andreyvelich Apr 16, 2020

gaocegege Apr 17, 2020

gaocegege commented Apr 17, 2020

andreyvelich left a comment

andreyvelich Apr 17, 2020

andreyvelich Apr 20, 2020

johnugeorge Apr 20, 2020

sperlingxx Apr 21, 2020

andreyvelich Apr 21, 2020

gaocegege Apr 27, 2020

gaocegege commented Apr 27, 2020

gaocegege left a comment

johnugeorge commented Apr 28, 2020

sperlingxx commented Apr 29, 2020 •

edited

Loading

andreyvelich commented Apr 29, 2020

gaocegege commented Apr 30, 2020

johnugeorge commented Apr 30, 2020

k8s-ci-robot commented Apr 30, 2020

Delete Suggestion deployment after Experiment is finished #1150

Delete Suggestion deployment after Experiment is finished #1150

Conversation

sperlingxx commented Apr 16, 2020 • edited Loading

kubeflow-bot commented Apr 16, 2020

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaocegege commented Apr 17, 2020

andreyvelich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaocegege commented Apr 27, 2020

gaocegege left a comment

Choose a reason for hiding this comment

johnugeorge commented Apr 28, 2020

sperlingxx commented Apr 29, 2020 • edited Loading

andreyvelich commented Apr 29, 2020

gaocegege commented Apr 30, 2020

johnugeorge commented Apr 30, 2020

k8s-ci-robot commented Apr 30, 2020

sperlingxx commented Apr 16, 2020 •

edited

Loading

sperlingxx commented Apr 29, 2020 •

edited

Loading