-
Notifications
You must be signed in to change notification settings - Fork 482
Refactor dyamic cache to use namespaced informers #2522
Conversation
I would like to see the tracing output of this running against our development cluster, before and after. |
Not a scientific approach, it looks like the times to List on overview pages are roughly the same. For reference, this is on the team's development cluster on the default namespace. Informers for CRD's are not shutting down after |
81a3c62
to
637e994
Compare
d.gvrCache.Store(gk, gvr) | ||
} else { | ||
gvr, _ = v.(schema.GroupVersionResource) | ||
gvr, _, err := d.client.Resource(gk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we feel about creating a static list, like we have for GVKs that makes the Key to a GVR for known types and attempts to look it up for unknown types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out the special case for CRDs was due to improperly passing the version to RESTMapper.
We probably don't need this except for testing via https://github.com/kubernetes/client-go/blob/master/dynamic/fake/simple.go#L43
@@ -260,7 +260,8 @@ func getBoundPersistentVolume(ctx context.Context, pvc *corev1.PersistentVolumeC | |||
|
|||
pv, err := objectStore.Get(ctx, key) | |||
if err != nil { | |||
return nil, errors.Wrapf(err, "get volume for key %+v", key) | |||
// Return empty persistent volume instead of error in case the dynamic cache has not found the volume yet | |||
return persistentVolume, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember doing this way long ago to make the educates usage of Octant work, that we have bring this back in to make it work is a sign that things still aren't quite right.
We should be handling these errors in a better way.
Imagine using Octant thinking you had access to PVs or knowing that you created some but you just see empty .. no error, no indication that there was an error of anykind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -173,60 +169,6 @@ func TestNewRunnerLoadsValidKubeConfigFilteringNonexistent(t *testing.T) { | |||
require.Equal(t, "test-context", kubeConfigEvent.Data["currentContext"].(string)) | |||
} | |||
|
|||
func TestNewRunnerUsesClusterClient(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is these test being deleted? That wasn't clear to me?
If the watcher for CRDs are not shutting down properly I think we want to fix what ever is causing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added back this test
f1511d8
to
868d405
Compare
Signed-off-by: Sam Foo <foos@vmware.com>
What this PR does / why we need it:
Adds more clarity around the nuances of the informer cache and why various informer factories were used throughout the project. Also cleans up misc console warnings/errors.
Which issue(s) this PR fixes
Special notes for your reviewer:
Binary is available
curl -Lo octant https://storage.googleapis.com/octant-nightlies/octant
and can run in the RBAC restricted lab environment via./octant --namespace $SESSION_NAMESPACE --disable-open-browser --listener-addr 127.0.0.1:10086 --accepted-hosts $SESSION_NAMESPACE-console.$INGRESS_DOMAIN
Signed-off-by: Sam Foo foos@vmware.com