API-1835: Scaffold dynamic informers - #7477
Conversation
|
@p0lyn0mial: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds a non-exported inputResourceInitializer that discovers and validates operator input resources, resolves GVKs via the management RESTMapper, starts deduplicated informers on the management cache and waits for cache sync; wires the initializer into controller setup, adds unit tests, and updates go.mod. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
Comment |
|
/assign @benluddy @bertinatto @csrwng |
| } | ||
|
|
||
| func (r *inputResourceInitializer) discoverInputResources() (map[string]*libraryinputresources.InputResources, error) { | ||
| return nil, fmt.Errorf("not implemented") |
There was a problem hiding this comment.
@benluddy this is where we could use the file sytem queue client to discover the input-res per operator.
| } | ||
|
|
||
| if registeredGVK.Has(gvk.String()) { | ||
| continue |
There was a problem hiding this comment.
so that we don't register multiple event handlers for the same gvk
There was a problem hiding this comment.
We could register multiple even handlers once we have more than one operator, right? Would make sense to move the declaration of registeredGVK out of the loop?
There was a problem hiding this comment.
I think that gvk can only be registered once. For operators requiring the same gvk we will register multiple filters.
| // | ||
| // TODO: in the future we need to extend to full list | ||
| registeredGVK := sets.NewString() | ||
| for _, exactResource := range resources.ApplyConfigurationResources.ExactResources { |
There was a problem hiding this comment.
for the poc we only support ApplyConfigurationResources.ExactResources in the future we need to cover the entire list
| return ctx.Err() | ||
| } | ||
| return fmt.Errorf("caches did not sync") | ||
| } |
There was a problem hiding this comment.
i forgot to add a TODO but at the end we need to send a signal to the main controller the caches are ready.
| if err != nil { | ||
| return err | ||
| } | ||
| // TODO: register informer event handlers |
There was a problem hiding this comment.
events will be passed to a different component that will be responsible for mapping resources to an operator and trigger the main controller. something like "if X resource(s) change then call/rigger this operator"
| "context" | ||
| "fmt" | ||
|
|
||
| "github.com/openshift/multi-operator-manager/pkg/library/libraryinputresources" |
There was a problem hiding this comment.
we will also pull a new dep
There was a problem hiding this comment.
Do modules that depend on multi-operator-manager need to keep their Kube dependencies in sync with multi-operator-manager's Kube dependencies? I don't know if we will (in the future) want/need to either (a) avoid a specific dependency in multi-operator-manager or (b) split multi-operator-manager into separate modules for e.g. parent, child, API.
There was a problem hiding this comment.
yeah, I think the modules need to keep their Kube deps in sync with the multi-operator-manager’s Kube deps.
this is a similar situation to what we already have today with library-go, which is used by HyperShift and also depends on a specific Kube version.
moreover, multi-operator-manager also uses library-go.
because of these intertwined dependencies, I think that we will be forced to keep all of the deps at the same level.
ab546fb to
29cde6f
Compare
|
@p0lyn0mial: This pull request references API-1835 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
29cde6f to
b6fda8d
Compare
|
/approve Looks like you need to fix your commit messages. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, p0lyn0mial The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
b6fda8d to
b153504
Compare
|
/verified by CI |
|
/verified by CI |
|
@p0lyn0mial: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
bd90b05 to
2a4b59b
Compare
|
/verified by CI |
|
@p0lyn0mial: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@control-plane-operator/controllers/openshiftmanager/input_resource_initializer.go`:
- Around line 36-49: The discoverInputResources function currently returns a
"not implemented" error which causes inputResourceInitializer.Start to always
fail; change discoverInputResources to return an empty
map[string]*libraryinputresources.InputResources and a nil error (i.e., make it
a no-op stub) so Start can proceed to checkSupportedInputResources and
startAndWaitForInformersFor without failing; update the function body of
discoverInputResources (referenced by inputResourceInitializer.Start) to return
make(map[string]*libraryinputresources.InputResources), nil.
♻️ Duplicate comments (1)
control-plane-operator/controllers/openshiftmanager/input_resource_initializer.go (1)
51-86: Deduplicate GVK registrations across operators.
registeredGVKis reset per-operator, so shared GVKs across operators will repeatGetInformerForKindcalls and could lead to duplicate event handlers later. Consider de-duplicating across all operators.♻️ Suggested refactor
func (r *inputResourceInitializer) startAndWaitForInformersFor(ctx context.Context, inputResources map[string]*libraryinputresources.InputResources) error { - for operator, resources := range inputResources { + registeredGVK := sets.NewString() + for operator, resources := range inputResources { // note that for the POC we are only interested in ApplyConfigurationResources.ExactResources // the checkSupportedInputResources ensures no other resources were provided. // // TODO: in the future we need to extend to full list - registeredGVK := sets.NewString() for _, exactResource := range resources.ApplyConfigurationResources.ExactResources { gvr := schema.GroupVersionResource{Group: exactResource.Group, Version: exactResource.Version, Resource: exactResource.Resource} gvk, err := r.managementClusterRESTMapper.KindFor(gvr)
| func (r *inputResourceInitializer) Start(ctx context.Context) error { | ||
| inputResources, err := r.discoverInputResources() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err = r.checkSupportedInputResources(inputResources); err != nil { | ||
| return err | ||
| } | ||
| return r.startAndWaitForInformersFor(ctx, inputResources) | ||
| } | ||
|
|
||
| func (r *inputResourceInitializer) discoverInputResources() (map[string]*libraryinputresources.InputResources, error) { | ||
| return nil, fmt.Errorf("not implemented") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n control-plane-operator/controllers/openshiftmanager/input_resource_initializer.goRepository: openshift/hypershift
Length of output: 6116
🏁 Script executed:
cd control-plane-operator/controllers/openshiftmanager && rg -A 5 -B 5 "newInputResourceInitializer|inputResourceInitializer" --type goRepository: openshift/hypershift
Length of output: 6171
🏁 Script executed:
cd control-plane-operator && rg -B 5 -A 5 "SetupWithManager" controllers/openshiftmanager/openshiftmanager_controller.goRepository: openshift/hypershift
Length of output: 426
🏁 Script executed:
cd control-plane-operator/controllers/openshiftmanager && git log --oneline -n 20 input_resource_initializer.goRepository: openshift/hypershift
Length of output: 114
🏁 Script executed:
cd control-plane-operator/controllers/openshiftmanager && git log --oneline -n 50 | head -20Repository: openshift/hypershift
Length of output: 114
🏁 Script executed:
cd control-plane-operator/controllers/openshiftmanager && git show 2a4b59b46 --statRepository: openshift/hypershift
Length of output: 50378
🏁 Script executed:
cd control-plane-operator/controllers/openshiftmanager && grep -r "SetupWithManager\|Add.*inputResInitializer" --include="*.go" -B 10 -A 2Repository: openshift/hypershift
Length of output: 1002
🏁 Script executed:
cd control-plane-operator && rg "SetupWithManager.*OpenShiftManager\|openshiftmanager_controller.Reconciler" --include="*.go" -B 5 -A 5Repository: openshift/hypershift
Length of output: 505
🏁 Script executed:
cd control-plane-operator && rg "SetupWithManager" -t go -B 5 -A 5 | head -50Repository: openshift/hypershift
Length of output: 2730
🏁 Script executed:
cd control-plane-operator && rg "openshiftmanager.*Reconciler\|OpenShiftManager" -t go -B 3 -A 3Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
cd control-plane-operator && rg "openshiftmanager" -t go -iRepository: openshift/hypershift
Length of output: 558
🏁 Script executed:
cd control-plane-operator && rg "openshiftmanager.Reconciler\|controllers/openshiftmanager" -t go -B 5 -A 5Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
cd control-plane-operator && find . -name "main.go" -type f | xargs grep -l "openshiftmanager\|OpenShiftManager" 2>/dev/nullRepository: openshift/hypershift
Length of output: 48
🏁 Script executed:
cd control-plane-operator && find . -name "*.go" -type f | xargs grep -l "SetupWithManager" | head -5Repository: openshift/hypershift
Length of output: 352
🏁 Script executed:
cd control-plane-operator && find . -name "main.go" | xargs cat | grep -B 10 -A 10 "openshiftmanager"Repository: openshift/hypershift
Length of output: 46
🏁 Script executed:
cd control-plane-operator && find . -name "main.go" -o -name "setup.go" | head -5Repository: openshift/hypershift
Length of output: 364
Return empty map instead of error to prevent startup failure.
The Start method will always fail with the "not implemented" error from discoverInputResources, causing manager startup to fail whenever this initializer is enabled. The suggested fix to return an empty map is the appropriate mitigation until discovery is implemented.
🛠️ Suggested no-op stub to avoid startup failure
func (r *inputResourceInitializer) discoverInputResources() (map[string]*libraryinputresources.InputResources, error) {
- return nil, fmt.Errorf("not implemented")
+ // TODO: implement discovery; for now return no input resources.
+ return map[string]*libraryinputresources.InputResources{}, nil
}🤖 Prompt for AI Agents
In
`@control-plane-operator/controllers/openshiftmanager/input_resource_initializer.go`
around lines 36 - 49, The discoverInputResources function currently returns a
"not implemented" error which causes inputResourceInitializer.Start to always
fail; change discoverInputResources to return an empty
map[string]*libraryinputresources.InputResources and a nil error (i.e., make it
a no-op stub) so Start can proceed to checkSupportedInputResources and
startAndWaitForInformersFor without failing; update the function body of
discoverInputResources (referenced by inputResourceInitializer.Start) to return
make(map[string]*libraryinputresources.InputResources), nil.
|
|
||
| _, err = r.managementClusterCache.GetInformerForKind(ctx, gvk, cache.BlockUntilSynced(true)) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
nit: maybe add some context for this error?
There was a problem hiding this comment.
I can do that in a follow-up.
There was a problem hiding this comment.
Had to rebase (Konflux) the PR and decided to add more context.
| } | ||
|
|
||
| if registeredGVK.Has(gvk.String()) { | ||
| continue |
There was a problem hiding this comment.
We could register multiple even handlers once we have more than one operator, right? Would make sense to move the declaration of registeredGVK out of the loop?
bertinatto
left a comment
There was a problem hiding this comment.
/lgtm
I think we can follow-up with a different PR if we need other changes.
Adds an input resource initializer runnable for OpenShiftManager that discovers input resources and wires informers, and registers it in the controller setup. Notes: discovery is still a stub and we still need to register informer handlers and add filtering.
updated dependencies reqired by the changes introduced in 7477 PR.
2a4b59b to
e47fe05
Compare
|
/verified by CI |
|
@p0lyn0mial: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/retest-required |
|
/verified by CI |
|
@p0lyn0mial: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/lgtm |
|
@p0lyn0mial: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Adds an input resource initializer runnable for OpenShiftManager that discovers
input resources and wires informers, and registers it in the controller setup.
Notes: discovery is still a stub and we still need to register informer handlers and add filtering.
requires: #7445