Suggestion volume resumePolicy override #1992

a9p · 2022-11-01T19:12:59Z

What this PR does / why we need it:

PBT currently relies on resumePolicy to be FromVolume to run properly. The proposed changes follow up on previous discussion on removing the requirement for users specifying this field and instead handling the logic automatically for suggestions that require it.

Which issue(s) this PR fixes:
Fixes #1893

Checklist:

Docs included if any changes are user facing

a9p · 2022-11-01T19:13:27Z

cc/ @andreyvelich was this closer to what you were looking for with your original comment?

coveralls · 2022-11-01T19:16:36Z

Coverage decreased (-0.1%) to 73.398% when pulling 3f1eba0 on a9p:reconcile into 6b55540 on kubeflow:master.

tenzen-y

@a9p Thanks for driving this!
I left a few comments.

pkg/controller.v1beta1/suggestion/suggestion_controller.go

tenzen-y · 2022-11-03T03:32:58Z

@a9p please get in touch with me when this PR is ready for review.

a9p · 2022-11-04T22:23:09Z

@tenzen-y thanks for the suggestion! I think this PR is ready for your review now -- let me know if you have any other comments!

tenzen-y

@a9p Thanks for updating this PR! This feature looks good to me.

tenzen-y · 2022-11-07T18:04:46Z

pkg/controller.v1beta1/suggestion/suggestion_controller.go

