Skip to content

feature(scheduler): implement matchLabelKeys/mismatchLabelKeys in PodAffinity and PodAntiAffinity#116065

Merged
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
sanposhiho:match-label-key-alternative
Oct 23, 2023
Merged

feature(scheduler): implement matchLabelKeys/mismatchLabelKeys in PodAffinity and PodAntiAffinity#116065
k8s-ci-robot merged 1 commit intokubernetes:masterfrom
sanposhiho:match-label-key-alternative

Conversation

@sanposhiho
Copy link
Copy Markdown
Member

@sanposhiho sanposhiho commented Feb 26, 2023 •
edited
Loading

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

implement matchLabelKeys in PodAffinity and PodAntiAffinity proposed from KEP-3633.

Which issue(s) this PR fixes:

Fixes #112947
Related kubernetes/enhancements#3633

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The matchLabelKeys/mismatchLabelKeys feature is introduced to the hard/soft PodAffinity/PodAntiAffinity.

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

- [KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-scheduling/3633-matchlabelkeys-to-podaffinity

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/code-generation area/test 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/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 Feb 26, 2023
@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Feb 26, 2023

cc @Huang-Wei
FYI @kubernetes/sig-scheduling-misc

🙏

@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Feb 26, 2023

/cc @Huang-Wei

@sanposhiho sanposhiho force-pushed the match-label-key-alternative branch from e4cafa9 to 0af786e Compare February 26, 2023 05:29
@k8s-triage-robot
Copy link
Copy Markdown

k8s-triage-robot commented Feb 26, 2023

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.

@sanposhiho sanposhiho force-pushed the match-label-key-alternative branch from 0af786e to 4cce848 Compare February 26, 2023 07:33
sanposhiho
sanposhiho commented Feb 26, 2023
View reviewed changes
Comment thread test/e2e/scheduling/predicates.go Outdated
Copy link
Copy Markdown
Member Author

@sanposhiho sanposhiho Feb 26, 2023

Choose a reason for hiding this comment

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

I'm not sure how the feature gate is treated in e2e testing.
Do I need to do something to run this e2e test with feature gate enabled?

Copy link
Copy Markdown
Member

@Huang-Wei Huang-Wei Mar 2, 2023

Choose a reason for hiding this comment

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

An alpha e2e test cannot be verified in regular pre-submit CI. But optionally, you can submit a PR to test-infra/ to have it enabled in a special CI job where the env enables particular alpha feature gates. Ref: kubernetes/test-infra#27862

So for the e2e, it'd be good to paste the PASS result here. And remember to add [Feature:<feature gate name>] [alpha] string here. Ref: #113442.

Copy link
Copy Markdown
Member Author

@sanposhiho sanposhiho Mar 3, 2023

Choose a reason for hiding this comment

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

Thanks. I'll follow that later.

Copy link
Copy Markdown
Member Author

@sanposhiho sanposhiho Mar 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Checked these added e2e tests are passed.

logs
_output/bin/e2e.test --provider=local --ginkgo.focus='MatchLabelKeysInPodAffinity'
  I0305 22:41:24.228053    8686 e2e.go:117] Starting e2e run "dd1bfd89-3119-4b70-ad44-f046dfd40634" on Ginkgo node 1
Running Suite: Kubernetes e2e suite - /Users/sanposhiho/workspace/kubernetes
============================================================================
Random Seed: 1678023684 - will randomize all specs

Will run 4 of 7124 specs
SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS•SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS

Ran 4 of 7124 Specs in 209.431 seconds
SUCCESS! -- 4 Passed | 0 Failed | 0 Pending | 7120 Skipped
PASS

Copy link
Copy Markdown
Member

@alculquicondor alculquicondor Mar 14, 2023

Choose a reason for hiding this comment

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

Remove e2e test. We generally only do them for features that involve more than just scheduler. Integration tests should be enough.

Copy link
Copy Markdown
Member Author

@sanposhiho sanposhiho Mar 30, 2023

Choose a reason for hiding this comment

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

Now, we (unfortunately) freed from the deadline of v1.27, no need to leave this as follow up.
I moved almost all test cases to unit tests, and keep only basic test cases in the integration tests.

@cici37
Copy link
Copy Markdown
Contributor

cici37 commented Feb 28, 2023

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Feb 28, 2023
@Huang-Wei
Copy link
Copy Markdown
Member

Huang-Wei commented Mar 2, 2023

@sanposhiho could you please rebase? I will review after the rebase.

@sanposhiho sanposhiho force-pushed the match-label-key-alternative branch from 4cce848 to 6b251ce Compare March 3, 2023 02:04
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2023
@Huang-Wei
Copy link
Copy Markdown
Member

Huang-Wei commented Jul 12, 2023

Scheduling changes LGTM.

cc @kubernetes/sig-api-machinery-api-reviews for API reviews.

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Jul 13, 2023

api-machinery isn't API reviews :)

@deads2k can review the API bit

@Huang-Wei
Copy link
Copy Markdown
Member

Huang-Wei commented Jul 13, 2023

api-machinery isn't API reviews :)

Hmm... I didn't understand that. Do that mean in the future we should be chasing individual API approver for API reviewing? (instead of route API review requests to sig-api-machinery)

@cici37
Copy link
Copy Markdown
Contributor

cici37 commented Jul 13, 2023

api-machinery isn't API reviews :)

Hmm... I didn't understand that. Do that mean in the future we should be chasing individual API approver for API reviewing? (instead of route API review requests to sig-api-machinery)

