-
Notifications
You must be signed in to change notification settings - Fork 366
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.
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
✨ Only schedule to synctarget with compatible APIs #2329
✨ Only schedule to synctarget with compatible APIs #2329
Conversation
pkg/reconciler/workload/placement/placement_reconcile_scheduling_test.go
Show resolved
Hide resolved
ea00e76
to
d789215
Compare
d789215
to
4b7c9d1
Compare
/assign @sttts |
a315436
to
c8d456d
Compare
pkg/reconciler/workload/apibindinglabel/apibindinglabel_controller.go
Outdated
Show resolved
Hide resolved
pkg/reconciler/workload/apibindinglabel/apibindinglabel_indexes.go
Outdated
Show resolved
Hide resolved
pkg/reconciler/workload/apibindinglabel/apibindinglabel_indexes.go
Outdated
Show resolved
Hide resolved
c281b5e
to
d386672
Compare
d386672
to
6f3eddb
Compare
Signed-off-by: Jian Qiu <jqiu@redhat.com>
d4549d7
to
d9a051b
Compare
/lgtm |
pkg/reconciler/workload/placement/placement_reconcile_scheduling.go
Outdated
Show resolved
Hide resolved
pkg/reconciler/workload/placement/placement_reconcile_scheduling.go
Outdated
Show resolved
Hide resolved
) | ||
|
||
const ( | ||
ControllerName = "kcp-api-export-extra-annotation-sync" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks like controller inflation. Why not add another reconciler to the existing apibinding controller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing apibinding controller only update status using committer. committer cannot update label and status once, and cannot remove the label key. If we add another reconciler in the apibinding controller, I think it will make it a bit messy, since we need to patch labels separately in this reconciler then return the updated placement to let other reconciler update status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have precedence for that, see the clusterworkspaces controller. The only rules to follow:
- each reconciler either modifies status or spec, not both
- all status reconcilers come first, and reconciliation it stopped (+ requeued) when spec is changed.
Signed-off-by: Jian Qiu <jqiu@redhat.com>
36dccd9
to
640ec60
Compare
pkg/reconciler/apis/extraannotationsync/apibindingannotation_controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: Jian Qiu <jqiu@redhat.com>
f35e23b
to
733c727
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: davidfestal, sttts 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 |
if placement.Status.Phase == schedulingv1alpha1.PlacementPending || placement.Status.SelectedLocation == nil { | ||
conditions.MarkFalse(placement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a big 🔴 . Never ever mutate in a getter.
} | ||
|
||
if len(filteredSyncTargets) == 0 { | ||
conditions.MarkFalse(placement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another one 🔴 . Don't mutate anything when it is not clearly expected by the function name.
add a filter to filter out synctarget whose supported resouces do not include what in the apibindings.
Signed-off-by: Jian Qiu jqiu@redhat.com
Summary
filter the synctarget based on what is the supported resource in synctarget and what is the served apibindings relating to compute in this workspace.
Related issue(s)
Fixes #1779