Skip to content
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

controller/engine: add queueing index method to NamedController #672

Closed
wants to merge 2 commits into from
Closed

controller/engine: add queueing index method to NamedController #672

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f2041d
controller/engine: add queueing index method to NamedController
sttts Feb 22, 2024
956937e
testing: add reader and cache mock
sttts Feb 23, 2024
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
35 changes: 35 additions & 0 deletions pkg/controller/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,15 +187,33 @@ func (e *Engine) Start(name string, o controller.Options, w ...Watch) error {
// NamedController is a controller that's not yet started. It gives access to
// the underlying cache, which may be used e.g. to add indexes.
type NamedController interface {
// Start the named controller. Start does not block.
Start(ctx context.Context) error
// GetCache returns the cache used by the named controller. If the
// controller hasn't been started, neither has the cache.
//
// Warning: Be careful when calling methods with a context before the
// controller has been started. The pass context will be used to e.g. start
// informers, and the life-cycle will differ from that of the controller. In
// particular, use IndexField instead to add indexes to the cache before the
// controller is started.
GetCache() cache.Cache
IndexField(obj client.Object, field string, extractValue client.IndexerFunc)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a doc string for this method, since the others all have them.

}

type namedController struct {
name string
e *Engine
ca cache.Cache
ctrl controller.Controller

indexes []index
}

type index struct {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to export this type and have Start take a variadic array of it? I'm not sure how this is called, so I'm happy if it's more ergonomic to use the method approach instead.

e.g.:

c.Start(ctx,
    Index{Object: o, Field: "spec.hi", ExtractValue: ifn},
    Index{Object: o, Field: "spec.hello", ExtractValue: ifn2})

obj client.Object
field string
extractValue client.IndexerFunc
}

// Create the named controller. Each controller gets its own cache
Expand Down Expand Up @@ -265,6 +283,12 @@ func (c *namedController) Start(ctx context.Context) error {
c.e.errors[c.name] = nil
c.e.mx.Unlock()

for _, idx := range c.indexes {
if err := c.ca.IndexField(ctx, idx.obj, idx.field, idx.extractValue); err != nil {
return errors.Wrap(err, "cannot add index")
}
}

go func() {
<-c.e.mgr.Elected()
c.e.done(c.name, errors.Wrap(c.ca.Start(ctx), errCrashCache))
Expand All @@ -281,7 +305,18 @@ func (c *namedController) Start(ctx context.Context) error {
return nil
}

// IndexField queues an index for addition to the cache on start.
Copy link
Member

Choose a reason for hiding this comment

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

Would QueueIndexField or IndexFieldOnStart or something similar make this more self-descriptive?

What happens if this is called after the controller/cache is started? Nothing I guess? (Worth mentioning in doc string.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, standby with this PR. The context passed to GetInformer and with that to IndexField is only used for waiting, not for the informer life-cycle. I think this was a misunderstanding.

func (c *namedController) IndexField(obj client.Object, field string, extractValue client.IndexerFunc) {
c.indexes = append(c.indexes, index{obj: obj, field: field, extractValue: extractValue})
}

// GetCache returns the cache used by the named controller.
//
// Warning: Be careful when calling methods with a context before the controller
// has been started. The pass context will be used to e.g. start informers, and
// the life-cycle will differ from that of the controller. In particular, use
// IndexField instead to add indexes to the cache before the controller is
// started.
func (c *namedController) GetCache() cache.Cache {
return c.ca
}
Loading