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

Display extended resources in node allocated resources #46079

Merged

Conversation

xiangpengzhao
Copy link
Contributor

@xiangpengzhao xiangpengzhao commented May 19, 2017

What this PR does / why we need it:
Displays opaque integer [extended] resources in node allocated resources of command kubectl describe node. This will give users more info about node OIR [extended resources] consumption.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
This is a partially fix of #44271.

Special notes for your reviewer:
This PR

  • only displays allocated OIR [extended resources] details of node, it doesn't display OIR [extended resources] requests/limits for each pod because it's hard to organize format. I tried to print OIR [extended resources] requests/limits of pods, but some strings have been eaten when a line is too long (the output has been separated into two lines by terminal). I think it's because a \t blank can't be show in two lines.
  • uses OIR-foo instead of pod.alpha.kubernetes.io/opaque-int-resource-foo for short.
  • doesn't display the percentage of OIR [extended resources] usage because I think the percentage is not so meaningful.
  • displays each OIR [extended resources] in single rows to be clear.

UPDATE:
Example with default namespace resource:

Non-terminated Pods:         (1 in total)
  Namespace                  Name                     CPU Requests  CPU Limits  Memory Requests  Memory Limits
  ---------                  ----                     ------------  ----------  ---------------  -------------
  default                    rc-nginx-single-krp84    1 (33%)       1 (33%)     512Mi (6%)       512Mi (6%)
Allocated resources:
  (Total limits may be over 100 percent, i.e., overcommitted.)
Resource                 Requests    Limits
  --------               --------    ------
  cpu                    1 (33%)     1 (33%)
  memory                 512Mi (6%)  512Mi (6%)
  kubernetes.io/widgets  111         0

/cc @ConnorDoyle @soltysh

ref #44181

Release note:

Display requests/limits of extended resources in node allocated resources.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 19, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 19, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels May 19, 2017
@xiangpengzhao
Copy link
Contributor Author

/assign @ConnorDoyle @soltysh

@k8s-ci-robot k8s-ci-robot assigned ConnorDoyle and soltysh and unassigned ericchiang and ghodss May 19, 2017
@spiffxp
Copy link
Member

spiffxp commented Jul 10, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 10, 2017
@pwittrock
Copy link
Member

/unassign

@pwittrock
Copy link
Member

/assign @Random-Liu

@pwittrock
Copy link
Member

/assign @dchen1107

@pwittrock
Copy link
Member

@xiangpengzhao Please add a release note

@xiangpengzhao
Copy link
Contributor Author

Editing release note in PR body didn't take effect. Add it manually here.

Display requests/limits of opaque integer resources in node allocated resources.

@xiangpengzhao
Copy link
Contributor Author

Release note:

Display requests/limits of opaque integer resources in node allocated resources.

@xiangpengzhao
Copy link
Contributor Author

Hmm, none of the ways I tried to add release note works...

@spxtr How can I add a release note in this case? Thanks!

I remember I saw some guy adding release note like this way: #46079 (comment) above.

@xiangpengzhao
Copy link
Contributor Author

/release-note

hope this works :)

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 1, 2017
@soltysh
Copy link
Contributor

soltysh commented Aug 8, 2017

The change itself looks good, but I'm not sure if this isn't breaking api, since we're adding quite a few additional information. @kubernetes/sig-cli-api-reviews wdyt should we keep that additional data behind a flag or something?

@dixudx
Copy link
Member

dixudx commented Feb 12, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2018
@spiffxp
Copy link
Member

spiffxp commented Feb 16, 2018

/unassign

@RenaudWasTaken
Copy link
Contributor

Can we get this in for 1.10 cc @vishh @smarterclayton ?

@jiayingz
Copy link
Contributor

/cc @jiayingz

@ConnorDoyle
Copy link
Contributor

Every now and then I get notifications from this PR and feel a great sadness.

@xiangpengzhao
Copy link
Contributor Author

@ConnorDoyle haha... The PR is 🔟 months old. 👶 --> 👴

ping some folks from approvers list: @adohe @smarterclayton @liggitt @mengqiy @deads2k @brendandburns @janetkuo :)

w.Write(LEVEL_0, "Allocated resources:\n (Total limits may be over 100 percent, i.e., overcommitted.)\n CPU Requests\tCPU Limits\tMemory Requests\tMemory Limits\n")
w.Write(LEVEL_1, "------------\t----------\t---------------\t-------------\n")
w.Write(LEVEL_0, "Allocated resources:\n (Total limits may be over 100 percent, i.e., overcommitted.)\n")
w.Write(LEVEL_0, "Resource\tRequests\tLimits\n")
Copy link
Member

Choose a reason for hiding this comment

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

should be LEVEL_1 to get Resource to align with --------, right?

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 catch! A big 👍 for you :)

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Mar 28, 2018
@xiangpengzhao
Copy link
Contributor Author

@liggitt inline comment addressed. PTAL. Thanks!

@liggitt
Copy link
Member

liggitt commented Mar 28, 2018

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorDoyle, dixudx, liggitt, soltysh, spiffxp, vishh, xiangpengzhao

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

@xiangpengzhao
Copy link
Contributor Author

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

2 similar comments
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 33482dd into kubernetes:master Mar 28, 2018
@xiangpengzhao xiangpengzhao deleted the print-allocated-oir branch March 28, 2018 14:17
@ConnorDoyle
Copy link
Contributor

🙏🏽 @liggitt !

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet