v1beta2: Ignore "enable: true" in the configMap for FairSharing and WaitForPodsReady

[mimowo]

For v1beta2 we have removed the enable field for FairSharing and WaitForPodsReady in the PRs:

• v1beta2: Delete .enable field from WaitForPodsReady API in config #7628
• v1beta2: Delete .enable field from FairSharing API in config #7583

as part of #7113

However, this means that users may experience a complication on upgrade if they forget to remove "enable: true" it will not get parsed.

I would like to parse the configMap, but specifically ignore "enable: true" for the features without error. We could just log a warning.

[mimowo]

This is inspired by discussion with @mwielgus
cc @PBundyra @tenzen-y

/assign @mbobrovskyi
who I know is already looking into that

[mimowo]

cc @kannon92

[tenzen-y]

@mimowo I'm wondering if we should keep the enable field as I mentioned in #7628.
The primary reason is that the way to enable waitForPodsReady and FairSharing with the default value is waitForPodsReady: {} / fairSharing: {}.

For waitForPodsReady, we can just re add enable field, but for fairSharing, we probably should revisit object structure to avoid confusion when they want to enable fairSharing and AFS.

cc @mbobrovskyi

[mimowo]

tbh I dont see any difference between waitForPodsRady and fairSharing in that regard. So I would propose:

1. find a way to ignore enable as mentioned in the Issue for both features
2. rollback the PRs for both features

I dont have a strong view. As I synced with @mwielgus his opinion was also to do one of the two

[mimowo]

We discussed this yesterday at wg-batch, and the sentiment was that (1.) was great, but there was no objection about the change anyway.

So, seems like we wouldn't need to rollback the change even if we don't find a solution for (1.).

[tenzen-y]

We discussed this yesterday at wg-batch, and the sentiment was that (1.) was great, but there was no objection about the change anyway.

@mimowo I was offline at the beginning of the meeting. So, I'm still wondering if we should re introduce enabled field to waitForPodsReady, fairSharing, and asmissionFairSharing for enabling those features with default parameters.

waitForPodsReady:
  enabled: true
...
fairSharing:
  enabled: true
...
admissionFairSharing:
  enabled: true
...

In the current v1beta2, users need to have a wierd value {} for enabling functionalities with default parameters:

waitForPodsReady: {}
...
fairSharing: {}
...
admissionFairSharing: {}
...

[mimowo]

So, I'm still wondering if we should re introduce enabled field to waitForPodsReady, fairSharing, and asmissionFairSharing for enabling those features with default parameters.

The fields used to be named "enable", so I wouldn't rename to "enabled", because this still means complications on upgrade.

In the current v1beta2, users need to have a wierd value {} for enabling functionalities with default parameters:

I don't think this is a real issue though. In my practice I haven't seen a deployment like

waitForPodsReady:
  enable: true

relying just on defaults. In all cases I've seen people would uncomment https://github.com/kubernetes-sigs/kueue/blob/main/config/components/manager/controller_manager_config.yaml#L26-L34, and tweak some values.

I for users how really want use defaults, we could also just add a comment to use waitForPodsReady: {}, but I think such users are really limited. Admins usually need to tweak the values anyway.

[mimowo]

Also note that we have many other features like: DeviceClassMappings, MultiKueue, WorkloadRetentionPolicy etc. which don't use enable(d) field, but potentially could. So I think it is more consistent to drop the enable field all together for all features.

[tenzen-y]

I discussed these fields with @mimowo offline.
As a conclusion, we decided to make .waitForPodsReady.timeout and .fairSharing.preemptionStrategies required fields.

• kueue/apis/config/v1beta2/configuration_types.go

  Line 224 in 8878f88

  Timeout *metav1.Duration `json:"timeout,omitempty"`

• kueue/apis/config/v1beta2/configuration_types.go

  Line 515 in 8878f88

  PreemptionStrategies []PreemptionStrategy `json:"preemptionStrategies,omitempty"`

The waitForPodsReady and fairSharing fields represent how to enable those features and their optional parameters. (Mixed Responsibility)
But the multiKueue is responsible only for representing their optional parameters. (Single Responsibility)

So, I argued to introduce the enabled fields to waitForPodsReady and fairSharing.
But, as opposed to adding enabled fields, if we can make one field required, the field could be Single Responsibility since it will represent only their parameters, which could eliminate the possibility for waitForPodsReady: {} and fairSharing: {} when enabling those features.

[tenzen-y]

^ cc @mbobrovskyi

[mbobrovskyi]

SGTM