Skip to content

Commit

Permalink
ray-operator coding style change (#2096)
Browse files Browse the repository at this point in the history
  • Loading branch information
@LeoLiao123
LeoLiao123 authored Apr 21, 2024
1 parent ff008a1 commit 7af8988
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 79 deletions.
6 changes: 3 additions & 3 deletions apiserver/pkg/model/converter.go
View file
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
View file
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
View file
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
View file
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
View file
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
View file
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

0 comments on commit 7af8988

Please sign in to comment.