Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package openshiftmanager

import (
"context"
"fmt"

"github.com/openshift/multi-operator-manager/pkg/library/libraryinputresources"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we will also pull a new dep

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/sets"

"sigs.k8s.io/controller-runtime/pkg/cache"
)

// inputResourceInitializer is responsible for discovering input resources
// required by operators and starting the corresponding informers.
//
// once all informers are successfully started and fully synced,
// the initializer completes its execution.
//
// TODO: after informer synchronization send readiness signal to the main controller.
type inputResourceInitializer struct {
managementClusterRESTMapper meta.RESTMapper
managementClusterCache cache.Cache
}

func newInputResourceInitializer(mgmtClusterRESTMapper meta.RESTMapper, mgmtClusterCache cache.Cache) *inputResourceInitializer {
return &inputResourceInitializer{
managementClusterRESTMapper: mgmtClusterRESTMapper,
managementClusterCache: mgmtClusterCache,
}
}

func (r *inputResourceInitializer) Start(ctx context.Context) error {
Comment thread
p0lyn0mial marked this conversation as resolved.
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")
}
Comment on lines +36 to +49

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n control-plane-operator/controllers/openshiftmanager/input_resource_initializer.go

Repository: openshift/hypershift

Length of output: 6116


🏁 Script executed:

cd control-plane-operator/controllers/openshiftmanager && rg -A 5 -B 5 "newInputResourceInitializer|inputResourceInitializer" --type go

Repository: openshift/hypershift

Length of output: 6171


🏁 Script executed:

cd control-plane-operator && rg -B 5 -A 5 "SetupWithManager" controllers/openshiftmanager/openshiftmanager_controller.go

Repository: openshift/hypershift

Length of output: 426


🏁 Script executed:

cd control-plane-operator/controllers/openshiftmanager && git log --oneline -n 20 input_resource_initializer.go

Repository: openshift/hypershift

Length of output: 114


🏁 Script executed:

cd control-plane-operator/controllers/openshiftmanager && git log --oneline -n 50 | head -20

Repository: openshift/hypershift

Length of output: 114


🏁 Script executed:

cd control-plane-operator/controllers/openshiftmanager && git show 2a4b59b46 --stat

Repository: openshift/hypershift

Length of output: 50378


🏁 Script executed:

cd control-plane-operator/controllers/openshiftmanager && grep -r "SetupWithManager\|Add.*inputResInitializer" --include="*.go" -B 10 -A 2

Repository: openshift/hypershift

Length of output: 1002


🏁 Script executed:

cd control-plane-operator && rg "SetupWithManager.*OpenShiftManager\|openshiftmanager_controller.Reconciler" --include="*.go" -B 5 -A 5

Repository: openshift/hypershift

Length of output: 505


🏁 Script executed:

cd control-plane-operator && rg "SetupWithManager" -t go -B 5 -A 5 | head -50

Repository: openshift/hypershift

Length of output: 2730


🏁 Script executed:

cd control-plane-operator && rg "openshiftmanager.*Reconciler\|OpenShiftManager" -t go -B 3 -A 3

Repository: openshift/hypershift

Length of output: 46


🏁 Script executed:

cd control-plane-operator && rg "openshiftmanager" -t go -i

Repository: openshift/hypershift

Length of output: 558


🏁 Script executed:

cd control-plane-operator && rg "openshiftmanager.Reconciler\|controllers/openshiftmanager" -t go -B 5 -A 5

Repository: 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/null

Repository: openshift/hypershift

Length of output: 48


🏁 Script executed:

cd control-plane-operator && find . -name "*.go" -type f | xargs grep -l "SetupWithManager" | head -5

Repository: 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 -5

Repository: 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.


func (r *inputResourceInitializer) startAndWaitForInformersFor(ctx context.Context, inputResources map[string]*libraryinputresources.InputResources) error {
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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

for the poc we only support ApplyConfigurationResources.ExactResources in the future we need to cover the entire list

gvr := schema.GroupVersionResource{Group: exactResource.Group, Version: exactResource.Version, Resource: exactResource.Resource}
gvk, err := r.managementClusterRESTMapper.KindFor(gvr)
if err != nil {
return fmt.Errorf("unable to find Kind for: %#v, for: %s operator, err: %w", exactResource, operator, err)
}

if registeredGVK.Has(gvk.String()) {
continue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so that we don't register multiple event handlers for the same gvk

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think that gvk can only be registered once. For operators requiring the same gvk we will register multiple filters.

}

_, err = r.managementClusterCache.GetInformerForKind(ctx, gvk, cache.BlockUntilSynced(true))
if err != nil {
return fmt.Errorf("unable get an informer for gvk: %v, operator: %v, err: %w", gvk, operator, err)
}
// TODO: register informer event handlers

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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"


registeredGVK.Insert(gvk.String())
}
}

if !r.managementClusterCache.WaitForCacheSync(ctx) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Eventually, I think we will need to handle cases where there's a mix of synced and unsynced informer caches. If all of the caches that a certain child controller needs have synced, the parent can still trigger that controller without also triggering another controller for which all informer caches have not synced.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we could have something like that per operator.

Right now, startAndWaitForInformersFor is for a single operator, but in the future we would need a new method that takes the inputs for multiple operators into account.

if ctx.Err() != nil {
return ctx.Err()
}
return fmt.Errorf("caches did not sync")
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i forgot to add a TODO but at the end we need to send a signal to the main controller the caches are ready.

return nil
}

// checkSupportedInputResources ensures only supported resources are present.
// this method is useful only for the POC purposes.
// in the future we will not need this method.
func (r *inputResourceInitializer) checkSupportedInputResources(inputResources map[string]*libraryinputresources.InputResources) error {
isResourceListSupportedFunc := func(resList libraryinputresources.ResourceList, areExactResourcesSupported bool, fieldPath string) error {
if !areExactResourcesSupported && len(resList.ExactResources) > 0 {
return fmt.Errorf("%v.ExactResources are unsupported for now", fieldPath)
}

if !equality.Semantic.DeepEqual(resList, libraryinputresources.ResourceList{ExactResources: resList.ExactResources}) {
if len(resList.GeneratedNameResources) > 0 {
return fmt.Errorf("%v.GeneratedNameResources are unsupported for now", fieldPath)
}
if len(resList.LabelSelectedResources) > 0 {
return fmt.Errorf("%v.LabelSelectedResources are unsupported for now", fieldPath)
}
if len(resList.ResourceReferences) > 0 {
return fmt.Errorf("%v.ResourceReferences are unsupported for now", fieldPath)
}
return fmt.Errorf("%v has an unknown field(s) set", fieldPath)
}
return nil
}

toCommonErrMsgFunc := func(operator string, err error) error {
return fmt.Errorf("unsupported input resources found for %s operator: %w", operator, err)
}
for operator, inputResource := range inputResources {
if err := isResourceListSupportedFunc(inputResource.ApplyConfigurationResources, true, "ApplyConfigurationResources"); err != nil {
return toCommonErrMsgFunc(operator, err)
}
if err := isResourceListSupportedFunc(inputResource.OperandResources.ConfigurationResources, false, "OperandResources.ConfigurationResources"); err != nil {
return toCommonErrMsgFunc(operator, err)
}
if err := isResourceListSupportedFunc(inputResource.OperandResources.ManagementResources, false, "OperandResources.ManagementResources"); err != nil {
return toCommonErrMsgFunc(operator, err)
}
if err := isResourceListSupportedFunc(inputResource.OperandResources.UserWorkloadResources, false, "OperandResources.UserWorkloadResources"); err != nil {
return toCommonErrMsgFunc(operator, err)
}
}
return nil
}
Loading