CARM cache implementation #255
Conversation
a-hilaly
force-pushed
the
carm-impl
branch
3 times, most recently
from
923830c to
085dbf3
Compare
jaypipes
changed the title
a-hilaly
force-pushed
the
carm-impl
branch
3 times, most recently
from
48f6078 to
265f8c2
Compare
a-hilaly
force-pushed
the
carm-impl
branch
2 times, most recently
from
20acee9 to
6f265ae
Compare
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
I'm good with this as-is. Awesome work, @a-hilaly. @mhausenblas and @vijtrip2 would you mind doing a thorough review of this patch before I merge please?
pkg/runtime/cache/account.go
Outdated
| c.log.V(1).Info("cached ack-role-account-map data") | ||
| } | ||
| }, | ||
| UpdateFunc: func(old, new interface{}) { |
There was a problem hiding this comment.
teeny nit: new is a keyword in Go and naming variables the same as keywords (something called "variable shadowing" ) is generally discouraged. Might be better to name these orig and desired?
There was a problem hiding this comment.
Ahh, nice catch! I will change that
| return err | ||
| } | ||
| r.cache = ackrtcache.New(kc, r.log) | ||
| r.kc = mgr.GetClient() |
There was a problem hiding this comment.
Any reason why you did the above instead of just:
r.kc = mgr.GetClient()
r.cache = ackrtcache.New(r.kc, r.log)There was a problem hiding this comment.
my bad for using kc as a variable name ... clientset makes much more sense.
ackrtcache.New first parameter takes a client-go/kubernetes.Interface , while mgr.GetClient returns a client.Client (from sigs.k8s.io/controller-runtime/pkg/client)
There was a problem hiding this comment.
Changed kc to clientset
There was a problem hiding this comment.
Sorry, you misunderstood me :) I was saying that the clientset object is already being created on line 71 with:
r.kc = mgr.GetClient()so you don't have to create another one with the call to mgr.GetConfig() and then kubernetes.NewForConfig(). You can just pass the reconciler's kc attribute (which is the kubernetes clientset) to your ackrtcache.New() function:
r.kc = mgr.GetClient()
r.cache = ackrtcache.New(r.kc, r.log)There was a problem hiding this comment.
Ahhh i see now... i think i tried this approach before but something wasn't compiling
There was a problem hiding this comment.
i think it's because: client-go/kubernetes.NewForConfig() returns a kubernetes.Interface, mgr.Client() returns a ctrlrt/client.Client and client.Client doesn't implement the kubernetes.Interface interface
here is the compile error i get:
cannot use r.kc (type "sigs.k8s.io/controller-runtime/pkg/client".Client) as type kubernetes.Interface in argument to "github.com/aws/aws-controllers-k8s/pkg/runtime/cache".New:
"sigs.k8s.io/controller-runtime/pkg/client".Client does not implement kubernetes.Interface (missing AdmissionregistrationV1 method)
There was a problem hiding this comment.
hmmm. interesting... OK, soon as I'm done with a customer call I'll look into perhaps a cleaner way to do this.
There was a problem hiding this comment.
Just closing this out, but I've asked @a-hilaly to create an issue with a TODO about this and we're going to merge this as-is.
mhausenblas
left a comment
mhausenblas
left a comment
There was a problem hiding this comment.
LGTM as well. Meta comment: we need to review the resource requests (non blocking).
Let's ship it!
- reconciler.getRegion now try lookup for CRs region annotation, namespace default region annotation before finally trying to use flag/env values. - Add pkg/runtime/cache.NamespacesCache object to simplify access to namespace annotations and keep the cache up to date whenever new changes are made. - Add pkg/runtime/cache.AccountsCache object to simplify access to CARM configmap and keep the cache up to date whenever new changes are made. - Add pkg/runtime/cache.Caches object to quickly create NamespacesCache and AccountsCache - Inject NAMESPACE environment variable to controller pods using k8s downward api
jaypipes
left a comment
jaypipes
left a comment
There was a problem hiding this comment.
See inline, sorry I wasn't clear.
| return err | ||
| } | ||
| r.cache = ackrtcache.New(kc, r.log) | ||
| r.kc = mgr.GetClient() |
There was a problem hiding this comment.
Sorry, you misunderstood me :) I was saying that the clientset object is already being created on line 71 with:
r.kc = mgr.GetClient()so you don't have to create another one with the call to mgr.GetConfig() and then kubernetes.NewForConfig(). You can just pass the reconciler's kc attribute (which is the kubernetes clientset) to your ackrtcache.New() function:
r.kc = mgr.GetClient()
r.cache = ackrtcache.New(r.kc, r.log)
| return err | ||
| } | ||
| r.cache = ackrtcache.New(kc, r.log) | ||
| r.kc = mgr.GetClient() |
There was a problem hiding this comment.
Just closing this out, but I've asked @a-hilaly to create an issue with a TODO about this and we're going to merge this as-is.
4 participants
Issue #, if available: #102
Description of changes:
Adding functionalities to help caching and accessing CARM configmap and namespace ACK related annotations.
ack-role-account-map) cacheBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.