Use informers #16
Use informers #16
Conversation
ottoyiu
left a comment
ottoyiu
left a comment
There was a problem hiding this comment.
Some minor comments but otherwise lgtm (apart from test coverage)
| func (c *Controller) disableSrcDstCheck(key string) error { | ||
| defer c.workqueue.Done(key) | ||
|
|
||
| return retry.RetryOnConflict(retry.DefaultBackoff, func() error { |
There was a problem hiding this comment.
is it possible to have a less greedy retry that does not calling 'c.modifySrcDstCheckAttribute' again if there's a conflict?
| } | ||
|
|
||
| func (c *Controller) checkSrcDstAttributeEnabled(key string) (enabled bool, err error) { | ||
| defer c.workqueue.Done(key) |
There was a problem hiding this comment.
Having this method call 'c.workqueue.Done' is a bit hard to follow. Perhaps it can be placed before :
https://github.com/ottoyiu/k8s-ec2-srcdst/pull/16/files#diff-bff40c211a4e0c5aed5c55ab3cdbda41R10
and have the failure cases on 'processNextWorkItem' return early instead.
| if _, ok := node.Annotations[SrcDstCheckDisabledAnnotation]; ok { | ||
| srcDstCheckEnabled = false | ||
| func (c *Controller) disableSrcDstCheck(key string) error { | ||
| defer c.workqueue.Done(key) |
There was a problem hiding this comment.
likewise to my other comment regarding modifying the workqueue in this method.
|
hey @gjtempleton, I will work on merging this soon! |
3 participants
This PR depends on PR #15.
This reimplements src-dst controller to use informers and workqueue. Will fix/add more tests and test on a cluster.