Hi @Huang-Wei , SIG api machinery is a peer SIG to SIG schedulling(https://github.com/kubernetes/community/tree/master/sig-api-machinery). And API review is a process we required for merging any API changes. Please refer to API review process for merging any API related changes. Thank you :)

@Huang-Wei
Copy link
Copy Markdown
Member

Huang-Wei commented Jul 13, 2023

Ah, sorry for the confusion. I should have requested api review using /label api-review, which is the official way.

@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Jul 18, 2023

Only rebase + regenerate protobuf etc.

@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Jul 18, 2023

/retest

1 similar comment
@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Jul 18, 2023

/retest

@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Jul 20, 2023

@deads2k kindly ping for the api-review.

sttts
sttts reviewed Jul 27, 2023
View reviewed changes
Comment thread pkg/apis/core/types.go
// a pod of the set of pods is running.
type PodAffinityTerm struct {
// A label query over a set of resources, in this case pods.
// If it's null, this PodAffinityTerm matches with no Pods.
Copy link
Copy Markdown
Contributor

@sttts sttts Jul 27, 2023

Choose a reason for hiding this comment

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

does this mean null means something different to unset?

Copy link
Copy Markdown
Member Author

@sanposhiho sanposhiho Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

When unset, here becomes null. And it's If it's null, that matches with no Pods, which is likely a mistake from users. So, in this sense, we should have made it required. But, given actually we've got it as an optional field now, we just added the warning for unset null labelSelector.
#115873

And, this comment is to clarify this point.

@kerthcet kerthcet mentioned this pull request Aug 6, 2023
Merged
@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Aug 9, 2023

@deads2k Hi, do you have time to review on this, or do we somehow need to find another assignee for api-review?

deads2k
deads2k reviewed Aug 15, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@deads2k deads2k left a comment

Choose a reason for hiding this comment

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

I'm going to do another pass, but a few initial questions to get my bearings.

Comment thread pkg/registry/core/pod/strategy.go
Comment thread staging/src/k8s.io/api/core/v1/types.go
Comment thread pkg/apis/core/validation/validation.go Outdated
@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Aug 15, 2023

Oh, and one other question. If I have a required matchLabelKey, how does the first pod get on any node?

@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Aug 23, 2023

@deads2k

Oh, and one other question. If I have a required matchLabelKey, how does the first pod get on any node?

Such Pod is handled like this - in short, a required PodAffinity is ignored so that the first Pod lands somewhere.

// This pod may be the first pod in a series that have affinity to themselves. In order
// to not leave such pods in pending state forever, we check that if no other pod
// in the cluster matches the namespace and selector of this pod, the pod matches
// its own terms, and the node has all the requested topologies, then we allow the pod
// to pass the affinity check.

kubernetes/pkg/scheduler/framework/plugins/interpodaffinity/filtering.go

Lines 351 to 355 in fb785f1

// This pod may be the first pod in a series that have affinity to themselves. In order
// to not leave such pods in pending state forever, we check that if no other pod
// in the cluster matches the namespace and selector of this pod, the pod matches
// its own terms, and the node has all the requested topologies, then we allow the pod
// to pass the affinity check.

@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Sep 24, 2023

@deads2k Please retake a look.

deads2k
deads2k reviewed Oct 11, 2023
View reviewed changes
Comment thread pkg/registry/core/pod/strategy.go
deads2k
deads2k reviewed Oct 11, 2023
View reviewed changes
Comment thread pkg/apis/core/validation/validation.go
}
for _, matchExpression := range labelSelector.MatchExpressions {
key := matchExpression.Key
if i, ok := labelKeysMap[key]; ok && labelSelectorKeys.Has(key) {
Copy link
Copy Markdown
Contributor

@deads2k deads2k Oct 11, 2023

Choose a reason for hiding this comment

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

My memory of labelSelector validation is that the .MatchLabels and .MatchExpressions are managed independently. so you can have a labelSelector with MatchLabels and MatchExpressions that check different labels. See https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/validation/validation.go#L56-L66

Does something force sameness inside of affinity terms?

Copy link
Copy Markdown
Member Author

@sanposhiho sanposhiho Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we're using the same validation function to validate the selector in affinity actually.

kubernetes/pkg/apis/core/validation/validation.go

Line 7592 in da12d7a

allErrs = append(allErrs, unversionedvalidation.ValidateLabelSelector(podAffinityTerm.LabelSelector, labelSelectorValidationOptions, fldPath.Child("labelSelector"))...)

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Oct 11, 2023

The implementation is creative, but given the immutability of PodAffinity and PodAntiAffinity on pod update, I think it is acceptable. Please add a comment indicating that changes to immutability of PodAffinity and PodAntiAffinity require determining how to reconcile this implementation: this area looks about right https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/validation/validation.go#L4894.

Some confirmation of scheduler behavior and whether validation covers all cases requested.

@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Oct 12, 2023

@deads2k I put the comment as suggested. PTAL again.

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Oct 16, 2023

API looks good

/lgtm
/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 16, 2023

LGTM label has been added.

DetailsGit tree hash: 147b0819b8afda3ae0d08837342ee902c4c9d937

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, sanposhiho

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

The pull request process is described here

Details 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

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Oct 16, 2023

/hold

hold in case the sig wants another look

Huang-Wei
Huang-Wei reviewed Oct 16, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Huang-Wei Huang-Wei left a comment

Choose a reason for hiding this comment

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

some nits. LGTM otherwise.

Comment thread test/integration/scheduler/filters/filters_test.go Outdated
Comment thread test/integration/scheduler/scoring/priorities_test.go Outdated
Comment thread pkg/registry/core/pod/strategy_test.go Outdated
Comment thread pkg/features/kube_features.go Outdated
Comment thread pkg/apis/core/types.go Outdated
@sanposhiho
Copy link
Copy Markdown
Member Author

sanposhiho commented Oct 18, 2023

@Huang-Wei @deads2k Thanks for reviewing. I addressed the comments from @Huang-Wei + squash + rebase.

@Huang-Wei
Copy link
Copy Markdown
Member

Huang-Wei commented Oct 18, 2023

/retest

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented Oct 23, 2023

/hold cancel
/lgtm

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Oct 23, 2023

LGTM label has been added.

DetailsGit tree hash: 7ed50cc0444eb32f6fe6a9ec5b153686704d9d67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sttts sttts sttts left review comments

@chendave chendave Awaiting requested review from chendave

@cici37 cici37 Awaiting requested review from cici37

@alculquicondor alculquicondor Awaiting requested review from alculquicondor

@deads2k deads2k Awaiting requested review from deads2k

+3 more reviewers

@Huang-Wei Huang-Wei Huang-Wei left review comments

@denkensk denkensk denkensk left review comments

@ahg-g ahg-g ahg-g left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@alculquicondor alculquicondor

@deads2k deads2k

Labels

api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation 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/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. 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/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. 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

API Reviews
Status: API review completed, 1.29
SIG Apps
Archived in project
SIG Scheduling
Archived in project

Milestone

v1.29

Development

Successfully merging this pull request may close these issues.

Introduce MatchLabelKeys (the same one as Pod Topology Spread) to PodAffinityTerm