Skip to content

Index metadata.name for cluster-name label #740

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 12, 2019
+4 −0
Merged

Index metadata.name for cluster-name label #740

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/controller/machine/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ func newReconciler(mgr manager.Manager, actuator Actuator) reconcile.Reconciler
actuator: actuator,
}

mgr.GetFieldIndexer().IndexField(&clusterv1.Cluster{}, "metadata.name", func(obj runtime.Object) []string {
Copy link
Member

@detiber detiber Feb 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Does it matter that IndexerFunc is meant to be unique across namespaces?

cluster-api/vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.go

Lines 91 to 93 in 0617f27

// IndexerFunc knows how to take an object and turn it into a series
// of (non-namespaced) keys for that object.
type IndexerFunc func(runtime.Object) []string

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that's taken care of in this wrapper

cluster-api/vendor/sigs.k8s.io/controller-runtime/pkg/cache/informer_cache.go

Lines 133 to 178 in 0617f27

func (ip *informerCache) IndexField(obj runtime.Object, field string, extractValue client.IndexerFunc) error {
informer, err := ip.GetInformer(obj)
if err != nil {
return err
}
return indexByField(informer.GetIndexer(), field, extractValue)
}
func indexByField(indexer cache.Indexer, field string, extractor client.IndexerFunc) error {
indexFunc := func(objRaw interface{}) ([]string, error) {
// TODO(directxman12): check if this is the correct type?
obj, isObj := objRaw.(runtime.Object)
if !isObj {
return nil, fmt.Errorf("object of type %T is not an Object", objRaw)
}
meta, err := apimeta.Accessor(obj)
if err != nil {
return nil, err
}
ns := meta.GetNamespace()
rawVals := extractor(obj)
var vals []string
if ns == "" {
// if we're not doubling the keys for the namespaced case, just re-use what was returned to us
vals = rawVals
} else {
// if we need to add non-namespaced versions too, double the length
vals = make([]string, len(rawVals)*2)
}
for i, rawVal := range rawVals {
// save a namespaced variant, so that we can ask
// "what are all the object matching a given index *in a given namespace*"
vals[i] = internal.KeyToNamespacedKey(ns, rawVal)
if ns != "" {
// if we have a namespace, also inject a special index key for listing
// regardless of the object namespace
vals[i+len(rawVals)] = internal.KeyToNamespacedKey("", rawVal)
}
}
return vals, nil
}
return indexer.AddIndexers(cache.Indexers{internal.FieldIndexName(field): indexFunc})
}

Copy link
Member

Choose a reason for hiding this comment

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

@vincepri From the looks of it, that field indexes both the namespaced and non-namespaced keys for each value, so wouldn't there be a collision for the non-namespaced value of two clusters with the same name in different namespaces?

Copy link
Contributor

Choose a reason for hiding this comment

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

there is another option to filter cluster object by using LabelSelector,
it does require adding a new label to cluster object.

// ListOptions contains options for limitting or filtering results.
// It's generally a subset of metav1.ListOptions, with support for
// pre-parsed selectors (since generally, selectors will be executed
// against the cache).
type ListOptions struct {
	// LabelSelector filters results by label.  Use SetLabelSelector to
	// set from raw string form.
	LabelSelector labels.Selector
	// FieldSelector filters results by a particular field.  In order
	// to use this with cache-based implementations, restrict usage to
	// a single field-value pair that's been added to the indexers.
	FieldSelector fields.Selector

have you considered this?
i guess my point is to avoid this extra indexing logic.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem that I see using labels is that it doesn't guarantee uniqueness and if we duplicate the name there we'd need to keep it in sync, which isn't ideal

return []string{obj.(*clusterv1.Cluster).Name}
})

if r.nodeName == "" {
klog.Warningf("environment variable %v is not set, this controller will not protect against deleting its own machine", NodeNameEnvVar)
}
Expand Down