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

ray-operator coding style change #2096

Merged
merged 2 commits into from
Apr 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions apiserver/pkg/model/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func PopulateHeadNodeSpec(spec rayv1api.HeadGroupSpec) *api.HeadGroupSpec {

// Here we update environment only for a container named 'ray-head'
if container, _, ok := util.GetContainerByName(spec.Template.Spec.Containers, "ray-head"); ok && len(container.Env) > 0 {
headNodeSpec.Environment = convert_env_variables(container.Env, true)
headNodeSpec.Environment = convertEnvVariables(container.Env, true)
}

if len(spec.Template.Spec.ServiceAccountName) > 1 {
Expand Down Expand Up @@ -289,7 +289,7 @@ func PopulateWorkerNodeSpec(specs []rayv1api.WorkerGroupSpec) []*api.WorkerGroup

// Here we update environment only for a container named 'ray-worker'
if container, _, ok := util.GetContainerByName(spec.Template.Spec.Containers, "ray-worker"); ok && len(container.Env) > 0 {
workerNodeSpec.Environment = convert_env_variables(container.Env, false)
workerNodeSpec.Environment = convertEnvVariables(container.Env, false)
}

if len(spec.Template.Spec.ServiceAccountName) > 1 {
Expand All @@ -306,7 +306,7 @@ func PopulateWorkerNodeSpec(specs []rayv1api.WorkerGroupSpec) []*api.WorkerGroup
return workerNodeSpecs
}

func convert_env_variables(cenv []corev1.EnvVar, header bool) *api.EnvironmentVariables {
func convertEnvVariables(cenv []corev1.EnvVar, header bool) *api.EnvironmentVariables {
env := api.EnvironmentVariables{
Values: make(map[string]string),
ValuesFrom: make(map[string]*api.EnvValueFrom),
Expand Down
12 changes: 6 additions & 6 deletions apiserver/test/e2e/config_server_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,14 +207,14 @@ func TestGetAllComputeTemplates(t *testing.T) {
require.Nil(t, actualRpcStatus, "No RPC status expected")
require.NotNil(t, response, "A response is expected")
require.NotEmpty(t, response.ComputeTemplates, "A list of compute templates is required")
found_name := false
foundName := false
for _, template := range response.ComputeTemplates {
if tCtx.GetComputeTemplateName() == template.Name && tCtx.GetNamespaceName() == template.Namespace {
found_name = true
foundName = true
break
}
}
require.Equal(t, found_name, true)
require.Equal(t, foundName, true)
}

// TestGetTemplatesInNamespace get all compute templates in namespace endpoint
Expand All @@ -236,14 +236,14 @@ func TestGetTemplatesInNamespace(t *testing.T) {
require.Nil(t, actualRpcStatus, "No RPC status expected")
require.NotNil(t, response, "A response is expected")
require.NotEmpty(t, response.ComputeTemplates, "A list of compute templates is required")
found_name := false
foundName := false
for _, template := range response.ComputeTemplates {
if tCtx.GetComputeTemplateName() == template.Name && tCtx.GetNamespaceName() == template.Namespace {
found_name = true
foundName = true
break
}
}
require.Equal(t, found_name, true)
require.Equal(t, foundName, true)
}

// TestDeleteTemplate sequentially iterates over the delete compute template endpoint
Expand Down
12 changes: 6 additions & 6 deletions apiserver/test/e2e/job_server_e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,14 +303,14 @@ func TestGetAllJobs(t *testing.T) {
require.Nil(t, actualRpcStatus, "No RPC status expected")
require.NotNil(t, response, "A response is expected")
require.NotEmpty(t, response.Jobs, "A list of jobs is required")
found_name := false
foundName := false
for _, job := range response.Jobs {
if testJobRequest.Job.Name == job.Name && tCtx.GetNamespaceName() == job.Namespace {
found_name = true
foundName = true
break
}
}
require.Equal(t, found_name, true)
require.Equal(t, foundName, true)
}

func TestGetJobsInNamespace(t *testing.T) {
Expand All @@ -333,14 +333,14 @@ func TestGetJobsInNamespace(t *testing.T) {
require.Nil(t, actualRpcStatus, "No RPC status expected")
require.NotNil(t, response, "A response is expected")
require.NotEmpty(t, response.Jobs, "A list of compute templates is required")
found_name := false
foundName := false
for _, job := range response.Jobs {
if testJobRequest.Job.Name == job.Name && tCtx.GetNamespaceName() == job.Namespace {
found_name = true
foundName = true
break
}
}
require.Equal(t, found_name, true)
require.Equal(t, foundName, true)
}

func TestGetJob(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions ray-operator/controllers/ray/common/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@ func BuildIngressForHeadService(ctx context.Context, cluster rayv1.RayCluster) (
}

// Copy other ingress configurations from cluster annotations to provide a generic way
// for user to customize their ingress settings. The `exclude_set` is used to avoid setting
// for user to customize their ingress settings. The `excludeSet` is used to avoid setting
// both IngressClassAnnotationKey annotation which is deprecated and `Spec.IngressClassName`
// at the same time.
exclude_set := map[string]struct{}{
excludeSet := map[string]struct{}{
IngressClassAnnotationKey: {},
}
annotation := map[string]string{}
for key, value := range cluster.Annotations {
if _, ok := exclude_set[key]; !ok {
if _, ok := excludeSet[key]; !ok {
annotation[key] = value
}
}
Expand Down
92 changes: 46 additions & 46 deletions ray-operator/controllers/ray/common/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,40 +33,40 @@ func BuildServiceForHeadPod(ctx context.Context, cluster rayv1.RayCluster, label
labels = make(map[string]string)
}

default_labels := HeadServiceLabels(cluster)
defaultLabels := HeadServiceLabels(cluster)

// selector consists of *only* the keys in default_labels, updated with the values in labels if they exist
// selector consists of *only* the keys in defaultLabels, updated with the values in labels if they exist
selector := make(map[string]string)
for k := range default_labels {
for k := range defaultLabels {
if _, ok := labels[k]; ok {
selector[k] = labels[k]
} else {
selector[k] = default_labels[k]
selector[k] = defaultLabels[k]
}
}

// Deep copy the selector to avoid modifying the original object
labels_for_service := make(map[string]string)
labelsForService := make(map[string]string)
for k, v := range selector {
labels_for_service[k] = v
labelsForService[k] = v
}

if annotations == nil {
annotations = make(map[string]string)
}

default_name, err := utils.GenerateHeadServiceName(utils.RayClusterCRD, cluster.Spec, cluster.Name)
defaultName, err := utils.GenerateHeadServiceName(utils.RayClusterCRD, cluster.Spec, cluster.Name)
if err != nil {
return nil, err
}
default_namespace := cluster.Namespace
default_type := cluster.Spec.HeadGroupSpec.ServiceType
defaultNamespace := cluster.Namespace
defaultType := cluster.Spec.HeadGroupSpec.ServiceType

defaultAppProtocol := utils.DefaultServiceAppProtocol
// `ports_int` is a map of port names to port numbers, while `ports` is a list of ServicePort objects
ports_int := getServicePorts(cluster)
// `portsInt` is a map of port names to port numbers, while `ports` is a list of ServicePort objects
portsInt := getServicePorts(cluster)
ports := []corev1.ServicePort{}
for name, port := range ports_int {
for name, port := range portsInt {
svcPort := corev1.ServicePort{Name: name, Port: port, AppProtocol: &defaultAppProtocol}
ports = append(ports, svcPort)
}
Expand Down Expand Up @@ -97,25 +97,25 @@ func BuildServiceForHeadPod(ctx context.Context, cluster rayv1.RayCluster, label
// Append default ports.
headService.Spec.Ports = append(headService.Spec.Ports, ports...)

setLabelsforUserProvidedService(headService, labels_for_service)
setNameforUserProvidedService(ctx, headService, default_name)
setNamespaceforUserProvidedService(ctx, headService, default_namespace)
setServiceTypeForUserProvidedService(ctx, headService, default_type)
setLabelsforUserProvidedService(headService, labelsForService)
setNameforUserProvidedService(ctx, headService, defaultName)
setNamespaceforUserProvidedService(ctx, headService, defaultNamespace)
setServiceTypeForUserProvidedService(ctx, headService, defaultType)

return headService, nil
}

headService := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: default_name,
Namespace: default_namespace,
Labels: labels_for_service,
Name: defaultName,
Namespace: defaultNamespace,
Labels: labelsForService,
Annotations: annotations,
},
Spec: corev1.ServiceSpec{
Selector: selector,
Ports: ports,
Type: default_type,
Type: defaultType,
},
}

Expand Down Expand Up @@ -191,17 +191,17 @@ func BuildServeService(ctx context.Context, rayService rayv1.RayService, rayClus
selectorLabels[utils.RayClusterServingServiceLabelKey] = utils.EnableRayClusterServingServiceTrue
}

default_name := utils.GenerateServeServiceName(name)
default_namespace := namespace
default_type := rayCluster.Spec.HeadGroupSpec.ServiceType
defaultName := utils.GenerateServeServiceName(name)
defaultNamespace := namespace
defaultType := rayCluster.Spec.HeadGroupSpec.ServiceType
if isRayService {
default_type = rayService.Spec.RayClusterSpec.HeadGroupSpec.ServiceType
defaultType = rayService.Spec.RayClusterSpec.HeadGroupSpec.ServiceType
}

// `ports_int` is a map of port names to port numbers, while `ports` is a list of ServicePort objects
ports_int := getServicePorts(rayCluster)
// `portsInt` is a map of port names to port numbers, while `ports` is a list of ServicePort objects
portsInt := getServicePorts(rayCluster)
ports := []corev1.ServicePort{}
for name, port := range ports_int {
for name, port := range portsInt {
if name == utils.ServingPortName {
svcPort := corev1.ServicePort{Name: name, Port: port}
ports = append(ports, svcPort)
Expand Down Expand Up @@ -246,9 +246,9 @@ func BuildServeService(ctx context.Context, rayService rayv1.RayService, rayClus
}

setLabelsforUserProvidedService(serveService, labels)
setNameforUserProvidedService(ctx, serveService, default_name)
setNamespaceforUserProvidedService(ctx, serveService, default_namespace)
setServiceTypeForUserProvidedService(ctx, serveService, default_type)
setNameforUserProvidedService(ctx, serveService, defaultName)
setNamespaceforUserProvidedService(ctx, serveService, defaultNamespace)
setServiceTypeForUserProvidedService(ctx, serveService, defaultType)

return serveService, nil
}
Expand All @@ -262,14 +262,14 @@ func BuildServeService(ctx context.Context, rayService rayv1.RayService, rayClus

serveService := &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: default_name,
Namespace: default_namespace,
Name: defaultName,
Namespace: defaultNamespace,
Labels: labels,
},
Spec: corev1.ServiceSpec{
Selector: selectorLabels,
Ports: ports,
Type: default_type,
Type: defaultType,
},
}

Expand Down Expand Up @@ -305,45 +305,45 @@ func BuildHeadlessServiceForRayCluster(rayCluster rayv1.RayCluster) (*corev1.Ser
return headlessService, nil
}

func setServiceTypeForUserProvidedService(ctx context.Context, service *corev1.Service, default_type corev1.ServiceType) {
func setServiceTypeForUserProvidedService(ctx context.Context, service *corev1.Service, defaultType corev1.ServiceType) {
log := ctrl.LoggerFrom(ctx)
// If the user has not specified a service type, use the default service type
if service.Spec.Type == "" {
log.Info("Using default serviceType passed for the user provided service",
"default_type passed", default_type,
"default_type passed", defaultType,
"service.ObjectMeta.Name", service.ObjectMeta.Name)
service.Spec.Type = default_type
service.Spec.Type = defaultType
} else {
log.Info("Overriding default serviceType with user provided serviceType",
"default_type passed", default_type,
"default_type passed", defaultType,
"service.ObjectMeta.Name", service.ObjectMeta.Name,
"default_type passed", default_type,
"default_type passed", defaultType,
"service.Spec.Type", service.Spec.Type)
}
}

func setNamespaceforUserProvidedService(ctx context.Context, service *corev1.Service, default_namespace string) {
func setNamespaceforUserProvidedService(ctx context.Context, service *corev1.Service, defaultNamespace string) {
log := ctrl.LoggerFrom(ctx)
// If the user has specified a namespace, ignore it and raise a warning
if service.ObjectMeta.Namespace != "" && service.ObjectMeta.Namespace != default_namespace {
if service.ObjectMeta.Namespace != "" && service.ObjectMeta.Namespace != defaultNamespace {
log.Info("Ignoring namespace in user provided service",
"provided_namespace", service.ObjectMeta.Namespace,
"service_name", service.ObjectMeta.Name,
"default_namespace", default_namespace)
"default_namespace", defaultNamespace)
}

service.ObjectMeta.Namespace = default_namespace
service.ObjectMeta.Namespace = defaultNamespace
}

func setNameforUserProvidedService(ctx context.Context, service *corev1.Service, default_name string) {
func setNameforUserProvidedService(ctx context.Context, service *corev1.Service, defaultName string) {
log := ctrl.LoggerFrom(ctx)
// If the user has not specified a name, use the default name passed
if service.ObjectMeta.Name == "" {
log.Info("Using default name for user provided service.", "default_name", default_name)
service.ObjectMeta.Name = default_name
log.Info("Using default name for user provided service.", "default_name", defaultName)
service.ObjectMeta.Name = defaultName
} else {
log.Info("Overriding default name for user provided service with name in service.ObjectMeta.Name.",
"default_name", default_name,
"default_name", defaultName,
"provided_name", service.ObjectMeta.Name)
}
}
Expand Down
30 changes: 15 additions & 15 deletions ray-operator/controllers/ray/common/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,45 +285,45 @@ func TestUserSpecifiedHeadService(t *testing.T) {
}
// These labels originate from HeadGroupSpec.Template.ObjectMeta.Labels
userTemplateClusterName := "userTemplateClusterName"
template_labels := map[string]string{utils.RayClusterLabelKey: userTemplateClusterName}
headService, err := BuildServiceForHeadPod(context.Background(), *testRayClusterWithHeadService, template_labels, testRayClusterWithHeadService.Spec.HeadServiceAnnotations)
templateLabels := map[string]string{utils.RayClusterLabelKey: userTemplateClusterName}
headService, err := BuildServiceForHeadPod(context.Background(), *testRayClusterWithHeadService, templateLabels, testRayClusterWithHeadService.Spec.HeadServiceAnnotations)
if err != nil {
t.Errorf("failed to build head service: %v", err)
}

// The selector field should only use the keys from the five default labels. The values should be updated with the values from the template labels.
// The user-provided HeadService labels should be ignored for the purposes of the selector field. The user-provided Selector field should be ignored.
default_labels := HeadServiceLabels(*testRayClusterWithHeadService)
defaultLabels := HeadServiceLabels(*testRayClusterWithHeadService)
// Make sure this test isn't spuriously passing. Check that RayClusterLabelKey is in the default labels.
if _, ok := default_labels[utils.RayClusterLabelKey]; !ok {
if _, ok := defaultLabels[utils.RayClusterLabelKey]; !ok {
t.Errorf("utils.RayClusterLabelKey=%s should be in the default labels", utils.RayClusterLabelKey)
}
for k, v := range headService.Spec.Selector {
// If k is not in the default labels, then the selector field should not contain it.
if _, ok := default_labels[k]; !ok {
if _, ok := defaultLabels[k]; !ok {
t.Errorf("Selector field should not contain key=%s", k)
}
// If k is in the template labels, then the selector field should contain it with the value from the template labels.
// Otherwise, it should contain the value from the default labels.
if _, ok := template_labels[k]; ok {
if v != template_labels[k] {
t.Errorf("Selector field should contain key=%s with value=%s, actual value=%s", k, template_labels[k], v)
if _, ok := templateLabels[k]; ok {
if v != templateLabels[k] {
t.Errorf("Selector field should contain key=%s with value=%s, actual value=%s", k, templateLabels[k], v)
}
} else {
if v != default_labels[k] {
t.Errorf("Selector field should contain key=%s with value=%s, actual value=%s", k, default_labels[k], v)
if v != defaultLabels[k] {
t.Errorf("Selector field should contain key=%s with value=%s, actual value=%s", k, defaultLabels[k], v)
}
}
}
// The selector field should have every key from the default labels.
for k := range default_labels {
for k := range defaultLabels {
if _, ok := headService.Spec.Selector[k]; !ok {
t.Errorf("Selector field should contain key=%s", k)
}
}

// Print default labels for debugging
for k, v := range default_labels {
for k, v := range defaultLabels {
fmt.Printf("default label: key=%s, value=%s\n", k, v)
}

Expand Down Expand Up @@ -593,7 +593,7 @@ func validateServiceTypeForUserSpecifiedService(svc *corev1.Service, userType co
}
}

func validateNameAndNamespaceForUserSpecifiedService(svc *corev1.Service, default_namespace string, userName string, t *testing.T) {
func validateNameAndNamespaceForUserSpecifiedService(svc *corev1.Service, defaultNamespace string, userName string, t *testing.T) {
// Test name and namespace are generated if not specified
if svc.ObjectMeta.Name == "" {
t.Errorf("Generated service name is empty")
Expand All @@ -602,8 +602,8 @@ func validateNameAndNamespaceForUserSpecifiedService(svc *corev1.Service, defaul
t.Errorf("Generated service namespace is empty")
}
// The user-provided namespace should be ignored, but the name should be respected
if svc.ObjectMeta.Namespace != default_namespace {
t.Errorf("User-provided namespace should be ignored: expected namespace=%s, actual namespace=%s", default_namespace, svc.ObjectMeta.Namespace)
if svc.ObjectMeta.Namespace != defaultNamespace {
t.Errorf("User-provided namespace should be ignored: expected namespace=%s, actual namespace=%s", defaultNamespace, svc.ObjectMeta.Namespace)
}
if svc.ObjectMeta.Name != userName {
t.Errorf("User-provided name should be respected: expected name=%s, actual name=%s", userName, svc.ObjectMeta.Name)
Expand Down
Loading