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

Actuallize podresources description #24641

Conversation

AlexeyPerevalov
Copy link
Contributor

@AlexeyPerevalov AlexeyPerevalov commented Oct 19, 2020

This commit updates description according to
kubernetes/enhancements#1884

/milestone 1.20
Issue: kubernetes/enhancements#2043

Signed-off-by: Alexey Perevalov alexey.perevalov@huawei.com

@k8s-ci-robot
Copy link
Contributor

@AlexeyPerevalov: You must be a member of the kubernetes/website-milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Website milestone maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

This commit updates description according to
kubernetes/enhancements#1884

/milestone 1.20

Signed-off-by: Alexey Perevalov alexey.perevalov@huawei.com

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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 19, 2020
@k8sio-netlify-preview-bot
Copy link
Collaborator

k8sio-netlify-preview-bot commented Oct 19, 2020

Deploy preview for kubernetes-io-vnext-staging processing.

Building with commit b61e575

https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/5fab959b5a8ef10008f1d103

@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Oct 19, 2020
@k8s-ci-robot k8s-ci-robot added 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. labels Oct 19, 2020
@reylejano
Copy link
Member

/milestone 1.20

@k8s-ci-robot k8s-ci-robot added this to the 1.20 milestone Oct 19, 2020
@kbhawkey
Copy link
Contributor

/cc @annajung

@k8s-ci-robot k8s-ci-robot requested a review from annajung October 20, 2020 18:39
@irvifa
Copy link
Member

irvifa commented Oct 20, 2020

/sig architecture

@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Oct 20, 2020
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.

This makes sense to me. I haven't really checked the gRPC details though.
As this page is getting long, I wonder if it makes sense to move that gRPC service description within https://k8s.io/reference/ ?

(if you agree - we can do that move as a second PR against dev-1.20)


```gRPC

// WatchPodResourcesRequest is the request made to the Watch PodResourcesLister service
Copy link
Contributor

Choose a reason for hiding this comment

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

PodResourcesLister or PodResources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. In both KEP (https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/2043-pod-resource-concrete-assigments, https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/606-compute-device-assignment) PodResources was used, but in implementation PodResourcesLister is used now.
Documentation should be written according to implementation.
But what to do with KEP, rename it there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, feel free to update the KEP as well

@AlexeyPerevalov
Copy link
Contributor Author

This makes sense to me. I haven't really checked the gRPC details though.
As this page is getting long, I wonder if it makes sense to move that gRPC service description within https://k8s.io/reference/ ?

(if you agree - we can do that move as a second PR against dev-1.20)

I had the same think when edited. I thought it's reasonable to move PodResorcesLister section into new file. But what is it - https://k8s.io/reference/? Is it https://kubernetes.io/docs/reference/?

@AlexeyPerevalov AlexeyPerevalov force-pushed the PodResourcesEnhancements branch from 0317f94 to 3de9794 Compare October 21, 2020 07:55
@AlexeyPerevalov
Copy link
Contributor Author

AlexeyPerevalov commented Oct 21, 2020

/cc @dashpole

@k8s-ci-robot k8s-ci-robot requested a review from dashpole October 21, 2020 19:15
@reylejano
Copy link
Member

reylejano commented Oct 27, 2020

/remove-sig architecture
/sig node
/hold
waiting for kubernetes/kubernetes#93243 to merge

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. labels Oct 27, 2020
@AlexeyPerevalov
Copy link
Contributor Author

/remove-sig architecture
/sig node
/hold
waiting for kubernetes/kubernetes#93243 to merge

Not only PR 93243, but also in this PR was mentioned API from kubernetes/kubernetes#95734.
Both should be merged.

@annajung
Copy link
Contributor

@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 Oct 27, 2020
@AlexeyPerevalov AlexeyPerevalov force-pushed the PodResourcesEnhancements branch from 3de9794 to edb0eb8 Compare November 3, 2020 11:28
@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 kbhawkey after the PR has been reviewed.
You can assign the PR to them by writing /assign @kbhawkey 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

@annajung
Copy link
Contributor

annajung commented Nov 5, 2020

/assign @reylejano-rxm

@sftim
Copy link
Contributor

sftim commented Nov 10, 2020

Hi @AlexeyPerevalov

Would you have a chance to rebase this against dev-1.20? It's currently got a merge conflict.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2020
@@ -204,7 +259,7 @@ DaemonSet, `/var/lib/kubelet/pod-resources` must be mounted as a
{{< glossary_tooltip term_id="volume" >}} in the plugin's
[PodSpec](/docs/reference/generated/kubernetes-api/{{< param "version" >}}/#podspec-v1-core).

Support for the "PodResources service" requires `KubeletPodResources` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/) to be enabled. It is enabled by default starting with Kubernetes 1.15.
Support for the "PodResourcesLister service" requires `KubeletPodResources` [feature gate](/docs/reference/command-line-tools-reference/feature-gates/) to be enabled. It is enabled by default starting with Kubernetes 1.15.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: You can remove the double quotes around, PodResourceLister service ...
and possibly format the name of the service with single back quotes,
PodResourcesLister?

@@ -193,9 +193,64 @@ for these devices:
// node resources consumed by pods and containers on the node
service PodResourcesLister {
rpc List(ListPodResourcesRequest) returns (ListPodResourcesResponse) {}
rpc GetAllocatableResources(AllocatableResourcesRequest) returns (AllocatableResourcesResponse) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an API offered by the kubelet? gRPC only?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

@irvifa
Copy link
Member

irvifa commented Nov 10, 2020

/retest

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2020
AlexeyPerevalov and others added 3 commits November 11, 2020 10:33
This commit updates description according to
kubernetes/enhancements#1884

Signed-off-by: Alexey Perevalov <alexey.perevalov@huawei.com>
…/device-plugins.md

Co-authored-by: Tim Bannister <tim@scalefactory.com>
…/device-plugins.md

Co-authored-by: Tim Bannister <tim@scalefactory.com>
@AlexeyPerevalov AlexeyPerevalov force-pushed the PodResourcesEnhancements branch from e2f6f69 to b61e575 Compare November 11, 2020 07:41
@irvifa
Copy link
Member

irvifa commented Nov 13, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 13, 2020
@irvifa
Copy link
Member

irvifa commented Nov 13, 2020

/hold
pending kubernetes/kubernetes#95734

@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 Nov 13, 2020
@irvifa
Copy link
Member

irvifa commented Nov 13, 2020

Hi @AlexeyPerevalov Since the code freeze is on effect, wondering if folks raise exception for this since one of the PR is not merged yet

@annajung
Copy link
Contributor

Note:

Exception filed (pending approval)

@annajung
Copy link
Contributor

/hold

@annajung
Copy link
Contributor

Exception request has been denied for this enhancement.

/milestone clear
/close

@k8s-ci-robot k8s-ci-robot removed this from the 1.20 milestone Nov 16, 2020
@k8s-ci-robot
Copy link
Contributor

@annajung: Closed this PR.

In response to this:

Exception request has been denied for this enhancement.

/milestone clear
/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 sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants