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.

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

begin to deprecate integrations.podOptions #4168

Open
dgrove-oss opened this issue Feb 6, 2025 · 7 comments · May be fixed by #4256
Open

begin to deprecate integrations.podOptions #4168

dgrove-oss opened this issue Feb 6, 2025 · 7 comments · May be fixed by #4256
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@dgrove-oss
Copy link
Contributor

What would you like to be cleaned:

Per the discussion in #3589, we want to migrate users from using integrations.podOptions to managedJobsNamespaceSelector.
For Kueue 0.11 we should take the next step in the process. This goal of this issue is to decide how far we can go in 0.11.

Some possible options:

  1. We just emit a warning message when the option is set to a non-default value.
  2. We emit a configuration error if it is set to a non-default value and remove all the code that processes it (but leave it defined by unused in the struct).
  3. Some intermediate point to be defined between these extremes?

Why is this needed:

We want to not carry integrations.podOptions into the next API version of the Configuration struct. We should start encouraging people to migrate away from it.

@dgrove-oss dgrove-oss added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Feb 6, 2025
@dgrove-oss
Copy link
Contributor Author

/cc @tenzen-y @mimowo

@kannon92
Copy link
Contributor

kannon92 commented Feb 7, 2025

Kubernetes tend to follow relatively conservative approachs for deprecations.

I think the following plan follows k8s practices:

  • 0.11 can begin marking the field as deprecated
    • We can emit a warning or event that this configuration option is deprecated and will be removed.
    • Add a comment that the field is deprecated.
  • 0.12 or 0.13 we could remove the option since our config API is still beta.

If config api went v1 we would probably leave the field as deprecated but not remove.

@tenzen-y
Copy link
Member

tenzen-y commented Feb 7, 2025

0.12 or 0.13 we could remove the option since our config API is still beta.

AFAIK, Beta API (not Beta feature) represents not to break API, right. So, we need to bump API version to v1beta2 instead of removal of fields.

Could you point the policy which mention allowing us to break Beta API? My beta API understanding might be not correct.

@kannon92
Copy link
Contributor

kannon92 commented Feb 7, 2025

I don’t know if there is anything for beta written down like that. I know that we dropped an API field for Swap in Beta that we decided was too risky to support. It was understood that this was the best option.

@dgrove-oss
Copy link
Contributor Author

/assign

@mimowo
Copy link
Contributor

mimowo commented Feb 7, 2025

I don’t know if there is anything for beta written down like that.

Actually, I think API elements (such as fields) should never be removed within API version based on https://kubernetes.io/docs/reference/using-api/deprecation-policy/.

"Rule #1: API elements may only be removed by incrementing the version of the API group."

We can remove fields when incrementing the API version.

I know that we dropped an API field for Swap in Beta that we decided was too risky to support. It was understood that this was the best option.

I don't know this specific case, but I can well imagine under some circumstances this is the most rational choice and this is why we have the pragmatic Exceptions.

I believe in case of integrations.podOptions there is no urgency regarding its removal. I would suggest to depracate it -review all docs and API comments to make sure it is not suggested to users / admins. Also, make it clear to users that it will be removed on the next API update. Then, we will remove it when updating the API.

@tenzen-y
Copy link
Member

tenzen-y commented Feb 8, 2025

I don’t know if there is anything for beta written down like that.

Actually, I think API elements (such as fields) should never be removed within API version based on https://kubernetes.io/docs/reference/using-api/deprecation-policy/.

"Rule #1: API elements may only be removed by incrementing the version of the API group."

We can remove fields when incrementing the API version.

I know that we dropped an API field for Swap in Beta that we decided was too risky to support. It was understood that this was the best option.

I don't know this specific case, but I can well imagine under some circumstances this is the most rational choice and this is why we have the pragmatic Exceptions.

I believe in case of integrations.podOptions there is no urgency regarding its removal. I would suggest to depracate it -review all docs and API comments to make sure it is not suggested to users / admins. Also, make it clear to users that it will be removed on the next API update. Then, we will remove it when updating the API.

Thank you for pointing the deprecation documentation. Your recommended step is what I imagined when we stopped serving fields.

@dgrove-oss dgrove-oss linked a pull request Feb 13, 2025 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants