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

Ensure container ports without names are also included in the head node service #891

Merged

Conversation

Yicheng-Lu-llll
Copy link
Contributor

@Yicheng-Lu-llll Yicheng-Lu-llll commented Jan 31, 2023

Why are these changes needed?

issue 463 describes that if container port names are not set in the ray cluster, workers are unable to communicate with the head due to the wrong head node Kubernetes service config.
This is because in the current implementation:

	for _, port := range cPorts {
		svcPorts[port.Name] = port.ContainerPort
	}

if port.Name == "", Only the last container port will be considered since all container ports have identical keys (port.Name) .

Considering these:

  1. container port names are optional (link). -> Completely legal to not set or partially set container port names.

  2. multi-port-services must set port names (link link). -> head node Kubernetes service must set service port names.

  3. As long as service port numbers are equal to container port numbers, the head node service's functionalities will work just fine( e. g. workers can communicate with the head). -> There is no extra constraint to the service port name or container port name except the name should be unique. -> This means though it is great to set the service port name equal to the container port name for better understanding, it is not mandatory.

  4. Fact 1,2,3 -> The fix should keep all functionalities working even without setting container port names. It can be done by giving service port names.

  5. Gcs, dashboard, and client's Port number can be changed(link). -> We cannot hardcode the service port names since we have no idea what the associate port number is used for.

  6. Fact 4,5 -> Assigning random but unique service port names if the user does not set the container port names is a good idea.

With the above consideration, This PR adds the following to fix the issue 463:

	for _, port := range cPorts {
		if port.Name == "" {
			port.Name = fmt.Sprint(port.ContainerPort) + "-port"
		}
		svcPorts[port.Name] = port.ContainerPort
	}

Related issue number

Closes #463

Checks

Step1:
Clone my repo and checkout to PortsWithoutNameAreIgnored branch

Step2:
Follow DEVELOPMENT.md to build Docker image and install kuberay operator.

Step3:
remove container port names in kuberay/ray-operator/config/samples/ray-cluster.complete.yaml:

           ports:
         - containerPort: 6379
          
         - containerPort: 8265
         
         - containerPort: 10001
        

Step4:

#path: kuberay/
kubectl apply -f ray-operator/config/samples/ray-cluster.complete.yaml

Step5:

kubectl describe svc raycluster-complete-head-svc
#should see:
#Port:              8265-port  8265/TCP
#TargetPort:        8265/TCP
#Endpoints:         <none>
#Port:              10001-port  10001/TCP
#TargetPort:        10001/TCP
#Endpoints:         <none>
#Port:              6379-port  6379/TCP
#TargetPort:        6379/TCP
#Endpoints:         <none>

@Yicheng-Lu-llll Yicheng-Lu-llll changed the title Ports without name are ignored Ensure container ports without names are also included in the head node service Jan 31, 2023
@Yicheng-Lu-llll Yicheng-Lu-llll marked this pull request as ready for review February 1, 2023 18:53
Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

Thanks @Yicheng-Lu-llll for this contribution! Do you mind add some tests?

@kevin85421
Copy link
Member

Can you rebase with master? #893 can fix the CI failure in "Go-build-and-test / Build Operator Binaries and Docker Images". Thanks!

@Yicheng-Lu-llll
Copy link
Contributor Author

Yicheng-Lu-llll commented Feb 9, 2023

Updated. Sorry for the above commit issue.

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kevin85421 kevin85421 left a comment

Choose a reason for hiding this comment

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

I also tested this PR manually.

Screen Shot 2023-02-14 at 11 56 02 AM

@kevin85421 kevin85421 merged commit 6490749 into ray-project:master Feb 14, 2023
lowang-bh pushed a commit to lowang-bh/kuberay that referenced this pull request Sep 24, 2023
…de service (ray-project#891)

Container ports without names will not be ignored.
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 this pull request may close these issues.

[Bug] [Operator] [Controller] [Service] | Ports without name are ignored
3 participants