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

[Subtask][operator] svc generation refinement #525

Closed
3 tasks done
advancedxy opened this issue Jan 30, 2023 · 1 comment · Fixed by #530
Closed
3 tasks done

[Subtask][operator] svc generation refinement #525

advancedxy opened this issue Jan 30, 2023 · 1 comment · Fixed by #530

Comments

@advancedxy
Copy link
Contributor

advancedxy commented Jan 30, 2023

Code of Conduct

Search before asking

  • I have searched in the issues and found no similar issues.

Describe the subtask

We should reconsider the service generated by RSS operator as it may not make too much sense.
Current behavior for RSS to generate svcs:

  1. generate NodePort service for coordinator, which is mandatory
    • the svc is passed to shuffle server as coordinator address
    • it requires RSS configuration must defines RPCNodePort and HTTPNodePort, which is restrictive and cost node ports
  2. NodePort svc is generated for shuffle server if node port is supplied
  3. headless svc is generated for shuffle server.

There are some ways to improves this situation:

  1. since coordinator and shuffle server are both in the k8s cluster, there's no need to use ClusterIP/NodePort for shuffle server to connect with coordinator. Therefore headless svcs should be created for coordinators, and passed to shuffle server
  2. it's pointless to generate svc for shuffle server as it should be accessed directly per node.
  3. for inter-cluster only workload, there's no need for coordinator to expose NodePort. Therefore NodePort configuration in RSS should be optional rather mandatory.
  4. to expose RSS cluster deployed on K8S to external compute resource, such as a yarn cluster, then coordinator and shuffle server should both be deployed with hostnetwork. The service part is preferred with LoadBalance.

Parent issue

#462

Are you willing to submit PR?

  • Yes I am willing to submit a PR!
@advancedxy
Copy link
Contributor Author

cc @wangao1236

advancedxy added a commit to advancedxy/incubator-uniffle that referenced this issue Feb 6, 2023
advancedxy added a commit to advancedxy/incubator-uniffle that referenced this issue Feb 6, 2023
advancedxy added a commit to advancedxy/incubator-uniffle that referenced this issue Feb 6, 2023
advancedxy added a commit to advancedxy/incubator-uniffle that referenced this issue Feb 6, 2023
advancedxy added a commit that referenced this issue Feb 6, 2023
### What changes were proposed in this pull request?
1. generate headless svc for coordinators and pass this service to shuffle servers
2. makes RPCNodePort/HTTPNodePort optional for coordinators
3. remove service creation for shuffle servers.

### Why are the changes needed?
This fixes #525 

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
It has already been manually verified, and will add more UTs.
advancedxy added a commit that referenced this issue Feb 9, 2023
### What changes were proposed in this pull request?
Add omitempty for `RPCNodePort` and `HTTPNodePort`

### Why are the changes needed?
This is a bug fixes. Otherwise, rss spec cannot omit `rpcNodePort` and `httpNodePort`

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
No need
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant