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

Recommend runtimeclasses for scheduling #29062

Closed

Conversation

ravisantoshgudimetla
Copy link
Contributor

In future, we will enforce runtimeclasses to be used. Pods having just nodeSelector
and tolerations are going to be rejected during apiserver admission. This commit
ensures we recommend using runtimeclasses in the current release so that the
transition will be smooth for end-users when we make it mandatory in future releases

In future, we will enforce runtimeclasses to be used. Pods having just nodeSelector 
and tolerations are going to be rejected during apiserver admission. This commit
ensures we recommend using runtimeclasses in the current release so that the
transition will be smooth for end-users when we make it mandatory in future releases
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jul 21, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign tengqm after the PR has been reviewed.
You can assign the PR to them by writing /assign @tengqm in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 21, 2021
Comment on lines 222 to 224
Please note that this is the recommended way to schedule pods onto Windows nodes instead of using nodeSelector and
tolerations as they can be added to pod spec by any user. In future, we will enforce runtimeclasses to be used. Pods
having just nodeSelector and tolerations are going to be rejected during apiserver admission.
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest calling this out earlier under Taints and Tolerations. Otherwise the reader is going think this might come in only if they are using multiple Windows versions in the same cluster. IOW, move RuntimeClasses to the same level as Taints and Tolerations and reword appropriately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree to move RuntimeClasses to be a peer of Taints and Tolerations

Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the "In future, ..." statement. We don't predict/document future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's the style guide on future statements.

Maybe the heading should change? RuntimeClass stops being a simplification and is instead the primary, recommended way to define the OS-level scheduling constraints for Pods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reorganized the file to bring in some more clarity. PTAL

@tengqm @sftim - This is sort of a deprecation notice. So, it should be good as per the link you mentioned above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think should say anything about In future, we will enforce runtimeclasses to be used. Pods having just nodeSelector and tolerations are going to be rejected during apiserver admission. In this PR.

We should get approval of kubernetes/enhancements#2803 from all participating SIGs before saying anything definitive.

That's a fair argument. I'll remove that statement.

Also - because of this maybe we shouldn't say using runtime classes are recommended over nodeSelector generally.
If we can provide specific reasons why it is better (that we can commit to) great.
If not should we just document it as an alternative right now?

I think the argument is other way around, we know that the runtimeclasses are better, so we are proposing to make them authoritative in the linked enhancement.

I'll go with @sftim's suggestion that using runtimeclasses is recommended and better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. PTAL

Copy link
Contributor

Choose a reason for hiding this comment

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

I more meant that we shouldn't just say they are better but we should list some reasons why they are better.
Right now nodeSelectors are ubiquitous for identifying Windows pods I can see a lot of people asking why they should switch.
Since the KEP isn't approved and we cannot say nodeSelectors won't work in the future can we give some examples of why people should use them today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing that here. I also mentioned that runtimeclasses are created by cluster admin. That is the reason for the switch. Do you want me to add more? Something from historical context that runtimeclasses didn't exist back then...

Copy link
Contributor

Choose a reason for hiding this comment

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

As you also need to be a cluster admin to register a Windows node / configure automation to allow admission, and this is the production Windows setup guide, I think it's not only OK to recommend using a RuntimeClass for scheduling, I strongly think we should.

@netlify
Copy link

netlify bot commented Jul 21, 2021

✔️ Deploy Preview for kubernetes-io-main-staging ready!

🔨 Explore the source changes: ecafc37

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/60faf8066a259f00070e6f24

😎 Browse the preview: https://deploy-preview-29062--kubernetes-io-main-staging.netlify.app/docs/setup/production-environment/windows/user-guide-windows-containers

@marosset
Copy link
Contributor

The example at https://kubernetes.io/docs/setup/production-environment/windows/user-guide-windows-containers/#simplifying-with-runtimeclass uses the docker runtime class handler.

Can we add an example for containerd that uses the default runtime class handler runhcs-wcow-process and then link to the https://kubernetes.io/docs/concepts/containers/runtime-class/#cri-configuration for more information?

@jsturtevant
Copy link
Contributor

How are we going to handle something like the getting started section where node selectors are used: https://kubernetes.io/docs/setup/production-environment/windows/user-guide-windows-containers/#getting-started-deploying-a-windows-container?

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 22, 2021
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

I'd like to make sure we address my query about deprecation of nodeSelector.

/hold
to highlight that

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

```yaml
apiVersion: node.k8s.io/v1
kind: RuntimeClass
metadata:
name: windows-2019
handler: 'docker'
handler: 'runhcs-wcow-process'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use tabs to show handles for docker and the containerd default handler similar to OS tabs at https://kubernetes.io/docs/setup/production-environment/container-runtimes/#cri-o?

A lot of windows users are still running with docker (unfortunately) and this can be a pretty big change for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need some help from @sftim or someone from familiar with that <tab> tag. I am not sure if I am having correct tags.

@perithompson
Copy link
Contributor

How are we going to handle something like the getting started section where node selectors are used: https://kubernetes.io/docs/setup/production-environment/windows/user-guide-windows-containers/#getting-started-deploying-a-windows-container?

I feel like we should address this at the same time? If it's windows and someone is just getting started, it would be good to set them on the RuntimeClass path in the getting started guide

nodeSelector:
kubernetes.io/os: windows
```
Create a file named `win-webserver.yaml` with the contents from [here](#Use RuntimeClass(Recommended approach))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Create a file named `win-webserver.yaml` with the contents from [here](#Use RuntimeClass(Recommended approach))
Create a file named `win-webserver.yaml` with the contents from [using RuntimeClasses](#Use RuntimeClass(Recommended approach))

I think it is a better to use descriptions over here for screen readers


### Ensuring OS-specific workloads land on the appropriate container host
For example: `--register-with-taints='os=windows:NoSchedule'`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
For example: `--register-with-taints='os=windows:NoSchedule'`
For example adding the flag `--register-with-taints='os=windows:NoSchedule'` to kubelet parameters cuases kubelet to register the taint.

Copy link
Contributor Author

@ravisantoshgudimetla ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

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

@jsturtevant - I addressed your comments


```yaml
apiVersion: node.k8s.io/v1
kind: RuntimeClass
metadata:
name: windows-2019
handler: 'docker'
handler: 'runhcs-wcow-process'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need some help from @sftim or someone from familiar with that <tab> tag. I am not sure if I am having correct tags.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 21, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 20, 2021
@k8s-ci-robot k8s-ci-robot added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 20, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2021
@k8s-ci-robot
Copy link
Contributor

@ravisantoshgudimetla: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@reylejano
Copy link
Member

Hi @ravisantoshgudimetla, this PR has had no action and the bot labelled this PR as lifecycle/rotten
This PR needs a rebase
I'm closing this PR due to inactivity and it needs to rebase but if you would like to continue working on this PR, feel free to reopen the PR and rebase
/close

@k8s-ci-robot
Copy link
Contributor

@reylejano: Closed this PR.

In response to this:

Hi @ravisantoshgudimetla, this PR has had no action and the bot labelled this PR as lifecycle/rotten
This PR needs a rebase
I'm closing this PR due to inactivity and it needs to rebase but if you would like to continue working on this PR, feel free to reopen the PR and rebase
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. language/en Issues or PRs related to English language lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants