Use label selector when listing pods in es controller #187
Conversation
munnerz
force-pushed
the
es-pilot-filter
branch
from
d2c583f
to
db4cc6e
Compare
|
/test e2e |
There was a problem hiding this comment.
Thanks @munnerz.
- Looks good and much simpler than checking the ownership of each pod.
- (assuming you agree that the ownership check is no longer necessary)
- And should also make it simpler to add unit tests for the Pilot sync method....no need to create a corresponding StatefulSet with ownerrefs, for the pods in tests.
- If an ownership check is still necessary, I was looking into whether the pods can be given multiple owner refs....owned by both the statefulset (controller=true) and the ElasticSearch cluster (controller=false)....but I couldn't find any way to add ownerrefs to the statefulset pod template.
- Also wondered if it's possible to supply a field selector to
List....in which case you could use that for the owner check.
Please merge when the tests pass.
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, pod := range allPods { | ||
| isMember, err := util.PodControlledByCluster(c, pod, e.statefulSetLister) |
There was a problem hiding this comment.
isMember is now unused and causing compile errors.
There was a problem hiding this comment.
Ack - accidentally removed too much code!
| for _, pod := range allPods { | ||
| isMember, err := util.PodControlledByCluster(c, pod, e.statefulSetLister) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("error checking if pod is controller by elasticsearch cluster: %s", err.Error()) | ||
| } | ||
|
|
||
| if isMember { |
There was a problem hiding this comment.
Isn't it important to also check that the pod is owned by the esCluster resource?
I thought this check was to ensure against deleting pods that, somehow, had been created independently, with the matching cluster label.
And if not, we've got code that looks up the owner of the owning statefulset; can that now be removed.
There was a problem hiding this comment.
Yep - this shouldn't have been removed. I've added that check back in.
munnerz
force-pushed
the
es-pilot-filter
branch
from
db4cc6e
to
f1a792a
Compare
|
/test all |
|
Thanks for taking a look @wallrj. I've updated to reintroduce the ownership check as it is still necessary. Regarding two ownerReferences - it would be nice, but also quite complex to do via a StatefulSet pod template (and perhaps not possible?). For now I think it's easiest to keep the behaviour the same. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: munnerz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
/test verify |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
What this PR does / why we need it:
Use a label selector in the Elasticsearch navigator-controller implementations when listing pods to create Pilots for.
This also simplifies the Pilot creation code and decouples it from node pools.
Release note: