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

fix: correctly handle non-existent / empty StorageClasses #783

Merged
merged 1 commit into from
Dec 7, 2023
Merged

fix: correctly handle non-existent / empty StorageClasses #783

merged 1 commit into from
Dec 7, 2023

Conversation

jmdeal
Copy link
Member

@jmdeal jmdeal commented Nov 14, 2023 •
edited

Fixes #664

Description
Karpenter doesn't correctly handle two StorageClass cases: when the StorageClass is explicitly "" and when the StorageClass does not exist. A pod with a PVC that falls into one of these cases should be able to successfully schedule if the PVC is already bound to a PV. Additionally, currently Karpenter treats both a nil StorageClass and "" StorageClass as the default StorageClass. This should only be the case when the StorageClass is nil.

How was this change tested?
make test

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@jmdeal jmdeal requested a review from a team as a code owner November 14, 2023 07:31
@coveralls
Copy link

coveralls commented Nov 14, 2023 •
edited

Pull Request Test Coverage Report for Build 6951585057

  • 48 of 66 (72.73%) changed or added relevant lines in 4 files are covered.
  • 37 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.4%) to 79.777%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/provisioning/scheduling/volumetopology.go 18 20 90.0%
pkg/utils/volume/volume.go 13 15 86.67%
pkg/scheduling/volumeusage.go 16 30 53.33%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/provisioning/provisioner.go 1 78.38%
pkg/scheduling/volumeusage.go 1 75.97%
pkg/controllers/provisioning/scheduling/volumetopology.go 5 84.87%
pkg/utils/atomic/cachedvar.go 13 43.48%
pkg/scheduling/storageclass.go 17 13.64%
Totals Coverage Status
Change from base Build 6949870138: -0.4%
Covered Lines: 8296
Relevant Lines: 10399
💛 - Coveralls

@jmdeal jmdeal changed the title fix: correctly handle empty string StorageClass fix: correctly handle non-existent / empty StorageClasses Nov 15, 2023
@jmdeal jmdeal force-pushed the empty-storageclass-fix branch 4 times, most recently from e2892b7 to 75f3f4c Compare November 15, 2023 21:03
@jmdeal jmdeal force-pushed the empty-storageclass-fix branch 7 times, most recently from e682f0b to f6bccf3 Compare November 16, 2023 22:55
jonathan-innis
jonathan-innis reviewed Nov 21, 2023
View reviewed changes
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
@jmdeal jmdeal force-pushed the empty-storageclass-fix branch 4 times, most recently from 42b747a to b60d2ef Compare November 21, 2023 22:55
jonathan-innis
jonathan-innis reviewed Nov 21, 2023
View reviewed changes
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
jonathan-innis
jonathan-innis reviewed Nov 22, 2023
View reviewed changes
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Show resolved Hide resolved
pkg/scheduling/volumeusage.go Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
@jmdeal
Copy link
Member Author

jmdeal commented Nov 22, 2023

/check-cla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2023
jonathan-innis
jonathan-innis reviewed Nov 23, 2023
View reviewed changes
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/scheduling/volumeusage.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
pkg/controllers/provisioning/scheduling/volumetopology.go Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Nov 24, 2023 •
edited

Pull Request Test Coverage Report for Build 7104866217

  • 54 of 71 (76.06%) changed or added relevant lines in 4 files are covered.
  • 39 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.5%) to 82.606%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/controllers/provisioning/scheduling/volumetopology.go 23 25 92.0%
pkg/utils/volume/volume.go 13 15 86.67%
pkg/scheduling/volumeusage.go 17 30 56.67%
Files with Coverage Reduction New Missed Lines %
pkg/controllers/provisioning/provisioner.go 1 78.95%
pkg/scheduling/volumeusage.go 1 76.56%
pkg/test/expectations/expectations.go 2 95.01%
pkg/controllers/provisioning/scheduling/volumetopology.go 5 84.87%
pkg/utils/atomic/cachedvar.go 13 43.48%
pkg/scheduling/storageclass.go 17 13.64%
Totals Coverage Status
Change from base Build 7096276944: -0.5%
Covered Lines: 7480
Relevant Lines: 9055
💛 - Coveralls

@jonathan-innis
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2023
@jonathan-innis
Copy link
Member

/hold

Holding this change for E2E storage test validation

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 27, 2023
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 5, 2023
@jmdeal jmdeal mentioned this pull request Dec 5, 2023
Merged
3 tasks
@k8s-ci-robot
Copy link
Contributor

@jmdeal: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-karpenter-test-1-26 174b61f link false /test pull-karpenter-test-1-26
pull-karpenter-test-1-27 174b61f link false /test pull-karpenter-test-1-27
pull-karpenter-test-1-29 174b61f link false /test pull-karpenter-test-1-29
pull-karpenter-test-1-28 174b61f link false /test pull-karpenter-test-1-28

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

jonathan-innis
jonathan-innis approved these changes Dec 5, 2023
View reviewed changes
Copy link
Member

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmdeal, jonathan-innis

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 Dec 5, 2023
@jonathan-innis
Copy link
Member

/unhold
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 7, 2023
@jonathan-innis
Copy link
Member

We have the E2E tests in place and will bump the Karpenter version that contains this fix before we merge the E2E testing in the AWS cloud provider repo: aws/karpenter-provider-aws#5229

@k8s-ci-robot k8s-ci-robot merged commit 58e1d75 into kubernetes-sigs:main Dec 7, 2023
8 of 12 checks passed
@jmdeal jmdeal mentioned this pull request Dec 7, 2023
Closed
@jmdeal jmdeal mentioned this pull request Jan 11, 2024
Closed
@jmdeal jmdeal mentioned this pull request Feb 6, 2024
Closed
@jmdeal jmdeal deleted the empty-storageclass-fix branch May 9, 2024 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonathan-innis jonathan-innis jonathan-innis approved these changes

@ellistarn ellistarn Awaiting requested review from ellistarn ellistarn was automatically assigned from aws/karpenter

Assignees

@jonathan-innis jonathan-innis

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Karpenter stuck on "Waiting on Cluster Sync" with non-existent StorageClass
4 participants
@jmdeal @coveralls @jonathan-innis @k8s-ci-robot