-
Notifications
You must be signed in to change notification settings - Fork 859
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
feat: support use index when to list by labelSelector #2287
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: likakuli <1154584512@qq.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
||
"k8s.io/client-go/rest" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
"sigs.k8s.io/controller-runtime/pkg/manager" |
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.
Using controllerruntime "sigs.k8s.io/controller-runtime"
here because alias.go
is provided.
} | ||
|
||
type labelIndexerManager struct { | ||
manager.Manager |
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.
controllerruntime.Manager
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.
got it, thx
// https://github.com/kubernetes/kubernetes/pull/109334 | ||
// https://github.com/kubernetes-sigs/controller-runtime/pull/1838 | ||
func NewLabelIndexerManager(config *rest.Config, options manager.Options) (manager.Manager, error) { | ||
manager, err := manager.New(config, options) |
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.
controllerruntime.NewManager
// register label indexer | ||
ctx := context.Background() | ||
for obj, indexerFunc := range typeIndexerFuncMap { | ||
err := lim.Manager.GetFieldIndexer().IndexField(ctx, obj, "metadata.labels", indexerFunc) |
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.
transfer metadata.labels
to a constant.
// typeIndexerFuncMap contains internal type and its indexerFunc used to set up indexer. | ||
var typeIndexerFuncMap = map[client.Object]client.IndexerFunc{ | ||
&workv1alpha2.ResourceBinding{}: indexForResourceBinding(), | ||
&workv1alpha1.ResourceBinding{}: indexForResourceBinding(), |
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.
I think there is no need to index workv1alpha1.ResourceBinding
, workv1alpha1.ClusterResourceBinding
either.
@@ -148,7 +149,7 @@ func run(ctx context.Context, karmadaConfig karmadactl.KarmadaConfig, opts *opti | |||
return fmt.Errorf("failed to generate execution space name for member cluster %s, err is %v", opts.ClusterName, err) | |||
} | |||
|
|||
controllerManager, err := controllerruntime.NewManager(controlPlaneRestConfig, controllerruntime.Options{ | |||
controllerManager, err := manager.NewLabelIndexerManager(controlPlaneRestConfig, controllerruntime.Options{ |
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.
I don't know why we establish our own manager and cache in util/manager
. I think we only need to add indexer to this controllerManager. And I have implement the respective indexer in controllerManager in https://github.com/karmada-io/karmada/blob/master/pkg/controllers/cluster/common.go
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.
And here is the list function. The field client.Client
already use a cache here. I think there is no need to establish our own cache, right? It could make the code cleaner.
karmada/pkg/controllers/cluster/taint_manager.go
Lines 68 to 74 in 616033f
if err := tc.List(ctx, rbList, client.MatchingFieldsSelector{ | |
Selector: fields.OneTermEqualSelector(rbClusterKeyIndex, cluster.Name), | |
}); err != nil { | |
klog.ErrorS(err, "Failed to list ResourceBindings", "cluster", cluster.Name) | |
return controllerruntime.Result{Requeue: true}, err | |
} | |
for i := range rbList.Items { |
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.
thanks for your suggestion.
I think there are three reasons why add custom manager and cache.
There are many callers to call list func with labelSelector and the first reason is just don't want to modify too many callers.
The second is that when controller-runtime and client-go support use multi-indexer for labelSelector then what we
need to do is just use original manager and cache instead of our custom types.
The third is that it's a non-business logic. User just do what they need to do in its 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.
Combine your views, I think we don't need a new manager and only need to establish a new cache to meet @likakuli's three reasons.Because code about cache index only exists in controller-runtime/pkg/cache, don't need to edit manager.
We can add new cache's constructor in here
karmada/cmd/controller-manager/app/controllermanager.go
Lines 111 to 118 in 7cce95f
controllerManager, err := controllerruntime.NewManager(config, controllerruntime.Options{ | |
Logger: klog.Background(), | |
Scheme: gclient.NewSchema(), | |
SyncPeriod: &opts.ResyncPeriod.Duration, | |
LeaderElection: opts.LeaderElection.LeaderElect, | |
LeaderElectionID: opts.LeaderElection.ResourceName, | |
LeaderElectionNamespace: opts.LeaderElection.ResourceNamespace, | |
LeaseDuration: &opts.LeaderElection.LeaseDuration.Duration, |
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.
Or we can directly add indexer to controllerManager using https://github.com/karmada-io/karmada/blob/master/pkg/controllers/cluster/common.go. It's ok for me.
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.
Got it👌. We don't want to modify so many List
callers, so we build our own cache which implement the List
function.
Actually, I'm not sure about how much performace it will improve? I think all objects are in cache, and it seems like there have not been any perform issue so far. So is it worthy to implement our own cache and manager here? Just some questions.
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.
Our own cache will add index of some specific labels to optimize the List from O(n) to O(1).You can see https://github.com/kubernetes-sigs/controller-runtime/blob/f46919744bee01060c9084a285e049afffd38c9d/pkg/cache/internal/cache_reader.go#L135-L162
When List with labelselector,it use range and costs O(n).We now convert labelselector to filedselector for optimization.
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.
@Garrybest Do you think it is worth optimizing now? I want to finish it.
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.
I think it's necessary. What we need to do is thinking about when and how to do it. Also i think we should provide some best practice for users just like k8s. How many clusters we can support? How many work we can support? e.g. Optimizing may help to increase the cap.
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.
Glad to see this feature. Meanwhile, I hope we could change the basic cache or manager as little as possible.
cc @RainbowMango who may have some ideas about this patch.
/assign |
} | ||
} | ||
|
||
func indexForWork() client.IndexerFunc { |
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.
When List work objects with label FederatedResourceQuotaNamespaceLabel and FederatedResourceQuotaNameLabel
karmada/pkg/controllers/federatedresourcequota/federated_resource_quota_status_controller.go
Lines 65 to 72 in ad94a94
if err := c.List(context.TODO(), workList, &client.ListOptions{ | |
LabelSelector: labels.SelectorFromSet(labels.Set{ | |
util.FederatedResourceQuotaNamespaceLabel: quota.Namespace, | |
util.FederatedResourceQuotaNameLabel: quota.Name, | |
}), | |
}); err != nil { | |
klog.Errorf("Failed to list workList created by federatedResourceQuota(%s), error: %v", req.NamespacedName.String(), err) | |
} |
According to your index function,if the work object has three labels it may be cause failure.
Eg:
workobject:
[
labels:
"FederatedResourceQuotaNamespaceLabel":1,
"FederatedResourceQuotaNameLabel":2,
"ServiceNameLabel":3,
]
the index value is "FederatedResourceQuotaNamespaceLabel=1&FederatedResourceQuotaNameLabel=2&ServiceNameLabel=3",
but List function calculate the index value is "FederatedResourceQuotaNamespaceLabel=1&FederatedResourceQuotaNameLabel=2".
I'am afraid this will cause can't find the objects L in such situation.
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.
Yes. Is this possible that work both have the three labels above?
Signed-off-by: likakuli 1154584512@qq.com
What type of PR is this?
/kind feature
What this PR does / why we need it:
Support use index when to list object by labelSelector
Which issue(s) this PR fixes:
Fixes # #2234
Special notes for your reviewer:
Does this PR introduce a user-facing change?: