Skip to content

Bug 2033751: Return Error when trying to use Scheduler Policy #390

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

Merged
openshift-merge-robot merged 1 commit into from
Jan 22, 2022
Merged

Bug 2033751: Return Error when trying to use Scheduler Policy #390

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 4 additions & 8 deletions pkg/operator/targetconfigcontroller/targetconfigcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,10 @@ func manageKubeSchedulerConfigMap_v311_00_to_latest(ctx context.Context, client
if err != nil {
return nil, false, err
}
// TOOD(jchaloup): remove the condition once the policy API is removed from the code
// Until that, ignore profiles if the policy API is used.
// Scheduler Policy API has been removed in k8s 1.23
// Still keep the CRD field for compatibility, but it does nothing now
if len(config.Spec.Policy.Name) > 0 {
kubeSchedulerConfiguration = bindata.MustAsset("assets/config/defaultconfig-postbootstrap-lownodeutilization.yaml")
return nil, false, fmt.Errorf("scheduler Policy config has been removed upstream, this field remains for CRD compatibility but does nothing now. Please use a Profile instead (defaulting to LowNodeUtilization).")
} else {
switch config.Spec.Profile {
case v1.LowNodeUtilization, "":
Expand Down Expand Up @@ -253,10 +253,6 @@ func managePod_v311_00_to_latest(ctx context.Context, configMapsGetter corev1cli
if err != nil {
return nil, false, err
}
if len(config.Spec.Policy.Name) > 0 {
required.Spec.Containers[0].Args = append(required.Spec.Containers[0].Args, "--policy-configmap=policy-configmap")
required.Spec.Containers[0].Args = append(required.Spec.Containers[0].Args, fmt.Sprintf("--policy-configmap-namespace=%s", operatorclient.TargetNamespace))
}

// there's no need to convert UnsupportedConfigOverrides.Raw from yaml to json as it always comes as json.
// passing no data to Unmarshal has unexpected behaviour and could be avoided by examining the length of the input.
Expand Down Expand Up @@ -301,7 +297,7 @@ func managePod_v311_00_to_latest(ctx context.Context, configMapsGetter corev1cli
configMap.Data["version"] = version.Get().String()
appliedConfigMap, changed, err := resourceapply.ApplyConfigMap(ctx, configMapsGetter, recorder, configMap)
if changed && len(config.Spec.Policy.Name) > 0 {
Copy link
Copy Markdown
Member

@ingvagabund ingvagabund Jan 6, 2022

Choose a reason for hiding this comment

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

With manageKubeSchedulerConfigMap_v311_00_to_latest returning error when len(config.Spec.Policy.Name) > 0, the kube-scheduler container will keep crash looping since the configmap with the profile will be missing (assuming the scheduler/cluster object was specified with .spec.policy.name set during the cluster provisioning). I wonder if you took this case into account and whether it would make more sense to move len(config.Spec.Policy.Name) > 0 check alongside the config, err := configSchedulerLister.Get("cluster") line at the beginning of the managePod_v311_00_to_latest function and return an error as well so the pod does not get created and kept crash looping until the policy field is cleared?

The case covers the bootstrapping phase in which (when .spec.policy.name is not empty) the installation fails. So it is unlikely any admin will update the scheduler/cluster object rather than running the installation again. So, the net benefit for the normal installation is quite low. On the other hand when the hypershift topology is used (with one cluster hosting many control planes), postponing the creation of the kube-scheduler pod might safe the step of debugging why the pod is crash looping (depending on how the control plan is provisioned).

Copy link
Copy Markdown
Member

@ingvagabund ingvagabund Jan 19, 2022

Choose a reason for hiding this comment

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

My comment is more for the debugging purposes than the functional/conceptual ones. The operator will go degraded when the policy field is set.

klog.Warning("Setting .spec.policy is deprecated and will be removed eventually. Please use .spec.profile instead.")
klog.Warning("Setting .spec.policy is no longer supported. Please use .spec.profile instead.")
}
return appliedConfigMap, changed, err
}
Expand Down