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

tighten pvc resource limits validation #107776

Closed
wants to merge 1 commit into from
Closed

tighten pvc resource limits validation #107776

wants to merge 1 commit into from

Conversation

jlsong01
Copy link
Contributor

@jlsong01 jlsong01 commented Jan 26, 2022 •
edited

What type of PR is this?

/kind bug
/sig storage

What this PR does / why we need it:

PVC never supported setting capacity limits. However, it looks like in validation, we never enforced it. This PR is to tighten the validation for pvc resource limits.

Which issue(s) this PR fixes:

Fixes #106637

Special notes for your reviewer:

fix according to Jordan's guidance: #103024 (comment)

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 26, 2022
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 26, 2022
@jlsong01
Copy link
Contributor Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 26, 2022
@jlsong01
Copy link
Contributor Author

jlsong01 commented Jan 26, 2022 •
edited

/assign @mattcary

@mattcary Could you please review it?

@jlsong01 jlsong01 changed the title tighten pvc resource limits validation [WIP] tighten pvc resource limits validation Jan 26, 2022
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2022
@jlsong01 jlsong01 force-pushed the tighten_pvc_limits_validation branch from 5614b2e to 1511281 Compare January 26, 2022 07:11
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jan 26, 2022
@jlsong01 jlsong01 force-pushed the tighten_pvc_limits_validation branch 3 times, most recently from 7449f46 to 6ebd809 Compare January 26, 2022 11:38
@jlsong01
Copy link
Contributor Author

/retest

@jlsong01 jlsong01 changed the title [WIP] tighten pvc resource limits validation tighten pvc resource limits validation Jan 26, 2022
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2022
@mattcary
Copy link
Contributor

(fyi I'm looking at this, but as you realized it turned out to be a bigger change than expected :-)

@jlsong01
Copy link
Contributor Author

jlsong01 commented Feb 4, 2022 •
edited

(fyi I'm looking at this, but as you realized it turned out to be a bigger change than expected :-)

@mattcary Hey Matt, any update on the review? :-)

@mattcary
Copy link
Contributor

mattcary commented Feb 4, 2022

sorry sorry, hopefully this afternoon I can take a look.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2022
@jlsong01 jlsong01 force-pushed the tighten_pvc_limits_validation branch from bfd377c to 96e005b Compare December 16, 2022 12:49
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jlsong01
Once this PR has been reviewed and has the lgtm label, please assign lavalamp for approval by writing /assign @lavalamp in a comment. For more information see the Kubernetes Code Review Process.

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

@jlsong01
Copy link
Contributor Author

/retest

@jlsong01
Copy link
Contributor Author

If you still need this PR then please rebase, if not, please close the PR

@dims Hey, rebase complete. Could you review it please?

@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 17, 2022
@jlsong01 jlsong01 force-pushed the tighten_pvc_limits_validation branch from 96e005b to c77f1cf Compare December 17, 2022 13:20
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2022
@jlsong01 jlsong01 force-pushed the tighten_pvc_limits_validation branch from c77f1cf to 8a40db6 Compare December 17, 2022 13:47
@jlsong01 jlsong01 force-pushed the tighten_pvc_limits_validation branch from 8a40db6 to 5fccbfa Compare December 17, 2022 16:08
@k8s-ci-robot
Copy link
Contributor

@jlsong01: 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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 14, 2023
@liggitt liggitt self-assigned this Feb 16, 2023
@liggitt liggitt moved this from Backlog to Assigned in API Reviews Feb 16, 2023
@liggitt liggitt moved this from Assigned to In progress in API Reviews Feb 16, 2023
@jpbetz
Copy link
Contributor

jpbetz commented Feb 16, 2023 •
edited

@msau42 Validate for coherent (positive quantity values, limits >= requests, ...), issue warnings otherwise?

EDIT: If anything non-storage related also included, warn as well?

@liggitt liggitt assigned msau42 and unassigned jpbetz Feb 16, 2023
@liggitt liggitt mentioned this pull request Feb 16, 2023
Merged
8 tasks
@pohly pohly mentioned this pull request Feb 16, 2023
Closed
2 tasks
@msau42
Copy link
Member

msau42 commented Feb 21, 2023

Added my proposal for what we can safely validate at #106637 (comment)

@liggitt liggitt moved this from In progress to Changes requested in API Reviews Feb 24, 2023
@liggitt liggitt removed their assignment May 13, 2023
@carlory
Copy link
Member

carlory commented Jul 3, 2023

@jlsong01 please rebase upstream changes

@dims dims added the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Oct 24, 2023
@k8s-triage-robot
Copy link

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

This bot triages 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 PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

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

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

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

This bot triages 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 PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

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

/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
Reviewers

@deads2k deads2k Awaiting requested review from deads2k

@justinsb justinsb Awaiting requested review from justinsb

@mattcary mattcary Awaiting requested review from mattcary

Assignees

@msau42 msau42

@mattcary mattcary

Labels
api-review Categorizes an issue or PR as actively needing an API review. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Apps
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PVC resource limits not validated
10 participants
@jlsong01 @mattcary @k8s-triage-robot @xing-yang @dims @k8s-ci-robot @jpbetz @msau42 @carlory @liggitt