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

DRA: test for structured parameters #123938

Merged
merged 7 commits into from Apr 18, 2024
Merged

DRA: test for structured parameters #123938

merged 7 commits into from Apr 18, 2024
+2,623 −59

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Mar 14, 2024 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

#123516 got merged with some tests still missing due to time pressure. This PR adds those tests. One bug was found while doing so.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. labels Mar 14, 2024
@k8s-ci-robot k8s-ci-robot added area/code-generation area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 14, 2024
@pohly pohly mentioned this pull request Mar 14, 2024
Closed
@pohly
Copy link
Contributor Author

pohly commented Mar 14, 2024

Unit testing depends on correct handling of ResourceClaimParameters and ResourceClassParameters by the fake client, which is currently broken because of a wrong guess of the plural form. The right solution is still pending. Note sure yet how to handle this. Merge without the dynamicresources_test.go changes?

@alculquicondor
Copy link
Member

/cc

@pohly pohly mentioned this pull request Mar 14, 2024
Open
@bart0sh bart0sh added this to Triage in SIG Node PR Triage Mar 14, 2024
@pohly
Copy link
Contributor Author

pohly commented Mar 14, 2024

The right solution is still pending. Note sure yet how to handle this. Merge without the dynamicresources_test.go changes?

I have a workaround. I'm currently waiting for #123932 to merge, then I will rebase.

@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@pohly
dra e2e: watch claims and validate them
f149d6d 
Logging claims helps with debugging test failures. Checking the finalizer
catches unexpected behavior.
@pohly
dra e2e: sanity check resource handle
cf8fffa 
When using structured parameters, the instance name must match and not be in
use already.

NodeUnprepareResources must be called with the same handle are
NodePrepareResources.
@pohly pohly force-pushed the dra-structured-parameters-tests branch from 1a4517f to 4179c63 Compare March 14, 2024 19:52
@pohly
dra scheduler: spelling fix
607261e
@pohly
dra scheduler: unit tests
458e227 
Coverage was checked with a cover profile. The biggest remaining gap is for
isSchedulableAfterClaimParametersChange and
isSchedulableAfterClassParametersChange which will get handled when refactoring
the
foreachPodResourceClaim (kubernetes#123697).
@pohly pohly force-pushed the dra-structured-parameters-tests branch from 79ed4eb to 458e227 Compare March 22, 2024 09:16
@pohly
Copy link
Contributor Author

pohly commented Mar 22, 2024

/kind bug

There is a (minor) fix included here, see first commit.

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 22, 2024
@bart0sh
Copy link
Contributor

bart0sh commented Mar 24, 2024

/triage accepted
/priority important-soon

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed 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. labels Mar 24, 2024
@bart0sh bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Mar 24, 2024
@pohly
dra scheduler: simplify unit tests
6f5696b 
The guideline in
https://github.com/kubernetes/community/blob/master/sig-scheduling/CONTRIBUTING.md#technical-and-style-guidelines
is to not compare error strings. This makes the tests less precise. In return,
unit tests don't need to be updated when error strings change.
@alculquicondor
Copy link
Member

We missed the test freeze, so this is no longer candidate for going to 1.30 or backporting.

But maybe we can backport the bugfix, if it's critical. How would you qualify it? If it's critical, split the fix into its own PR to go for 1.30.1

alculquicondor
alculquicondor reviewed Mar 27, 2024
View reviewed changes
Copy link
Member

@alculquicondor alculquicondor 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 k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 382063ffdcd13dfa9cab39fe51726be36cfbf567

@alculquicondor
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, pohly

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 Mar 27, 2024
@pohly
Copy link
Contributor Author

pohly commented Mar 27, 2024

The bug is highly unlikely to occur in practice. Given that the feature is alpha and new in 1.30, I think we don't need to backport.

@SergeyKanzhelev SergeyKanzhelev moved this from Triage to Done in SIG Node CI/Test Board Apr 3, 2024
@SergeyKanzhelev
Copy link
Member

/cc

@bart0sh bart0sh moved this from Needs Reviewer to Done in SIG Node PR Triage Apr 4, 2024
@k8s-ci-robot k8s-ci-robot merged commit d2ce87e into kubernetes:master Apr 18, 2024
18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone Apr 18, 2024
@BenTheElder BenTheElder mentioned this pull request Apr 19, 2024
Closed
@pohly pohly mentioned this pull request Apr 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alculquicondor alculquicondor alculquicondor left review comments

@bart0sh bart0sh Awaiting requested review from bart0sh

@chendave chendave Awaiting requested review from chendave

@SergeyKanzhelev SergeyKanzhelev Awaiting requested review from SergeyKanzhelev

Assignees

@alculquicondor alculquicondor

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation area/kubelet area/test 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/bug Categorizes issue or PR as related to a bug. 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. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. 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/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. 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
Status: Done
SIG Node CI/Test Board
Done
SIG Node PR Triage
Done
SIG Node: Dynamic Resource Allocation
Status: 👀 In review
Milestone
v1.31
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@pohly @alculquicondor @k8s-triage-robot @Jefftree @bart0sh @k8s-ci-robot @SergeyKanzhelev