Add watcher for agent pods to endpoints controller #457
Conversation
thisisnotashwin
force-pushed
the
reconcile-on-running-agent-pods
branch
from
65d5dca to
ffc5ea1
Compare
There was a problem hiding this comment.
Looks great!! I've left some minor comments, but nothing blocking.
Re acceptance test, I think we'd need to write a new one for this case (currently we're not testing it). I think it's ok for it to come after the PR is merged, but it's up to you. I think we just need to make sure we have coverage before we merge the feature branch.
thisisnotashwin
force-pushed
the
webhook-refactor
branch
2 times, most recently
from
6f5f8e7 to
2d1c75b
Compare
thisisnotashwin
force-pushed
the
reconcile-on-running-agent-pods
branch
from
ffc5ea1 to
22a35ba
Compare
thisisnotashwin
force-pushed
the
webhook-refactor
branch
2 times, most recently
from
9b36f01 to
c2c32fb
Compare
- This watcher watches for Consul Agent pods to be in a running phase and the condition ready to be true and then reconcile all endpoints that have a ready/not-ready address that share a node name with that of the consul agent pod.
thisisnotashwin
force-pushed
the
reconcile-on-running-agent-pods
branch
from
22a35ba to
b75098e
Compare
| // which in this case are Pods. It only returns true if the Pod is a Consul Client Agent Pod. It reads the labels | ||
| // from the meta of the resource and uses the values of the "app" and "component" label to validate that | ||
| // the Pod is a Consul Client Agent. | ||
| func (r EndpointsController) filterAgentPods(meta metav1.Object, object runtime.Object) bool { |
There was a problem hiding this comment.
@thisisnotashwin These should be function pointers I think: r *EndpointsController
There was a problem hiding this comment.
sorry for the late response. They don't seem to use any values from the r struct. this might be something I dont quite understand here but why would it need to be a function pointer?
There was a problem hiding this comment.
Just because the other functions in this struct do use the struct pointer so these should be consistent. Or if they're not accessing anything on the struct they could be functions instead.
There was a problem hiding this comment.
that is a good point. i looked at the other method and i think it does make sense to use pointers to keep them consistent. plus i could add some logging here just for some added information in which case i can use the logger from the struct! good catch!
| Watches( | ||
| &source.Kind{Type: &corev1.Pod{}}, | ||
| &handler.EnqueueRequestsFromMapFunc{ToRequests: handler.ToRequestsFunc(r.requestsForRunningAgentPods)}, | ||
| builder.WithPredicates(predicate.NewPredicateFuncs(r.filterAgentPods)), | ||
| ).Complete(r) |
Remove the cleanup controller and replace it with the supporting logic from #457.
Remove the cleanup controller and replace it with the supporting logic from #457.
Changes proposed in this PR:
How I've tested this PR:
How I expect reviewers to test this PR:
Will link an acceptance test run to show that behavior hasnt been impacted.
Checklist: