make webhook to use local nad cache #106
Conversation
pperiyasamy
changed the title
|
/cc @JanScheurich |
pkg/tools/cache.go
Outdated
| &sync.Mutex{}, make(chan struct{})} | ||
| } | ||
|
|
||
| // Start start the informer for NetworkAttachmentDefinition, upon events populate the cache |
pkg/tools/cache.go
Outdated
|
|
||
| // Start start the informer for NetworkAttachmentDefinition, upon events populate the cache | ||
| func (nc *NetAttachDefCache) Start() { | ||
| factory := externalversions.NewFilteredSharedInformerFactory(setupNetAttachDefClient(), 0, "", func(o *metav1.ListOptions) {}) |
There was a problem hiding this comment.
I see the autogenerated func comment says this func is depreciated and to use NewSharedInformerFactoryWithOptions instead. I am worried about this function being phased out in future releases and we may have broken code here. Can you switch to NewSharedInformerFactoryWithOptions - seems simple?
|
|
||
| // Stop stop the NetworkAttachmentDefinition informer | ||
| func (nc *NetAttachDefCache) Stop() { | ||
| close(nc.stopper) |
There was a problem hiding this comment.
I thought the informer would force the user who is calling the "Run" function to pass a channel which would signal back to main thread when it has ended following the signal to the informer to stop here.
I was just thinking if we close this, and then if the main process ends before the informer go routine ends, then it will be killed without finishing naturally. Did you look into this? Looks like nothing you can do so I am not saying to rework this.
There was a problem hiding this comment.
good find! now added a code for graceful shutdown of nad cache with atomic isRunning variable, hope that's ok now.
pkg/tools/cache.go
Outdated
|
|
||
| func (nc *NetAttachDefCache) remove(networkName string) { | ||
| nc.networkAnnotationsMapMutex.Lock() | ||
| delete(nc.networkAnnotationsMap[networkName], networkName) |
There was a problem hiding this comment.
is this right? shouldn't it be delete(nc.networkAnnotationsMap, networkName)
pkg/tools/cache.go
Outdated
|
|
||
| // Get returns annotations map for the given network name, if it's not available | ||
| // return nil | ||
| func (nc *NetAttachDefCache) Get(networkName string) map[string]string { |
There was a problem hiding this comment.
small nit - feel free to ignore: up to you if you want to change it - it would be better to have two arguments here I think - namespace and nad name, then you can bury the joining of the two strings net.Namespace + "/" + net.Name in this function instead of having to do it consistently when calling it like I see below. WDYT?
There was a problem hiding this comment.
yes, it makes sense. done.
pkg/webhook/webhook.go
Outdated
| glog.Error(reason) | ||
| return reqs, nsMap, reason | ||
| var annotationsMap map[string]string | ||
| annotationsMap = nadCache.Get(net.Namespace + "/" + net.Name) |
There was a problem hiding this comment.
nit: why not ":=" save one line :P
Signed-off-by: Periyasamy Palanisamy <[email protected]>
Signed-off-by: Periyasamy Palanisamy <[email protected]>
pkg/tools/cache.go
Outdated
| // Stop teardown the NetworkAttachmentDefinition informer | ||
| func (nc *NetAttachDefCache) Stop() { | ||
| close(nc.stopper) | ||
| func(limit time.Duration) { |
There was a problem hiding this comment.
why wrap this logic in a func? I think it makes it harder to read versus a normal loop.
| atomic.StoreInt32(&(nc.isRunning), int32(1)) | ||
| // informer Run blocks until informer is stopped | ||
| glog.Infof("starting net-attach-def informer") | ||
| informer.Run(nc.stopper) |
There was a problem hiding this comment.
Can we put in a log after .Run "net-attach-def informer stopped"? It may aid debugging.
Signed-off-by: Periyasamy Palanisamy <[email protected]>
There was a problem hiding this comment.
I have verified this change with #98 ! Thanks @pperiyasamy for all your hard work and changes here! I am waiting on @zshi-redhat to review this now as there are changes since he last approved.
| // Stop teardown the NetworkAttachmentDefinition informer | ||
| func (nc *NetAttachDefCache) Stop() { | ||
| close(nc.stopper) | ||
| tEnd := time.Now().Add(3 * time.Second) |
There was a problem hiding this comment.
Could you explain why you choose 3 seconds? Is this enough to stop net-attach-def informer?
There was a problem hiding this comment.
@MichalGuzieniuk thanks for looking at this. I just chose 3 seconds based on Kubelet's default shutdown grace period (i.e. 30 sec) for a pod. but, yes, if net-attach-def events are not yet flushed away from informer then it might take a while for informer to return after close.
there is no much activity happening in this cache module apart from updating its local map, do you see this as a problem or any other concern ?
There was a problem hiding this comment.
Fine for me. I was just interested if you choose this value because in tests you discovered that is the best or had other reason behind.
Thanks for all the changes and reviews! |
this implements local cache using network-attachment-definition-client's informer, web hook looks up this cache, fall back to K8 api when entry is not found.
Fixes #101
Signed-off-by: Periyasamy Palanisamy [email protected]