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

[Feature] Replace service name with Fully Qualified Domain Name #938

Merged
merged 10 commits into from
Mar 8, 2023

Conversation

kevin85421
Copy link
Member

@kevin85421 kevin85421 commented Mar 2, 2023

Why are these changes needed?

Some big tech companies do not use kube-dns as the DNS server for Kubernetes Pods. They have a lot of Kubernetes services in the cluster, but services in different namespaces may have the same name. Hence, IP address with the information of both Kubernetes service name and Kubernetes namespace is useful in this case. That is, Fully Qualified Domain Name (FQDN) is a must.

  • Use kube-dns as the DNS server: We can use ${HEAD_SERVICE} to access the head service.
  • With FQDN, we need to use ${HEAD_SERVICE}.${NAMESPACE}.svc.cluster.local to access the head service.

Backward compatibility

Note: It breaks the backward compatibility (users need to update until nslookup $RAY_IP ... in their YAML files). However, this update is a must. It actually blocks some users to adopt KubeRay.

[Update]: We plan to maintain both $FQ_RAY_IP and $RAY_IP environment variables at the same time. Hence, user does not need to update their YAML files.

  • $FQ_RAY_IP: ${HEAD_SERVICE}.${NAMESPACE}.svc.cluster.local
  • $RAY_IP: ${HEAD_SERVICE}

Why do I remove anything related to the head service (svcName & fqdnRayIP) in buildHeadPod?

  • (function call graph) buildHeadPod calls:

    • BuildPod calls:
      • SetInitContainerEnvVars: Set RAY_IP for init container. (case 1)
      • SetContainerEnvVars: Set RAY_IP for the Ray head container. (case 2)
    • DefaultHeadPodTemplate calls:
      • SetMissingRayStartParams: Set rayStartParams["address"] for the Ray head container. (case 3)
  • case 1: init container

    • "init containers run to completion before any app containers start" (ref) => Before Ray head's init containers complete, it is impossible for the Ray cluster to be ready. => Init containers do not need svcName & fqdnRayIP.
  • case 2: Ray head container

    • In case 2, the RAY_IP env variable for the head Pod is hardcoded to "LOCAL_HOST". (The following code is without this PR.) => Do not need the information of svcName & fqdnRayIP.
      var rayIP string
      if rayNodeType == rayiov1alpha1.HeadNode {
      // if head, use localhost
      rayIP = LOCAL_HOST
      } else {
      // if worker, use the service name of the head
      rayIP = svcName
      }
  • case 3: rayStartParams for Ray head

    • If the node is a head node, the operator will not set rayStartParams["address"]. (The following code is without this PR.) => Do not need the information of svcName & fqdnRayIP.
      if nodeType == rayiov1alpha1.WorkerNode {
      if _, ok := rayStartParams["address"]; !ok {
      address := fmt.Sprintf("%s:%s", svcName, headPort)
      rayStartParams["address"] = address
      }
      }

To conclude, we can remove anything related to the head service (svcName & fqdnRayIP) in buildHeadPod.

function ExtractRayIPFromFQDN

// GenerateFQDNServiceName generates a Fully Qualified Domain Name.
func GenerateFQDNServiceName(clusterName string, namespace string) string {
	return fmt.Sprintf("%s.%s.svc.cluster.local", GenerateServiceName(clusterName), namespace)
}

// ExtractRayIPFromFQDN extracts the head service name (i.e., RAY_IP, deprecated) from a fully qualified
// domain name (FQDN). This function is provided for backward compatibility purposes only.
func ExtractRayIPFromFQDN(fqdnRayIP string) string {
	return strings.Split(fqdnRayIP, ".")[0]
}
  • $FQ_RAY_IP: ${HEAD_SERVICE}.${NAMESPACE}.svc.cluster.local
  • $RAY_IP: ${HEAD_SERVICE}

In addition, "." is invalid in Kubernetes service name (the regex for Kubernetes service name is [a-z]([-a-z0-9]*[a-z0-9])). Thus, the implementation is valid.

# svc.yaml 
apiVersion: v1
kind: Service
metadata:
  name: my.service # invalid service name