+	// If ResumePolicy = FromVolume (or overriden), volume is reconciled for suggestion
+	if suggestionConfigData.VolumeForceMount || instance.Spec.ResumePolicy == experimentsv1beta1.FromVolume {


Can you add the test for this logic?

andreyvelich

Thanks for this @a9p.
I left few suggestions.

andreyvelich · 2022-11-08T15:09:34Z

pkg/util/v1beta1/katibconfig/config.go

@@ -42,6 +42,7 @@ type SuggestionConfig struct {
 	PersistentVolumeClaimSpec corev1.PersistentVolumeClaimSpec `json:"persistentVolumeClaimSpec,omitempty"`
 	PersistentVolumeSpec      corev1.PersistentVolumeSpec      `json:"persistentVolumeSpec,omitempty"`
 	PersistentVolumeLabels    map[string]string                `json:"persistentVolumeLabels,omitempty"`
+	VolumeForceMount          bool                             `json:"volumeForceMount,omitempty"`


Can we avoid additional VolumeForceMount parameter in config ?

One suggestion that I have: Always get desired volume if Suggestion Config Data contains Persistent Volume Spec or if Resume Policy = From Volume.
The default values for PVC, we can set under the Desired Volume function.

I understand that it doesn't solve the problem when user wants to use the default values for Suggestion volume and still attach the volume, but we can find the better solution.

Any other ideas how to avoid more parameters in Katib Config @a9p @tenzen-y @johnugeorge ?

Always get desired volume if Suggestion Config Data contains Persistent Volume Spec or if Resume Policy = From Volume.
The default values for PVC, we can set under the Desired Volume function.

Maybe, it's idea is better to avoid more parameters in katib-config.

Any other ideas how to avoid more parameters in Katib Config

I have no good ideas.

I agree with removing the new parameter 👍🏽

I'm not sure what a good default is for the mounts or if there is one -- it seems like that would be setup dependent. One thought would be to prevent the suggestion service from starting if the mount doesn't exist in the config (making the config issue apparent up front) for services that require one? Currently, PBT for example, basically does a random search at the initial seed until run limits are hit which is a bit opaque unless you read the documentation for the config needed to operate correctly.

One thought would be to prevent the suggestion service from starting if the mount doesn't exist in the config (making the config issue apparent up front) for services that require one?

@a9p How we are going to understand for which Suggestion service the Volume is mandatory ?

I was originally thinking something like this, but hardcoding this in the suggestion controller isn't ideal. Possibly linking the required resources to the suggestion service and reading that in at the controller (e.g. an api change to the rpc endpoint implemented by the suggestion defaulting to no additional requirements)?

Hi @andreyvelich / @tenzen-y ! Curious if either of you have any thoughts on the above?

@a9p Apologies for the late reply, please can we discuss it again after the Kubeflow release (e.g. after 2 weeks) ?

@andreyvelich / @tenzen-y -- just following up if either of you have any thoughts on the best way to include the k8s spec in this repo?

Sorry for the late response. I'm missing the notifications. I came back to this thread right now.
I'll check your changes.

is git submoduling in the manager/v1beta1 directory the preferred approach? I put a temporary shim in the build.sh tying it to the minor k8s version just to verify static checks, but the code should still fail at runtime since it's deleting the directory.

@a9p Thanks for your investigating.
Can we store the proto definition for the k8s.io/apps/v1 only when generating codes? I guess we can store the k8s proto file locally like the following.

https://github.com/kubeflow/mpi-operator/blob/c034b8caf613b2951aa2550c32f1333c58f56a39/Makefile#L170-L172

I guess that we can do that with GOPATH=/tmp go install k8s.io/api.
I appreciate your effort!

andreyvelich · 2022-11-08T15:22:06Z

pkg/controller.v1beta1/suggestion/suggestion_controller.go

+	}
+
+	// If ResumePolicy = FromVolume (or overriden), volume is reconciled for suggestion
+	if suggestionConfigData.VolumeForceMount || instance.Spec.ResumePolicy == experimentsv1beta1.FromVolume {


I think we need to identify more places where we check if Suggestion has volume and we need to do something.
e.g. Controller deletes volume when Experiment is finished:

katib/pkg/controller.v1beta1/experiment/experiment_controller_util.go

Line 168 in 3f1eba0

} else if instance.Spec.ResumePolicy == experimentsv1beta1.FromVolume {

Maybe we can update the Suggestion CR's API with volumeAttached: bool in addition to resumePolicy.
So we can do appropriate changes based on this spec setting.
We are creating Suggestion CR before reconcile Suggestion, so you don't need to get Katib Config before reconcile Suggestion volume.

What do you think about this API change @tenzen-y @johnugeorge @a9p ?

What are the conditions under which katib-controller sets volumeAttached to true? Do you plan to do only when Spec.ResumePolicy == experimentsv1beta1.FromVolume? Or also check whether status.phase == Bound and so on in PVC?

One more case where it would be true is where the service needs a volume independent of ResumePolicy (this is what I think the original issue was pointing at separating)

…y for pbt

…po to enable protoc compilation. Go outputs look to correctly use upstream package.

…thmSpec.suggestion_spec (DeploymentSpec) or resumePolicy is FromVolume

a9p · 2023-06-04T08:04:32Z

/hold

andreyvelich

Hi @a9p, do you have any free time to complete this PR so we can merge it as part of our next release (Katib release-0.16)?

a9p · 2023-07-25T21:47:21Z

Hi @andreyvelich, it doesn't look like I will be able to wrap up this PR before feature freeze. Could we push this to the next release target? I should have a fair amount more time to work on this closer to October.

andreyvelich · 2023-07-26T12:05:18Z

Sure, no problem @a9p!
Thank you for your time

github-actions · 2023-10-24T15:05:21Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tenzen-y · 2023-10-28T11:26:19Z

@a9p Hi, are you still here?

github-actions · 2024-01-26T20:05:13Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tenzen-y · 2024-01-26T20:29:02Z

/remove-lifecycle stale

Still, we should move this forward.

github-actions · 2024-04-26T00:19:05Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2024-05-16T00:19:10Z

This pull request has been automatically closed because it has not had recent activity. Please comment "/reopen" to reopen it.

google-oss-prow · 2024-05-16T12:45:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tenzen-y for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

github-actions · 2024-08-14T15:05:10Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

andreyvelich · 2024-08-14T16:14:53Z

/remove-lifecycle stale
/help
/good-first-issue

google-oss-prow bot added the do-not-merge/work-in-progress label Nov 1, 2022

google-oss-prow bot requested review from anencore94 and tenzen-y November 1, 2022 19:13

google-oss-prow bot added the size/S label Nov 1, 2022

tenzen-y reviewed Nov 2, 2022

View reviewed changes

pkg/controller.v1beta1/suggestion/suggestion_controller.go Outdated Show resolved Hide resolved

a9p marked this pull request as ready for review November 4, 2022 08:16

google-oss-prow bot removed the do-not-merge/work-in-progress label Nov 4, 2022

google-oss-prow bot requested review from sperlingxx and tenzen-y November 4, 2022 08:17

tenzen-y reviewed Nov 7, 2022

View reviewed changes

andreyvelich reviewed Nov 8, 2022

View reviewed changes

a9p force-pushed the reconcile branch from 3f1eba0 to 38c9f3d Compare January 27, 2023 21:57

google-oss-prow bot added size/XL and removed size/S labels Jan 27, 2023

andreyvelich mentioned this pull request May 18, 2023

[Release] Katib 0.16 Release #2156

Closed

6 tasks

andreyvelich added the release/0.16 label May 18, 2023

a9p added 8 commits June 3, 2023 16:50

feat: support reconciling suggestion volume regardless of resumePolic…

93942c5

…y for pbt

feat: add volumeForceMount option to SuggestionConfig

4d6f319

wip: add DeploymentSpec to AlgorithmSpec. Clones k8s 1.22.17 proto re…

f8aacd1

…po to enable protoc compilation. Go outputs look to correctly use upstream package.

wip: add deployment spec validation to pbt suggestion service

950f4c5

wip: reconcile suggestion volume if pvc override specified via Algori…

ec7b5ad

…thmSpec.suggestion_spec (DeploymentSpec) or resumePolicy is FromVolume

wip: using pvc definitions from cm; added deploymentspec to common types

916ffe7

chore: gofmt, lint

9dcbd9f

wip: use go.mod installed api and apimachinery to generate proto files

8da4db2

a9p force-pushed the reconcile branch from 26a7526 to 8da4db2 Compare June 4, 2023 08:03

a9p marked this pull request as draft June 4, 2023 08:06

google-oss-prow bot added do-not-merge/work-in-progress size/XXL and removed size/XL labels Jun 4, 2023

a9p force-pushed the reconcile branch from 2bc5f5c to decd414 Compare June 5, 2023 05:49

feat: patch python sdk target

2fa50ad

a9p force-pushed the reconcile branch from a03f4bc to 2fa50ad Compare June 11, 2023 03:22

andreyvelich reviewed Jul 18, 2023

View reviewed changes

andreyvelich removed the release/0.16 label Jul 26, 2023

github-actions bot added the lifecycle/stale label Oct 24, 2023

github-actions bot removed the lifecycle/stale label Oct 28, 2023

github-actions bot added the lifecycle/stale label Jan 26, 2024

google-oss-prow bot removed the lifecycle/stale label Jan 26, 2024

github-actions bot added the lifecycle/stale label Apr 26, 2024

github-actions bot closed this May 16, 2024

andreyvelich reopened this May 16, 2024

github-actions bot removed the lifecycle/stale label May 16, 2024

github-actions bot added the lifecycle/stale label Aug 14, 2024

google-oss-prow bot removed the lifecycle/stale label Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggestion volume resumePolicy override #1992

Suggestion volume resumePolicy override #1992

a9p commented Nov 1, 2022

a9p commented Nov 1, 2022

coveralls commented Nov 1, 2022 •

edited

Loading

tenzen-y left a comment

tenzen-y commented Nov 3, 2022

a9p commented Nov 4, 2022

tenzen-y left a comment

tenzen-y Nov 7, 2022

andreyvelich left a comment

andreyvelich Nov 8, 2022

tenzen-y Nov 16, 2022

a9p Nov 30, 2022

andreyvelich Dec 1, 2022

a9p Dec 1, 2022

a9p Mar 3, 2023 •

edited

Loading

andreyvelich Mar 3, 2023

a9p Apr 26, 2023

tenzen-y Apr 26, 2023

tenzen-y May 2, 2023

andreyvelich Nov 8, 2022

tenzen-y Nov 16, 2022

a9p Nov 30, 2022

a9p commented Jun 4, 2023

andreyvelich left a comment

a9p commented Jul 25, 2023

andreyvelich commented Jul 26, 2023

github-actions bot commented Oct 24, 2023

tenzen-y commented Oct 28, 2023

github-actions bot commented Jan 26, 2024

tenzen-y commented Jan 26, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented May 16, 2024

google-oss-prow bot commented May 16, 2024

github-actions bot commented Aug 14, 2024

andreyvelich commented Aug 14, 2024

		// If ResumePolicy = FromVolume (or overriden), volume is reconciled for suggestion
		if suggestionConfigData.VolumeForceMount \|\| instance.Spec.ResumePolicy == experimentsv1beta1.FromVolume {

Suggestion volume resumePolicy override #1992

Are you sure you want to change the base?

Suggestion volume resumePolicy override #1992

Conversation

a9p commented Nov 1, 2022

a9p commented Nov 1, 2022

coveralls commented Nov 1, 2022 • edited Loading

tenzen-y left a comment

Choose a reason for hiding this comment

tenzen-y commented Nov 3, 2022

a9p commented Nov 4, 2022

tenzen-y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

a9p Mar 3, 2023 • edited Loading

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

Choose a reason for hiding this comment

a9p commented Jun 4, 2023

andreyvelich left a comment

Choose a reason for hiding this comment

a9p commented Jul 25, 2023

andreyvelich commented Jul 26, 2023

github-actions bot commented Oct 24, 2023

tenzen-y commented Oct 28, 2023

github-actions bot commented Jan 26, 2024

tenzen-y commented Jan 26, 2024

github-actions bot commented Apr 26, 2024

github-actions bot commented May 16, 2024

google-oss-prow bot commented May 16, 2024

github-actions bot commented Aug 14, 2024

andreyvelich commented Aug 14, 2024

coveralls commented Nov 1, 2022 •

edited

Loading

a9p Mar 3, 2023 •

edited

Loading