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.

Sign up for GitHub

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

More container fields for SuggestionConfig #2000

Merged
merged 7 commits into from
Jan 25, 2023
Merged

More container fields for SuggestionConfig #2000

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c6f87bd
More container fields for SuggestionConfig
fischor Nov 5, 2022
427f115
Inline corev1.Container into SuggestionConfig
fischor Nov 14, 2022
7f1b284
Set default value for suggestion container name
fischor Dec 17, 2022
5410bdc
Append suggestion volume and port only if not present
fischor Jan 15, 2023
92d0471
Deep-Copy base suggestion container
fischor Jan 15, 2023
caa08c8
Check for suggestion container port number as well
fischor Jan 15, 2023
6cc917e
Prohibit suggestion port to be set in suggestion config
fischor Jan 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 60 additions & 20 deletions pkg/controller.v1beta1/suggestion/composer/composer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,21 +181,30 @@ func (g *General) desiredContainers(s *suggestionsv1beta1.Suggestion,
suggestionConfigData katibconfig.SuggestionConfig,
earlyStoppingConfigData katibconfig.EarlyStoppingConfig) []corev1.Container {

containers := []corev1.Container{}
suggestionContainer := corev1.Container{
Name: consts.ContainerSuggestion,
Image: suggestionConfigData.Image,
ImagePullPolicy: suggestionConfigData.ImagePullPolicy,
Ports: []corev1.ContainerPort{
{
Name: consts.DefaultSuggestionPortName,
ContainerPort: consts.DefaultSuggestionPort,
},
},
Resources: suggestionConfigData.Resource,
var (
containers []corev1.Container
suggestionContainer corev1.Container
)

suggestionConfigData.Container.DeepCopyInto(&suggestionContainer)

// Assign default values for suggestionContainer fields that are not set via
// the suggestion config.

if suggestionContainer.Name == "" {
suggestionContainer.Name = consts.ContainerSuggestion
}

if viper.GetBool(consts.ConfigEnableGRPCProbeInSuggestion) {
if !containsContainerPortWithName(suggestionContainer.Ports, consts.DefaultSuggestionPortName) &&
!containsContainerPort(suggestionConfigData.Ports, consts.DefaultSuggestionPort) {
Copy link
Member

@andreyvelich andreyvelich Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should we verify that port name and containerPort is set together in the Katib Config ?
DefaultSuggestionPortName and DefaultSuggestionPort is always used for the Suggestion's service.
So if user set DefaultSuggestionPortName port with different address, the Suggestion service fails.

cc @tenzen-y

Copy link
Member

Choose a reason for hiding this comment

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

cc @johnugeorge

Copy link
Member

Choose a reason for hiding this comment

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

It might be better to prohibit setting containerPort in katibConfig.

Copy link
Member

Choose a reason for hiding this comment

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

We should add a validation check either ways.

Copy link
Member

@andreyvelich andreyvelich Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@fischor Do you have time to check my latest message ? We are getting closer for the Katib 0.15 release, and it would be great to include your feature in that release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andreyvelich I should have some spare time tomorrow. Do we do it as you suggested: verify that name and port are correct, otherwise return an error in DesiredDeployment method? Or prohibit the DefaultSuggestionPort to be set at all?

Copy link
Member

Choose a reason for hiding this comment

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

@fischor Thank you! Let's prohibit to set DefaultSuggestionPort for now as @tenzen-y suggested. So we can avoid errors from the user side.

Copy link
Member

Choose a reason for hiding this comment

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

@fischor We are planning to cut a release in a day. Do you have any update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@johnugeorge Thanks for the reminder...you can check the latest commit now.

suggestionPort := corev1.ContainerPort{
Name: consts.DefaultSuggestionPortName,
ContainerPort: consts.DefaultSuggestionPort,
}
suggestionContainer.Ports = append(suggestionContainer.Ports, suggestionPort)
}

if viper.GetBool(consts.ConfigEnableGRPCProbeInSuggestion) && suggestionContainer.ReadinessProbe == nil {
suggestionContainer.ReadinessProbe = &corev1.Probe{
ProbeHandler: corev1.ProbeHandler{
Exec: &corev1.ExecAction{
Expand All @@ -209,6 +218,8 @@ func (g *General) desiredContainers(s *suggestionsv1beta1.Suggestion,
InitialDelaySeconds: defaultInitialDelaySeconds,
PeriodSeconds: defaultPeriodForReady,
}
}
if viper.GetBool(consts.ConfigEnableGRPCProbeInSuggestion) && suggestionContainer.LivenessProbe == nil {
suggestionContainer.LivenessProbe = &corev1.Probe{
ProbeHandler: corev1.ProbeHandler{
Exec: &corev1.ExecAction{
Expand All @@ -226,15 +237,14 @@ func (g *General) desiredContainers(s *suggestionsv1beta1.Suggestion,
}
}

// Attach volume mounts to the suggestion container if ResumePolicy = FromVolume
if s.Spec.ResumePolicy == experimentsv1beta1.FromVolume {
suggestionContainer.VolumeMounts = []corev1.VolumeMount{
{
Name: consts.ContainerSuggestionVolumeName,
MountPath: suggestionConfigData.VolumeMountPath,
},
if s.Spec.ResumePolicy == experimentsv1beta1.FromVolume && !containsVolumeMountWithName(suggestionContainer.VolumeMounts, consts.ContainerSuggestionVolumeName) {
suggestionVolume := corev1.VolumeMount{
Name: consts.ContainerSuggestionVolumeName,
MountPath: suggestionConfigData.VolumeMountPath,
}
suggestionContainer.VolumeMounts = append(suggestionContainer.VolumeMounts, suggestionVolume)
}

containers = append(containers, suggestionContainer)

if s.Spec.EarlyStopping != nil && s.Spec.EarlyStopping.AlgorithmName != "" {
Expand All @@ -255,6 +265,36 @@ func (g *General) desiredContainers(s *suggestionsv1beta1.Suggestion,
return containers
}

func containsVolumeMountWithName(volumeMounts []corev1.VolumeMount, name string) bool {
for i := range volumeMounts {
if volumeMounts[i].Name == name {
return true
}
}

return false
}

func containsContainerPortWithName(ports []corev1.ContainerPort, name string) bool {
for i := range ports {
if ports[i].Name == name {
return true
}
}

return false
}

func containsContainerPort(ports []corev1.ContainerPort, port int32) bool {
for i := range ports {
if ports[i].ContainerPort == port {
return true
}
}

return false
}

// DesiredVolume returns desired PVC and PV for Suggestion.
// If PV doesn't exist in Katib config return nil for PV.
func (g *General) DesiredVolume(s *suggestionsv1beta1.Suggestion) (*corev1.PersistentVolumeClaim, *corev1.PersistentVolume, error) {
Expand Down
26 changes: 14 additions & 12 deletions pkg/controller.v1beta1/suggestion/composer/composer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,18 +646,20 @@ func newFakeSuggestionConfig() katibconfig.SuggestionConfig {
diskQ, _ := resource.ParseQuantity(disk)

return katibconfig.SuggestionConfig{
Image: image,
ImagePullPolicy: imagePullPolicy,
Resource: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceCPU: cpuQ,
corev1.ResourceMemory: memoryQ,
corev1.ResourceEphemeralStorage: diskQ,
},
Requests: corev1.ResourceList{
corev1.ResourceCPU: cpuQ,
corev1.ResourceMemory: memoryQ,
corev1.ResourceEphemeralStorage: diskQ,
Container: corev1.Container{
Image: image,
ImagePullPolicy: imagePullPolicy,
Resources: corev1.ResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceCPU: cpuQ,
corev1.ResourceMemory: memoryQ,
corev1.ResourceEphemeralStorage: diskQ,
},
Requests: corev1.ResourceList{
corev1.ResourceCPU: cpuQ,
corev1.ResourceMemory: memoryQ,
corev1.ResourceEphemeralStorage: diskQ,
},
},
},
ServiceAccountName: serviceAccount,
Expand Down
4 changes: 3 additions & 1 deletion pkg/controller.v1beta1/suggestion/suggestion_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,9 @@ func newKatibConfigMapInstance() *corev1.ConfigMap {
// Create suggestion config
suggestionConfig := map[string]katibconfig.SuggestionConfig{
"random": {
Image: suggestionImage,
Container: corev1.Container{
Image: suggestionImage,
},
},
}
bSuggestionConfig, _ := json.Marshal(suggestionConfig)
Expand Down
6 changes: 2 additions & 4 deletions pkg/util/v1beta1/katibconfig/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ import (

// SuggestionConfig is the JSON suggestion structure in Katib config.
type SuggestionConfig struct {
Image string `json:"image"`
ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"`
Resource corev1.ResourceRequirements `json:"resources,omitempty"`
corev1.Container `json:",inline"`
ServiceAccountName string `json:"serviceAccountName,omitempty"`
VolumeMountPath string `json:"volumeMountPath,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

@fischor Do we need VolumeMountPath if that is part of Container parameters:

katib/pkg/controller.v1beta1/suggestion/composer/composer.go

Line 237 in 7f1b284

if s.Spec.ResumePolicy == experimentsv1beta1.FromVolume && len(suggestionContainer.VolumeMounts) == 0 {
?

Copy link
Contributor Author

@fischor fischor Dec 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The current Katib SuggestionConfig has a JSON field volumeMountPath thats just a filepath, the corev1.Container has a json field volumeMounts thats an array of volume mounts. So volumeMountPath is not part if the container fields. I think in order to not break anything, we need it.

The referenced snippet kind of follows the logic:

... if parameters are set by the user we should use them, otherwise use the default values.

In case the volumeMounts are set we prefer that, otherwise mount the default suggestion volume under the specificed volumeMountPath.

Copy link
Member

Choose a reason for hiding this comment

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

I see, that is because we set the default value name for Suggestion Volume. Please can you leave comment that we should refactor VolumeMountPath parameter once we allow to set Deployment spec in our Suggestion config ?

Also, I think we should modify len(suggestionContainer.VolumeMounts) == 0 check.
We always should append VolumeMount if ResumePolicy is FromVolume.

if s.Spec.ResumePolicy == experimentsv1beta1.FromVolume {
volumeMount := corev1.VolumeMount{
	Name:      consts.ContainerSuggestionVolumeName,
	MountPath: suggestionConfigData.VolumeMountPath,
}
suggestionContainer.VolumeMounts = append(suggestionContainer.VolumeMounts, volumeMount)
}

WDYT @fischor ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andreyvelich Have you seen that the consts.DefaultSuggestionPortName port is also just set if the Container specified in the suggestion has no ports set? See https://github.com/fischor/katib/blob/7f1b2842f87158c25c54f577da1b82ddc7a958e3/pkg/controller.v1beta1/suggestion/composer/composer.go#L194-L201
Would you also change that to always append the default port?

I think both ways of doing this have pros/cons. If we always append the default volume/port then the user has no option to overwrite it. If we never append the default volume/port, then the user needs to be aware that it has to specify it when setting any volumes/ports.

A third option would be to check if the default volume/port is already set in the container and only append if its not. I think I would prefer that.

Copy link
Member

@andreyvelich andreyvelich Jan 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A third option would be to check if the default volume/port is already set in the container and only append if its not. I think I would prefer that.

I think that should work, but we need to make sure that volume and port name/number is correct (ContainerSuggestionVolumeName and DefaultSuggestionPortName/DefaultSuggestionPort ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andreyvelich I have added check to only append a VolumeMount for the Suggestion Volume only in case no volume mount with that default name ("suggestion-volume") exists. And a check to only append a ContainerPort for the Suggestion API in case no port with the default suggestion api port name and/or port number exists.

PersistentVolumeClaimSpec corev1.PersistentVolumeClaimSpec `json:"persistentVolumeClaimSpec,omitempty"`
Expand Down Expand Up @@ -98,7 +96,7 @@ func GetSuggestionConfigData(algorithmName string, client client.Client) (Sugges
suggestionConfigData.ImagePullPolicy = setImagePullPolicy(suggestionConfigData.ImagePullPolicy)

// Set resource requirements for suggestion
suggestionConfigData.Resource = setResourceRequirements(suggestionConfigData.Resource)
suggestionConfigData.Resources = setResourceRequirements(suggestionConfigData.Resources)

// Set default suggestion container volume mount path
if suggestionConfigData.VolumeMountPath == "" {
Expand Down
14 changes: 8 additions & 6 deletions pkg/util/v1beta1/katibconfig/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,13 @@ func TestGetSuggestionConfigData(t *testing.T) {
katibConfig: func() *katibConfig {
kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}}
kc.suggestion[testAlgorithmName].ImagePullPolicy = corev1.PullAlways
kc.suggestion[testAlgorithmName].Resource = *newFakeCustomResourceRequirements()
kc.suggestion[testAlgorithmName].Resources = *newFakeCustomResourceRequirements()
return kc
}(),
expected: func() *SuggestionConfig {
c := newFakeSuggestionConfig()
c.ImagePullPolicy = corev1.PullAlways
c.Resource = *newFakeCustomResourceRequirements()
c.Resources = *newFakeCustomResourceRequirements()
return c
}(),
inputAlgorithmName: testAlgorithmName,
Expand Down Expand Up @@ -107,7 +107,7 @@ func TestGetSuggestionConfigData(t *testing.T) {
testDescription: "GetSuggestionConfigData sets resource.requests and resource.limits for the suggestion service",
katibConfig: func() *katibConfig {
kc := &katibConfig{suggestion: map[string]*SuggestionConfig{testAlgorithmName: newFakeSuggestionConfig()}}
kc.suggestion[testAlgorithmName].Resource = corev1.ResourceRequirements{}
kc.suggestion[testAlgorithmName].Resources = corev1.ResourceRequirements{}
return kc
}(),
expected: newFakeSuggestionConfig(),
Expand Down Expand Up @@ -402,9 +402,11 @@ func newFakeSuggestionConfig() *SuggestionConfig {
defaultVolumeStorage, _ := resource.ParseQuantity(consts.DefaultSuggestionVolumeStorage)

return &SuggestionConfig{
Image: "suggestion-image",
ImagePullPolicy: consts.DefaultImagePullPolicy,
Resource: *setFakeResourceRequirements(),
Container: corev1.Container{
Image: "suggestion-image",
ImagePullPolicy: consts.DefaultImagePullPolicy,
Resources: *setFakeResourceRequirements(),
},
VolumeMountPath: consts.DefaultContainerSuggestionVolumeMountPath,
PersistentVolumeClaimSpec: corev1.PersistentVolumeClaimSpec{
AccessModes: []corev1.PersistentVolumeAccessMode{
Expand Down