spec:
  selector:
    app: my-app
  ports:
    - name: http
      port: 80
      targetPort: 80
  type: ClusterIP
$ kubectl apply -f svc.yaml
The Service "my.service" is invalid: metadata.name: Invalid value: "my.service": a DNS-1035 label must consist of lower case alphanumeric characters, and end with an alphanumeric character (e.g. 'my-name',  or 'abc-123', regex used for validation is '[a-z]([-a-z0-9]*[a-z0-9])?')

[Need discussion] FQ_RAY_IP and RAY_IP should not be set by users.

  • In this PR, the values of FQ_RAY_IP and RAY_IP will always be determined by KubeRay operator. Users cannot set them. I cannot come up with any case that users need to specify these two values by themselves.
# This PR removes this kind of check.
if !envVarExists("FQ_RAY_IP", container.Env) {
  ... 
}

Related issue number

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(
# Step 1: Clone my repo, and check out to my branch.
# Step 2: Build the KubeRay operator image.
# Step 3: Install KubeRay operator with the new Docker image.
# Step 4: Install RayCluster with the repo's chart (path: helm-chart/ray-cluster)
helm install raycluster .
  • Check worker Pod.
    • init container
      • FQ_RAY_IP environment variable is raycluster-kuberay-head-svc.default.svc.cluster.local.
      • RAY_IP environment variable is raycluster-kuberay-head-svc.
    • ray-worker
      • FQ_RAY_IP environment variable is raycluster-kuberay-head-svc.default.svc.cluster.local.
      • RAY_IP environment variable is raycluster-kuberay-head-svc.
      • ray start --block --address=raycluster-kuberay-head-svc.default.svc.cluster.local:6379 ...

Test backward compatibility

  1. KubeRay operator in this PR is compatible with old configuration YAML files.

    # Step 1: Clone my repo, and check out to my branch. (image: controller:latest)
    # Step 2: Check out to the master's branch
    # Step 3: Test YAML files which use deprecated RAY_IP.
    RAY_IMAGE=rayproject/ray:2.3.0 OPERATOR_IMAGE=controller:latest python3 tests/test_sample_raycluster_yamls.py 2>&1 | tee log

    Screen Shot 2023-03-05 at 8 54 46 AM

  2. I will replace RAY_IP in the configuration files with FQ_RAY_IP after release 0.5.0 because we need to make sure the configuration YAML files in the master branch are compatible with both nightly and the latest release KubeRay operator. [Feature] Replace RAY_IP with FQ_RAY_IP after release 0.5.0 #941

@kevin85421 kevin85421 marked this pull request as ready for review March 2, 2023 21:40
@kevin85421 kevin85421 added this to the v0.5.0 release milestone Mar 2, 2023
@kevin85421 kevin85421 changed the title (DO NOT MERGE) [Feature] Replace service name with Fully Qualified Domain Name Mar 3, 2023
@kevin85421
Copy link
Member Author

cc @Yicheng-Lu-llll

@kevin85421 kevin85421 mentioned this pull request Mar 3, 2023
5 tasks
@kevin85421 kevin85421 self-assigned this Mar 3, 2023
@DmitriGekhtman
Copy link
Collaborator

You can maintain compatibility by introducing a new env, say $FQ_RAY_IP, and keeping the old env as is (perhaps with a comment in the code that it's needed for backwards compatibility).

@kevin85421
Copy link
Member Author

You can maintain compatibility by introducing a new env, say $FQ_RAY_IP, and keeping the old env as is (perhaps with a comment in the code that it's needed for backwards compatibility).

Good idea. Will do that.

@kevin85421
Copy link
Member Author

kevin85421 commented Mar 6, 2023

Hi @DmitriGekhtman, this PR is currently backward compatible. Would you mind reviewing it? Thanks!

@Yicheng-Lu-llll
Copy link
Contributor

LGTM!

I have also tested it and the worker pod connects to the head pod successfully using the fqdnRayIP.
worker pod's ray starts command now becomes:

ulimit -n 65536; ray start  --num-cpus=1  --memory=1000000000  --block  --address=raycluster-complete-head-svc.default.svc.cluster.local:6379  --metrics-export-port=8080 

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.

3 participants