Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

add intersection behavior for RSP #1393

Merged
merged 3 commits into from
Jun 10, 2021
Merged

add intersection behavior for RSP #1393

merged 3 commits into from
Jun 10, 2021

Conversation

xunpan
Copy link
Contributor

@xunpan xunpan commented Apr 5, 2021

What this PR does / why we need it:
We add intersection behavior because it makes sense to result placement for resource propagation.

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 #981

Special notes for your reviewer:

  • placement computing rule is get from current federated resource calculation. That means the related rule is reused in RSP for placement calculation
  • I'm trying to add test and add test cases. So, please review when related tasks are done.

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Apr 5, 2021
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2021
@xunpan xunpan force-pushed the new branch 2 times, most recently from 634d607 to 3a6f119 Compare April 7, 2021 10:40
@xunpan
Copy link
Contributor Author

xunpan commented Apr 7, 2021

/retest

@xunpan xunpan force-pushed the new branch 4 times, most recently from 25bd481 to a84e4a5 Compare April 12, 2021 03:16
@xunpan
Copy link
Contributor Author

xunpan commented Apr 12, 2021

/retest

@xunpan
Copy link
Contributor Author

xunpan commented Apr 12, 2021

/test

@k8s-ci-robot
Copy link
Contributor

@xunpan: No presubmit jobs available for kubernetes-sigs/kubefed@master

In response to this:

/test

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.

@xunpan xunpan changed the title WIP add intersection behavior for RSP add intersection behavior for RSP Apr 12, 2021
@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 Apr 12, 2021
@xunpan
Copy link
Contributor Author

xunpan commented Apr 12, 2021

@irfanurrehman
It is ready to review.

@irfanurrehman
Copy link
Contributor

Thanks for doing this @xunpan. I will positively take a look at this tomorrow.

@irfanurrehman irfanurrehman requested review from makkes, hectorj2f, jimmidyson and irfanurrehman and removed request for font April 12, 2021 05:37
irfanurrehman
irfanurrehman suggested changes Apr 13, 2021
View reviewed changes
Copy link
Contributor

@irfanurrehman irfanurrehman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@xunpan Many thanks for spending time on this.
However, I think there is some rework that would be needed on this PR. Please take a look at my comments.

charts/kubefed/charts/controllermanager/crds/crds.yaml Outdated Show resolved Hide resolved
pkg/apis/scheduling/v1alpha1/replicaschedulingpreference_types.go Outdated Show resolved Hide resolved
pkg/apis/scheduling/v1alpha1/replicaschedulingpreference_types.go Outdated Show resolved Hide resolved
pkg/schedulingtypes/replicascheduler.go Outdated Show resolved Hide resolved
charts/kubefed/charts/controllermanager/crds/crds.yaml Outdated Show resolved Hide resolved
pkg/controller/util/placement.go Outdated Show resolved Hide resolved
pkg/schedulingtypes/plugin.go Outdated
if p.typeConfig.GetNamespaced() {
return util.ComputeNamespacedPlacement(fedObject, fedNsObject, clusters, p.limitedScope)
}
return util.ComputePlacement(fedObject, clusters)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please see my comment above about using this as is here. I think we need a separate function which gets only the clusterselector from the target resource. We are trying to intersect with the clusterselector only, not the clusternames.

pkg/schedulingtypes/replicascheduler.go
return ctlutil.StatusError
}

preferredClusters := []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could not understand the need of preferredClusters it seems to be a copy of resultClusters

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: @xunpan this is still unresolved but I will approve as this does not seem to harm the logic at all.

test/e2e/scheduling.go Outdated Show resolved Hide resolved
hectorj2f
hectorj2f reviewed Apr 19, 2021
View reviewed changes
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

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

Could you add some documentation with an example as part of our current docs ?

@hectorj2f
Copy link
Contributor

@xunpan Could you address the requested changes by @irfanurrehman ?

@xunpan
Copy link
Contributor Author

xunpan commented May 6, 2021

Yes. The code was done and I'm testing it locally. The updated diff will be committed by this week.

@irfanurrehman
Copy link
Contributor

Yes. The code was done and I'm testing it locally. The updated diff will be committed by this week.

@xunpan I missed checking on this. I see that you did some updates. Were they finalised by you? Should I check them again?

@xunpan
Copy link
Contributor Author

xunpan commented Jun 3, 2021

@irfanurrehman I forgot to send you a message after update the PR.

I think I addressed your comments above. One does not changed:
[Q] preferredClusters it seems to be a copy of resultClusters
[A] It is used to transform sets.String to string slice

@irfanurrehman
Copy link
Contributor

Thanks for doing this @xunpan.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irfanurrehman, xunpan

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:
  • OWNERS [irfanurrehman,xunpan]

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 merged commit 33b7f00 into kubernetes-retired:master Jun 10, 2021
@irfanurrehman irfanurrehman mentioned this pull request Sep 6, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hectorj2f hectorj2f hectorj2f left review comments

@irfanurrehman irfanurrehman irfanurrehman approved these changes

@gyliu513 gyliu513 Awaiting requested review from gyliu513

@makkes makkes Awaiting requested review from makkes

@jimmidyson jimmidyson Awaiting requested review from jimmidyson

Assignees

@irfanurrehman irfanurrehman

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 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.

clusterSelector is ignored with ReplicaSchedulingPreference
4 participants
@xunpan @k8s-ci-robot @irfanurrehman @hectorj2f