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.

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

Headless Service Per Replica KEP #188

Merged
merged 8 commits into from
Aug 15, 2024

Conversation

Edwinhr716
Copy link
Contributor

What type of PR is this?

/kind feature

What this PR does / why we need it

KEP for #173 .

Based on original KEP #177. Added suggestions, and modified the way LWS_LEADER_ADDRESS is set.

Which issue(s) this PR fixes

Fixes #

Special notes for your reviewer

Does this PR introduce a user-facing change?


@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 6, 2024
@k8s-ci-robot k8s-ci-robot requested review from ahg-g and liurupeng August 6, 2024 17:33
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 6, 2024
@Edwinhr716 Edwinhr716 changed the title added kep Headless Service Per Replica KEP Aug 6, 2024

### Implementation

With HeadlessServicePerReplica true, we will create a headless service per replica.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we create a headless service per statefulset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the leader will have one, while the workers will have a different one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, ideally we only need the headless service to generate the dns record

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are the benefits of creating a new one per statefulset instead? Creating a new one per sts creates more headless services

Copy link
Contributor

Choose a reason for hiding this comment

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

The headless service name will be part of the fully qualified host name of the pod, so how many and what name we give the service matters. It is also why I am suggesting that the API to be a knob that decides on the subdomain policy because this is the user facing side effect.

The pod hostname is <pod-name>.<svc-name>

So if the new "policy" is UniquePerReplica, then each replica including the leader should be part of the same headless service so that the middle part above is common among the pods of a replica. Example:

Replica 0:

  • my-lws-0.my-lws-0
  • my-lws-0-1.my-lws-0

Replica 1:

  • my-lws-1.my-lws-1
  • my-lws-1-1.my-lws-1

If we create a headless service per sts including a special one for leaders, then we can't achieve the above since the leader will have a different subdomain name. Here is what it will look like:

Replica 0:

  • my-lws-0.my-lws
  • my-lws-0-1.my-lws-0

Replica 1:

  • my-lws-1.my-lws
  • my-lws-1-1.my-lws-1

I am fine with the above, but what would we call such a domain name "policy"? it wouldn't be UniquePerReplica.

Copy link
Contributor

Choose a reason for hiding this comment

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

UniquePerReplicaWorkers maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me, but it does rely on the assumption that if the subdomain is UniquePerReplicaWorkers, that means that the leaders must have their own subdomain.

Copy link
Contributor

@kerthcet kerthcet Aug 14, 2024

Choose a reason for hiding this comment

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

What about leaderSharedWorkerDedicated vs shared or leaderWorkerShared

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of the three I prefer leaderSharedWorkerDedicated

Copy link
Contributor

Choose a reason for hiding this comment

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

LeadersSharedWorkersDedicated

Comment on lines 160 to 175
```golang
for i := 0; i < lws.Spec.Replicas; i++ {
headlessService := corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("%s-%s", lws.Name, i),
Namespace: lws.Namespace,
},
Spec: corev1.ServiceSpec{
ClusterIP: "None", // defines service as headless
Selector: map[string]string{
leaderworkerset.GroupIndexLabelKey: i,
},
PublishNotReadyAddresses: true,
},
}
}
Copy link
Collaborator

@liurupeng liurupeng Aug 7, 2024

Choose a reason for hiding this comment

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

how to select the generated pods to these headless service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses the GroupIndexLabelKey as selector, so that way all pods that are part of the same replica are selected to the same headless service

Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to add the details how pods will be selected by which headless service as well


### Implementation

With HeadlessServicePerReplica true, we will create a headless service per replica.
Copy link
Contributor

Choose a reason for hiding this comment

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

The headless service name will be part of the fully qualified host name of the pod, so how many and what name we give the service matters. It is also why I am suggesting that the API to be a knob that decides on the subdomain policy because this is the user facing side effect.

The pod hostname is <pod-name>.<svc-name>

So if the new "policy" is UniquePerReplica, then each replica including the leader should be part of the same headless service so that the middle part above is common among the pods of a replica. Example:

Replica 0:

  • my-lws-0.my-lws-0
  • my-lws-0-1.my-lws-0

Replica 1:

  • my-lws-1.my-lws-1
  • my-lws-1-1.my-lws-1

If we create a headless service per sts including a special one for leaders, then we can't achieve the above since the leader will have a different subdomain name. Here is what it will look like:

Replica 0:

  • my-lws-0.my-lws
  • my-lws-0-1.my-lws-0

Replica 1:

  • my-lws-1.my-lws
  • my-lws-1-1.my-lws-1

I am fine with the above, but what would we call such a domain name "policy"? it wouldn't be UniquePerReplica.

…ca number of services for the workers
@Edwinhr716
Copy link
Contributor Author

Changed implementation to creating a headless service per sts. Still have to come up with a name for the new subdomainPolicy.
@ahg-g @liurupeng PTAL

service for all of the leaders, and a headless service per replica for the workers. In order
to ensure backwards compatability, the default value if non is set will be Shared.

A label will be added to determine whether a pod is a leader or a worker.
Copy link
Contributor

Choose a reason for hiding this comment

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

We have too many labels, can't we determine by the workerIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can use workerIndex = 0 to select the leaders, but cannot do workerIndex != 0 to select the workers since AFAIK selector does not have that capability.

// SubdomainUniquePerReplica will create a headless service for each
// leader-worker group.
// The leader host names will look like:
// Replica 0: my-lws-0.my-lws
Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, if uniquePerReplica, which means the leader pod and the worker Pods should have the same svc name, but here they're different. my-lws-0.my-lws vs my-lws-0-1.my-lws-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 implementation is for headless service per sts, still figuring out a naming scheme so left the old name to discuss it further


### Implementation

With HeadlessServicePerReplica true, we will create a headless service per replica.
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer the uniquePerReplica option. It makes sense for the leader and worker to have the subdomain in common.

+1, or it's weird I think, unless we have other use cases like visiting the workers only.


### Implementation

With HeadlessServicePerReplica true, we will create a headless service per replica.
Copy link
Contributor

Choose a reason for hiding this comment

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

LeadersSharedWorkersDedicated

@ahg-g
Copy link
Contributor

ahg-g commented Aug 15, 2024

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 15, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahg-g, Edwinhr716

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 15, 2024
@k8s-ci-robot k8s-ci-robot merged commit 1e870db into kubernetes-sigs:main Aug 15, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants