Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expose the index 0 pod for minimal service #203

Merged
merged 1 commit into from
Aug 28, 2023
Merged

expose the index 0 pod for minimal service #203

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions controllers/flux/minicluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alculquicondor please confirm this, it will be good to have a double check from you here

Copy link
Member

@vsoch vsoch Aug 28, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we completely override the selector we no longer match on the cluster.Name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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: true

It 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

payload := []patchPayload{{
but that was more for autoscaler user cases of needing to select just the index 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👀 😆

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic! I'll need to slowly deprecate that then.

selector["job-index"] = "0"
}

result, err = r.exposeServices(ctx, cluster, cluster.Spec.Network.HeadlessName, selector)
Expand Down