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

Clearer information about the emptyDir and correction for how topologyKey works #42780

Merged
merged 7 commits into from
Oct 17, 2023

Conversation

oculushut
Copy link
Contributor

Minor amendment to the emptyDir documentation. I found the existing text a little confusing.

I believe the statement is confusing since we are in the emptyDir section of the documentation.  

If a Node is restarted then all pods that resided on that node will be rescheduled onto another Node.  Rescheduled pods will have an empty volume  whether you choose emptyDir.medium "Memory" or not.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 29, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot
Copy link
Contributor

Welcome @oculushut!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. language/en Issues or PRs related to English language labels Aug 29, 2023
@k8s-ci-robot k8s-ci-robot requested review from msau42 and thockin August 29, 2023 18:09
@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 29, 2023
@netlify
Copy link

netlify bot commented Aug 29, 2023

Pull request preview available for checking

Name Link
🔨 Latest commit d3057c1
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/6524051cfa54a90008538c93
😎 Deploy Preview https://deploy-preview-42780--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@dipesh-rawat
Copy link
Member

@oculushut Thanks you for your contribution.
Could you please sign the CLA before the PR can be reviewed.
You can follow the steps documented here: CLA Steps

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 29, 2023
oculushut and others added 2 commits August 30, 2023 10:01
…affinity rule (#1)

The existing text does not make sense to me.  

There is no zone "V" or "R" in the example.  

I have changed the text to be consistent with top answer here which seems to make more sense: https://stackoverflow.com/questions/72240224/what-is-topologykey-in-pod-affinity
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 30, 2023
@oculushut oculushut changed the title Clearer information about the difference between the use of RAM and disks in the case of emptyDir Clearer information about the emptyDir and correction for how topologyKey works Aug 30, 2023
@tengqm
Copy link
Contributor

tengqm commented Sep 16, 2023

Don't understand the merits of this change. ...

@oculushut
Copy link
Contributor Author

Don't understand the merits of this change. ...

It is correcting the documentation:

  • The current example text refers to Zones which do not exist in the example configuration. Check out the Pod Affinity page fully rendered rather than via GitHub to see the problem more clearly (https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/).

  • The text we have at the moment suggests that emptyDir disks are not cleared after a reboot - which is incorrect. They are cleared after a reboot. This is more obvious when you look at the full page and see that we are in the emptyDir section of the documentation.

@tengqm
Copy link
Contributor

tengqm commented Sep 19, 2023

Don't understand the merits of this change. ...

It is correcting the documentation:

* The current example text refers to Zones which do not exist in the example configuration.  Check out the Pod Affinity page fully rendered rather than via GitHub to see the problem more clearly (https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/).

* The text we have at the moment suggests that emptyDir disks are not cleared after a reboot - which is incorrect.  They are cleared after a reboot.  This is more obvious when you look at the full page and see that we are in the emptyDir section of the documentation.

I can get the second point, which IMHO is a clean fix. But I don't understand why the (anti-)affinity text should be removed. Are they incorrect?

@oculushut
Copy link
Contributor Author

Don't understand the merits of this change. ...

It is correcting the documentation:

* The current example text refers to Zones which do not exist in the example configuration.  Check out the Pod Affinity page fully rendered rather than via GitHub to see the problem more clearly (https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/).

* The text we have at the moment suggests that emptyDir disks are not cleared after a reboot - which is incorrect.  They are cleared after a reboot.  This is more obvious when you look at the full page and see that we are in the emptyDir section of the documentation.

I can get the second point, which IMHO is a clean fix. But I don't understand why the (anti-)affinity text should be removed. Are they incorrect?

So, go here: https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/

Search for "zone=V"

Now look at the example YAML just above where you find "zone=V". There is no such thing as zone V in the YAML. Similiarly, lower down there is a reference to zone R. Again, this does not exist in the above YAML. So, the textual description is discussing configurations that do not exist in the example and should be removed. At least, this is my understanding unless there is some kind of implicit zone V or zone R.

@tengqm
Copy link
Contributor

tengqm commented Sep 20, 2023

@oculushut The so called 'zone=V' or 'zone=R' is about pod spread topology. It implies that you are running the Pod in zone 'V', or zone 'R', and you are "supposed" to have such a label on the Node objects. With this preconditions, you can combine Pod spread topology with affinity rules.

So ... in my opinion, it is a complicated scenario, but the complexity worth it. If we revise the text, we will need to revise the YAML as well. That is, why we have a topology key in the YAML?

@oculushut
Copy link
Contributor Author

oculushut commented Sep 20, 2023

@oculushut The so called 'zone=V' or 'zone=R' is about pod spread topology. It implies that you are running the Pod in zone 'V', or zone 'R', and you are "supposed" to have such a label on the Node objects. With this preconditions, you can combine Pod spread topology with affinity rules.

So ... in my opinion, it is a complicated scenario, but the complexity worth it. If we revise the text, we will need to revise the YAML as well. That is, why we have a topology key in the YAML?

@oculushut The so called 'zone=V' or 'zone=R' is about pod spread topology. It implies that you are running the Pod in zone 'V', or zone 'R', and you are "supposed" to have such a label on the Node objects. With this preconditions, you can combine Pod spread topology with affinity rules.

So ... in my opinion, it is a complicated scenario, but the complexity worth it. If we revise the text, we will need to revise the YAML as well. That is, why we have a topology key in the YAML?

I think must be missing something.

This is the YAML that is currently published:

apiVersion: v1
kind: Pod
metadata:
  name: with-pod-affinity
spec:
  affinity:
    podAffinity:
      requiredDuringSchedulingIgnoredDuringExecution:
      - labelSelector:
          matchExpressions:
          - key: security
            operator: In
            values:
            - S1
        topologyKey: topology.kubernetes.io/zone
    podAntiAffinity:
      preferredDuringSchedulingIgnoredDuringExecution:
      - weight: 100
        podAffinityTerm:
          labelSelector:
            matchExpressions:
            - key: security
              operator: In
              values:
              - S2
          topologyKey: topology.kubernetes.io/zone
  containers:
  - name: with-pod-affinity
    image: registry.k8s.io/pause:2.0

How would the system know that it should put the pod in V nodes and avoid R nodes based on the above configuration? V and R are mentioned in the descriptive text, but not that YAML example above. So either there is a misconfiguration in the YAML or the text is wrong. I've amended the text in my change.

Is there another configuration file on that page which shows that there are V and R nodes?

@tengqm
Copy link
Contributor

tengqm commented Sep 20, 2023

How would the system know that it should put the pod in V nodes and avoid R nodes based on the above configuration? V and R are mentioned in the descriptive text, but not that YAML example above. So either there is a misconfiguration in the YAML or the text is wrong. I've amended the text in my change.

Is there another configuration file on that page which shows that there are V and R nodes?

The topologyKey: topology.kubernetes.io/zone is used for this purpose. The 'V' and 'R' are not real values, they are there as examples. They are zone labels to be found on Node objects. See https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/#node-labels. There are some stackoverflow discussions on this as well. https://stackoverflow.com/questions/72240224/what-is-topologykey-in-pod-affinity

We don't have a Node spec on this page, because we assumed users understand this concept. Instead of removing those texts, I'd suggest we make them more readable, more understandable.
If your concern is that the example is not a good candidate for copy-paste and experiment, we can refrain from making it an example, just a snippet in the markdown would be okay.

@oculushut
Copy link
Contributor Author

How would the system know that it should put the pod in V nodes and avoid R nodes based on the above configuration? V and R are mentioned in the descriptive text, but not that YAML example above. So either there is a misconfiguration in the YAML or the text is wrong. I've amended the text in my change.
Is there another configuration file on that page which shows that there are V and R nodes?

The topologyKey: topology.kubernetes.io/zone is used for this purpose. The 'V' and 'R' are not real values, they are there as examples. They are zone labels to be found on Node objects. See https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/#node-labels. There are some stackoverflow discussions on this as well. https://stackoverflow.com/questions/72240224/what-is-topologykey-in-pod-affinity

We don't have a Node spec on this page, because we assumed users understand this concept. Instead of removing those texts, I'd suggest we make them more readable, more understandable. If your concern is that the example is not a good candidate for copy-paste and experiment, we can refrain from making it an example, just a snippet in the markdown would be okay.

I see your point now. Perhaps something like this would make it clearer that zone V would be specified via node labels? Esp. for anybody reading this section ahead of the zone labels section.

The affinity rule specifies that the scheduler is allowed to place the example Pod on a node only if that node belongs to a specific zone where other Pods have been labeled with security=S1. For instance, if we have a cluster with a designated zone, let's call it "Zone V," consisting of nodes labeled with topology.kubernetes.io/zone=V, the scheduler can assign the Pod to any node within Zone V, as long as there is at least one Pod within Zone V already labeled with security=S1. Conversely, if there are no Pods with security=S1 labels in Zone V, the scheduler will not assign the example Pod to any node in that zone.

@tengqm
Copy link
Contributor

tengqm commented Sep 21, 2023

@oculushut The revised text looks good to me.

The affinity rule says that the scheduler can only schedule a Pod onto a node if
the node is in the same zone (`topologyKey: topology.kubernetes.io/zone`) as one or more existing Pods with the label
`security=S1`.
The affinity rule specifies that the scheduler is allowed to place the example Pod on a node only if that node belongs to a specific [zone](https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/) where other Pods have been labeled with `security=S1`. For instance, if we have a cluster with a designated zone, let's call it "Zone V," consisting of nodes labeled with `topology.kubernetes.io/zone=V`, the scheduler can assign the Pod to any node within Zone V, as long as there is at least one Pod within Zone V already labeled with `security=S1`. Conversely, if there are no Pods with `security=S1` labels in Zone V, the scheduler will not assign the example Pod to any node in that zone.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please manually wrap the long line for ease of change tracking and localization.
Please use relative links [zone](/docs/...) without hostname.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2023
@tengqm
Copy link
Contributor

tengqm commented Oct 9, 2023

/label tide/merge-method-squash
/approve

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 9, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tengqm

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

The pull request process is described 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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2023
@tengqm
Copy link
Contributor

tengqm commented Oct 9, 2023

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 9025d0b61806fa3c5fbcd1a27721dd88e615a282

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 9, 2023
@k8s-ci-robot k8s-ci-robot requested a review from tengqm October 9, 2023 13:50
Copy link
Member

@windsonsea windsonsea left a comment

Choose a reason for hiding this comment

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

Thanks.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 3249982be65bc9df9fa2900575c672083af8a6d3

@k8s-ci-robot k8s-ci-robot merged commit e124688 into kubernetes:main Oct 17, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants