WIP Viewer framework for resource status #825
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: njhale The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
There was a problem hiding this comment.
This is looking good! excited to start using this
I wrote this out for myself when reviewing to remind myself of a desired state at a high level. sharing here for reference (but it doesn't map 1-1 with the abstractions here)
func (o *Operator) syncSubscription(sub *v1alpha1.Subscription) error {
sub.SetCatalogStatus(o.views["catalogstatus"].ByIndex("namespacePlusGlobal").Get(sub.GetNamespace()))
}
func (o *Operator) syncCatalogSource(catsrc *v1alpha1.CatalogSource) {
// check health, etc
healthy := true
catstatuskey := catsrc.GetNamespace() + "/" + catsrc.GetName()
// set value in the view
o.views["catalogstatus"].ByIndex("catatalogsource").Set(catstatuskey, NewCatSrcStatusView(catsrc, healthy))
// update indexes
if catsrc.IsGlobal() {
for namespace := range o.lister.CoreV1().NamespaceLister().List(metav1.ListOptions{}) {
o.views["catalogstatus"].ByIndex("namespacePlusGlobal").SetKey(namespace, catsrckey)
}
} else {
o.views["catalogstatus"].ByIndex("namespacePlusGlobal").SetKey(catsrc.GetNamespace(), catsrckey)
}
}
func (o *Operator) syncCatSrcStatusView(viewkey CatSrcStatusViewKey) {
for subkey := range o.views["catalogstatus"].ByReverseIndex("subscriptions").List(viewkey) {
o.requeue("subscriptions", subkey)
}
}|
|
||
| // RemoveSubscriptionCatalogStatus removes the SubscriptionCatalogStatus matching the given ObjectReference from a SubscriptionStatus | ||
| // and returns true if the status was removed; false otherwise. | ||
| func (status *SubscriptionStatus) RemoveSubscriptionCatalogStatus(target *corev1.ObjectReference) bool { |
There was a problem hiding this comment.
This isn't used yet, but ideally we don't have Add/Remove (just Set)
| } | ||
|
|
||
| // Views are a set of ViewFuncs keyed by their resource type | ||
| type Views map[reflect.Type]View |
There was a problem hiding this comment.
on the fence, but it might make sense to have this be a string key instead. avoids reflection and would mean we could register multiple views per type.
There was a problem hiding this comment.
The key of this map is intended to be the view type, not the resource type. So this should still let us register multiple views per resource type.
There was a problem hiding this comment.
// Views are a set of ViewFuncs keyed by their resource type
I think this comment got me :)
There was a problem hiding this comment.
Totally understand - I think the interface itself is ambiguous too.
| return nil, err | ||
| } | ||
|
|
||
| key, err := view.Key(value) |
There was a problem hiding this comment.
I was thinking that we want to get the value of what's stored in the cache rather than a fresh view.
There was a problem hiding this comment.
My last comment was wrong, sorry.
What I meant was:
Why not have an interface for getting a key directly from an object?
i.e.
type View interface {
Key(obj interface{}) (key string, err error)
Value(obj interface{}) (value interface{}, err error)
}vs
type View interface {
Key(value interface{}) (key string, err error)
Value(obj interface{}) (value interface{}, err error)
}so that here, you would just write view.Key(obj) and not need to get the value at all.
There was a problem hiding this comment.
Ah, got it - makes sense. We probably want to add another function for getting the key from the "view" itself (to satisfy some internal requirements of the underlying cache implementation).
| reconcilerFactory reconciler.RegistryReconcilerFactory | ||
| } | ||
|
|
||
| func NewSubCatalogView(configmapRegistryImage string, options ...viewOption) *subCatalogView { |
There was a problem hiding this comment.
nit, but maybe we want to call these ViewBuilders or Viewers and call the output of .Value() be the View object?
There was a problem hiding this comment.
Definitely want better names with less overlap.
njhale
force-pushed
the
viewer-framework
branch
from
9070105
to
ddab521
Compare
openshift-ci-robot
added
size/XL
njhale
force-pushed
the
viewer-framework
branch
9 times, most recently
from
8f9fb3d
to
c0472bf
Compare
openshift-ci-robot
added
size/XXL
njhale
force-pushed
the
viewer-framework
branch
4 times, most recently
from
bb56f32
to
7f08c5d
Compare
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
@njhale: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
|
@njhale: PR needs rebase. 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 kubernetes/test-infra repository. |
openshift-ci-robot
added
the
needs-rebase
|
superseded by #881. |
Currently, this PR represents an experiment in caching alternate views of resources.
Goal
Provide a framework that makes it easy to:
Test Case
The first test case for the framework is to provide
SubscriptionCatalogStatusupdates to dependentSubscriptions.Implementation
The experimental code is located in
pkg/controller/operators/catalog/scratch.go.