add watches for owned resources and CRBs #526
Conversation
|
/hold |
KPostOffice
force-pushed
the
watch4owned
branch
from
7bca5c8 to
f1a8a13
Compare
| @@ -220,7 +222,7 @@ func crbNameFromCluster(cluster *rayv1.RayCluster) string { | |||
| func desiredOAuthClusterRoleBinding(cluster *rayv1.RayCluster) *rbacapply.ClusterRoleBindingApplyConfiguration { | |||
| return rbacapply.ClusterRoleBinding( | |||
There was a problem hiding this comment.
I really wonder, can this be a RoleBinding instead of a ClusterRoleBinding?
There was a problem hiding this comment.
I believe I tried this a while back and it didn't work. I can give it another go though
KPostOffice
force-pushed
the
watch4owned
branch
2 times, most recently
from
afd3a10 to
d0eec5d
Compare
| @@ -233,7 +237,7 @@ func crbNameFromCluster(cluster *rayv1.RayCluster) string { | |||
| func desiredOAuthClusterRoleBinding(cluster *rayv1.RayCluster) *rbacapply.ClusterRoleBindingApplyConfiguration { | |||
| return rbacapply.ClusterRoleBinding( | |||
| crbNameFromCluster(cluster)). | |||
| WithLabels(map[string]string{"ray.io/cluster-name": cluster.Name}). | |||
| WithLabels(map[string]string{"ray.io/cluster-name": cluster.Name, "ray.io/cluster-namespace": cluster.Namespace}). | |||
There was a problem hiding this comment.
Can we scope the labels to ray.openshift.ai instead of ray.io that we do not own?
| func(c context.Context, o client.Object) []reconcile.Request { | ||
| return []reconcile.Request{{ | ||
| NamespacedName: client.ObjectKey{ | ||
| Name: o.GetLabels()["ray.io/cluster-name"], |
There was a problem hiding this comment.
Should we check for the absence of these labels? The watch will receive events for all CRBs. An alternative is to configure the cache to filter events only for our CRBs.
There was a problem hiding this comment.
I've changed it to check for the absence of the labels, I'm not sure how to configure the cache to filter for only our CRBs
There was a problem hiding this comment.
I've created https://github.com/project-codeflare/codeflare-operator/tree/pr-cache-example that shows how to configure the manager cache.
There was a problem hiding this comment.
Thanks Antonin, I'm adding this too so that it can be followed if someone finds this
https://github.com/kubernetes-sigs/controller-runtime/blob/main/designs/use-selectors-at-cache.md
There was a problem hiding this comment.
For the record, this broke the AppWrapper controller. I submitted a fix here #640
KPostOffice
force-pushed
the
watch4owned
branch
3 times, most recently
from
a778eb8 to
7b1490e
Compare
pkg/controllers/support.go
Outdated
| @@ -20,7 +20,7 @@ func serviceNameFromCluster(cluster *rayv1.RayCluster) string { | |||
|
|
|||
| func desiredRayClientRoute(cluster *rayv1.RayCluster) *routeapply.RouteApplyConfiguration { | |||
| return routeapply.Route(rayClientNameFromCluster(cluster), cluster.Namespace). | |||
| WithLabels(map[string]string{"ray.io/cluster-name": cluster.Name}). | |||
| WithLabels(map[string]string{"ray.openshift.ai/cluster-name": cluster.Name}). | |||
There was a problem hiding this comment.
Should that be RayClusterNameLabel?
There was a problem hiding this comment.
Yep, did a find and replace in the controller code. Missed this one. Thanks for catching
There was a problem hiding this comment.
I also added WithController to the OwnerReferences in support.go
|
/lgtm |
|
/lgtm |
KPostOffice
force-pushed
the
watch4owned
branch
3 times, most recently
from
6496d3f to
bad723c
Compare
|
/unhold |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: astefanutti 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 |
Signed-off-by: Kevin <[email protected]>
|
New changes are detected. LGTM label has been removed. |
|
/retest |
openshift-merge-bot
bot
merged commit 4673aeb
into
project-codeflare:main
Issue link
What changes have been made
I've added watches for owned resources so that the controller will automatically re-reconcile on updates to owned resources. I've also added a label to the CRB make mapping from CRB to RayCluster resource possible
Verification steps
Checks