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.

Sign up for GitHub

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

Jump to bottom

Removed opaque integer resources (deprecated in v1.8) #55103

Merged
merged 1 commit into from
Nov 22, 2017
Merged

Removed opaque integer resources (deprecated in v1.8) #55103

merged 1 commit into from
Nov 22, 2017

Conversation

ConnorDoyle
Copy link
Contributor

What this PR does / why we need it:

  • Remove opaque integer resources (OIR) support from the code base. This feature was deprecated in v1.8 and replaced by Extended Resources (ER).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #55102

Release note:

Remove opaque integer resources (OIR) support (deprecated in v1.8.)

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 4, 2017
@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Nov 4, 2017
dixudx
dixudx reviewed Nov 4, 2017
View reviewed changes
pkg/api/helper/helpers.go Outdated
@@ -143,8 +143,7 @@ func IsStandardContainerResourceName(str string) bool {
// IsExtendedResourceName returns true if the resource name is not in the
// default namespace, or it has the opaque integer resource prefix.
Copy link
Member

Choose a reason for hiding this comment

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

remove OIR from the comment as well.

pkg/api/v1/helper/helpers.go Outdated
@@ -33,7 +33,7 @@ import (
// default namespace, or it has the opaque integer resource prefix.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

pkg/api/v1/helper/helpers.go Outdated
@@ -33,7 +33,7 @@ import (
// default namespace, or it has the opaque integer resource prefix.
func IsExtendedResourceName(name v1.ResourceName) bool {
// TODO: Remove OIR part following deprecation.
Copy link
Member

Choose a reason for hiding this comment

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

Remove this TODO.

@ConnorDoyle ConnorDoyle force-pushed the remove-oir branch 8 times, most recently from a760ee8 to 18c5ac1 Compare November 9, 2017 07:50
@ConnorDoyle
Copy link
Contributor Author

ping

  • @kubernetes/sig-scheduling-api-reviews
  • @kubernetes/sig-scheduling-pr-reviews
  • @vishh

@k8s-ci-robot k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Nov 9, 2017
@resouer resouer added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 9, 2017
@ConnorDoyle ConnorDoyle mentioned this pull request Nov 9, 2017
Closed
22 tasks
@ConnorDoyle
Copy link
Contributor Author

@davidopp could you please add the v1.9 milestone to this one?

zhangxiaoyu-zidif
zhangxiaoyu-zidif reviewed Nov 10, 2017
View reviewed changes
pkg/apis/core/validation/validation_test.go
},
Limits: core.ResourceList{
helper.OpaqueIntResourceName("A"): resource.MustParse("20"),
core.ResourceName("example.com/a"): resource.MustParse("10"),
Copy link
Contributor

Choose a reason for hiding this comment

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

why change 20 to 10？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case is to verify the handling of valid extended resource requests. One difference between extended resources and opaque integer resources is that extended resources may not be overcommitted. So, if both request and limit are specified then they must be equal to pass validation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have other test cases in this file where extended resource request != limit, but we expect them to fail validation.

@mikedanese mikedanese assigned vishh and davidopp and unassigned mikedanese Nov 14, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2017
@ConnorDoyle
Copy link
Contributor Author

Attn: @kubernetes/api-approvers (@smarterclayton @thockin @lavalamp)

@ConnorDoyle
Copy link
Contributor Author

/status in-progress

@k8s-ci-robot
Copy link
Contributor

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

@ConnorDoyle
Copy link
Contributor Author

/retest

@ConnorDoyle
Copy link
Contributor Author

/unassign @balajismaniam @vishh @davidopp @soltysh
/assign @thockin @lavalamp @smarterclayton

@ConnorDoyle
Copy link
Contributor Author

/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Nov 22, 2017
@ConnorDoyle
Copy link
Contributor Author

/remove-priority important-soon

@k8s-ci-robot k8s-ci-robot removed the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 22, 2017
@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Needs Attention

@ConnorDoyle @lavalamp @smarterclayton @thockin @kubernetes/sig-scheduling-misc

Action required: During code slush, pull requests in the milestone should be in progress.
If this pull request is not being actively worked on, please remove it from the milestone.
If it is being worked on, please add the status/in-progress label so it can be tracked with other in-flight pull requests.

Note: This pull request is marked as priority/critical-urgent, and must be updated every 3 days during code slush.

Example update:

ACK.  In progress
ETA: DD/MM/YYYY
Risks: Complicated fix required
Pull Request Labels
  • sig/scheduling: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help

@ConnorDoyle
Copy link
Contributor Author 

ACK.  In progress
ETA: Unknown
Risks: Waiting for approver.

@thockin
Copy link
Member

thockin commented Nov 22, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ConnorDoyle, thockin, vishh

Associated issue: 55102

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55103, 56036, 56186). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 2a18a2a into kubernetes:master Nov 22, 2017
@ConnorDoyle ConnorDoyle deleted the remove-oir branch December 11, 2017 21:58
@weinliu weinliu mentioned this pull request Feb 7, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsalamat bsalamat bsalamat left review comments

@dixudx dixudx dixudx left review comments

@zhangxiaoyu-zidif zhangxiaoyu-zidif zhangxiaoyu-zidif left review comments

@vishh vishh vishh approved these changes

Assignees

@lavalamp lavalamp

@smarterclayton smarterclayton

@thockin thockin

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 kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. milestone/needs-attention priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.

Remove support for opaque integer resources (deprecated in v1.8)