expose the index 0 pod for minimal service #203
Conversation
LabelSelectors multiple requirements are ANDed, however, the code was completely overriden the selector for the specific minicluster name, hence if there are multiple miniculster running in parallel the Service will expose all the indexed 0 pods from all the cluster. Signed-off-by: Antonio Ojea <aojea@google.com>
|
/assign @vsoch @alculquicondor |
| @@ -88,9 +88,10 @@ func (r *MiniClusterReconciler) ensureMiniCluster( | |||
| // Create headless service for the MiniCluster OR single service for the broker | |||
| selector := map[string]string{"job-name": cluster.Name} | |||
|
|
|||
| // If we are adding a minimal service to the index 0 pod only | |||
| // If we are adding a minimal service expose the index 0 pod only | |||
| // LabelSelectors are ANDed | |||
There was a problem hiding this comment.
@alculquicondor please confirm this, it will be good to have a double check from you here
There was a problem hiding this comment.
The docs can also confirm for us: https://github.com/kubernetes-client/python/blob/master/kubernetes/docs/V1LabelSelector.md
The requirements are ANDed.
| if cluster.Spec.Flux.MinimalService { | ||
| selector = map[string]string{"job-index": "0"} |
There was a problem hiding this comment.
if we completely override the selector we no longer match on the cluster.Name
There was a problem hiding this comment.
@aojea this minimal service was intended to only provide a service exposed for the index 0 pod (hence why we created new labels). This also only is relevant when you have:
flux:
minimalServlce: trueIt was a test to see if we could reduce the load of the service (it was @alculquicondor idea and a pretty good one) although I don't think it practice it worked beyond dummy examples. Could you explain what you are trying to do? Note that we add labels based on the index here
flux-operator/controllers/flux/labels.go
Line 57 in ce1f440
There was a problem hiding this comment.
The problem here is that you override the selector and only use "job-index": "0" and the selector. That means that will consider any pod that has that label ... independently of the job name , since you no longer match on job-name
If there are N jobs, you'll have N pods with "job-index": "0" and the Service selector will match on all of them, that is not what you want
See an example in https://gist.github.com/aojea/ce7b08fe9e9d57125ccc6a9728a414b6
There was a problem hiding this comment.
Yes agree that's an issue - here is what I'd recommend from slack:
selector = map[string]string{"job-index": fmt.Sprintf("%s-0", cluster.Name)}Basically the same thing but namespaced to the job name.
There was a problem hiding this comment.
Can services have double selectors? We could also do job-index 0 and the name of the job. Whatever solution we decide is best here, we might want to suggest this for Indexed Jobs upstream. It would be nice to have this ability to granular-ly select.
There was a problem hiding this comment.
And the oversight is on my part - most of our cases for testing / experiments have been creating one MiniCluster at a time. Of course this does not hold for many use cases, and thank you for the extra eyes to make me realize this! 👀 😆
There was a problem hiding this comment.
+1 on separate selectors.
Remember that in k8s 1.28 we actually have the job indexes as labels, so no need to put your own index.
There was a problem hiding this comment.
Fantastic! I'll need to slowly deprecate that then.
|
Something is up with the Python tests - could be spurious. These are related to the service port forward. I’ll make some time to test today. |
There was a problem hiding this comment.
As I suspected - the tests for Python are flaky! Likely I can improve upon this if I switch over to kind - there are a lot of variables at play here (MiniKube, the Python SDK, and the operator cluster and service coming online) so we see blurps in testing like this. I was taken aback that it happened in two PRs at once!
When this is merged, it will build and deploy to main branch manifests / images.
LabelSelectors multiple requirements are ANDed, however, the code was completely overriden the selector for the specific minicluster name, hence if there are multiple miniculster running in parallel the Service will expose all the indexed 0 pods from all the cluster.