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.

Sign up for GitHub

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

remove unused scripts #1521

Merged
merged 1 commit into from
Jan 15, 2022
Merged

remove unused scripts #1521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 0 additions & 2 deletions pkg/apis/mpi/v1/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ package v1
import common "github.com/kubeflow/common/pkg/apis/common/v1"

const (
// EnvKubeflowNamespace is ENV for kubeflow namespace specified by user.
EnvKubeflowNamespace = "KUBEFLOW_NAMESPACE"
Copy link
Member

Choose a reason for hiding this comment

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

I remember this was the default. So this isn't used anywhere anymore?

Copy link
Member Author

@zw0610 zw0610 Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Seems not. Let's see scripts below:

lucas@Wangs-MacBook-Pro-at-Tencent > /tmp/training-operator-master > grep -rin "KUBEFLOW_NAMESPACE" .
./docs/development/developer_guide.md:47:### Configure KUBECONFIG and KUBEFLOW_NAMESPACE
./docs/development/developer_guide.md:54:export KUBEFLOW_NAMESPACE=$(your_namespace)
./docs/development/developer_guide.md:57:- KUBEFLOW_NAMESPACE is used when deployed on Kubernetes, we use this variable to create other resources (e.g. the resource lock) internal in the same namespace. It is optional, use `default` namespace if not set.
./pkg/apis/mpi/v1/constants.go:21:	EnvKubeflowNamespace = "KUBEFLOW_NAMESPACE"
./pkg/common/constants.go:19:	EnvKubeflowNamespace = "KUBEFLOW_NAMESPACE"

 lucas@Wangs-MacBook-Pro-at-Tencent > /tmp/training-operator-master > grep -rin "EnvKubeflowNamespace" .
./pkg/apis/mpi/v1/constants.go:20:	// EnvKubeflowNamespace is ENV for kubeflow namespace specified by user.
./pkg/apis/mpi/v1/constants.go:21:	EnvKubeflowNamespace = "KUBEFLOW_NAMESPACE"
./pkg/common/constants.go:18:	// EnvKubeflowNamespace is ENV for kubeflow namespace specified by user.
./pkg/common/constants.go:19:	EnvKubeflowNamespace = "KUBEFLOW_NAMESPACE"

I double checked with v1.2.0 and EnvKubeflowNamespace is used there. It seems the transition results in a change in the main function, which lost this option. But we can still remove it here as it's also defined in the kubeflow/common library: https://github.com/kubeflow/common/blob/97658773cce10b2b706e38858e48137ae370ed10/pkg/util/util.go#L31

I open another issue to restore these options: #1522

// DefaultPortName is name of the port used to communicate between Master and Workers.
DefaultPortName = "mpi-port"
// DefaultContainerName is the name of the MPIJob container.
Expand Down
20 changes: 0 additions & 20 deletions pkg/common/constants.go
View file
Open in desktop

This file was deleted.

9 changes: 0 additions & 9 deletions pkg/common/util/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,9 @@
package util

import (
"os"

"sigs.k8s.io/controller-runtime/pkg/client"
)

func HomeDir() string {
if h := os.Getenv("HOME"); h != "" {
return h
}
return os.Getenv("USERPROFILE") // windows
}

// TODO (Jeffwan@): Find an elegant way to either use delegatingReader or directly use clientss
// GetDelegatingClientFromClient try to extract client reader from client, client
// reader reads cluster info from api client.
Expand Down
9 changes: 9 additions & 0 deletions pkg/common/util/scheduler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,12 @@ func IsGangSchedulerSet(replicas map[commonv1.ReplicaType]*commonv1.ReplicaSpec,

return false
}

func GetSchedulerName(replicas map[commonv1.ReplicaType]*commonv1.ReplicaSpec) string {
for _, spec := range replicas {
if len(spec.Template.Spec.SchedulerName) > 0 {
return spec.Template.Spec.SchedulerName
}
}
return ""
}
8 changes: 0 additions & 8 deletions pkg/common/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,3 @@ func GetReplicaTypes(specs map[commonv1.ReplicaType]*commonv1.ReplicaSpec) []com
}
return keys
}
func GetSchedulerName(replicas map[commonv1.ReplicaType]*commonv1.ReplicaSpec) string {
for _, spec := range replicas {
if len(spec.Template.Spec.SchedulerName) > 0 {
return spec.Template.Spec.SchedulerName
}
}
return ""
}
2 changes: 1 addition & 1 deletion pkg/common/util/v1/testutil/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func NewServiceList(count int32, job metav1.Object, typ string, refs []metav1.Ow
return services
}

func SetServicesV2(client client.Client, job metav1.Object, typ string, activeWorkerServices int32,
func SetServices(client client.Client, job metav1.Object, typ string, activeWorkerServices int32,
refs []metav1.OwnerReference, basicLabels map[string]string) {
ctx := context.Background()
for _, svc := range NewServiceList(activeWorkerServices, job, typ, refs) {
Expand Down
78 changes: 0 additions & 78 deletions pkg/common/util/v1/unstructured/informer.go
View file
Open in desktop

This file was deleted.

14 changes: 14 additions & 0 deletions pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
// Copyright 2021 The Kubeflow Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License

package config

// Config is the global configuration for the training operator.
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller.v1/register_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"fmt"
"strings"

"sigs.k8s.io/controller-runtime/pkg/manager"

mpiv1 "github.com/kubeflow/training-operator/pkg/apis/mpi/v1"
mxnetv1 "github.com/kubeflow/training-operator/pkg/apis/mxnet/v1"
pytorchv1 "github.com/kubeflow/training-operator/pkg/apis/pytorch/v1"
Expand All @@ -28,7 +30,6 @@ import (
pytorchcontroller "github.com/kubeflow/training-operator/pkg/controller.v1/pytorch"
tensorflowcontroller "github.com/kubeflow/training-operator/pkg/controller.v1/tensorflow"
xgboostcontroller "github.com/kubeflow/training-operator/pkg/controller.v1/xgboost"
"sigs.k8s.io/controller-runtime/pkg/manager"
)

const ErrTemplateSchemeNotSupported = "scheme %s is not supported yet"
Expand Down
76 changes: 76 additions & 0 deletions pkg/controller.v1/register_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
// Copyright 2022 The Kubeflow Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package controller_v1

import (
mpiv1 "github.com/kubeflow/training-operator/pkg/apis/mpi/v1"
tfv1 "github.com/kubeflow/training-operator/pkg/apis/tensorflow/v1"
"testing"
)

func TestEnabledSchemes(t *testing.T) {
testES := EnabledSchemes{}

if testES.String() != "" {
t.Errorf("empty EnabledSchemes converted no-empty string %s", testES.String())
}

if !testES.Empty() {
t.Error("Empty method returned false for empty EnabledSchemes")
}

if testES.Set("TFJob") != nil {
t.Error("failed to restore TFJob")
} else {
stored := false
for _, kind := range testES {
if kind == tfv1.Kind {
stored = true
}
}
if !stored {
t.Errorf("%s not successfully registered", tfv1.Kind)
}
}

if testES.Set("mpijob") != nil {
t.Error("failed to restore PyTorchJob(pytorchjob)")
} else {
stored := false
for _, kind := range testES {
if kind == mpiv1.Kind {
stored = true
}
}
if !stored {
t.Errorf("%s not successfully registered", mpiv1.Kind)
}
}

dummyJob := "dummyjob"
if testES.Set(dummyJob) == nil {
t.Errorf("successfully registerd non-supported job %s", dummyJob)
}

if testES.Empty() {
t.Error("Empty method returned true for non-empty EnabledSchemes")
}

es2 := EnabledSchemes{}
es2.FillAll()
if es2.Empty() {
t.Error("Empty method returned true for fully registered EnabledSchemes")
}
}
12 changes: 6 additions & 6 deletions pkg/controller.v1/tensorflow/job_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,8 @@ var _ = Describe("TFJob controller", func() {
tc.pendingPSPods, tc.activePSPods, tc.succeededPSPods, tc.failedPSPods,
nil, refs, basicLabels)

testutil.SetServicesV2(testK8sClient, tc.tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServicesV2(testK8sClient, tc.tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tc.tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tc.tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)

podList := &corev1.PodList{}
Expect(testK8sClient.List(ctx, podList, listOpt)).Should(Succeed())
Expand Down Expand Up @@ -385,8 +385,8 @@ var _ = Describe("TFJob controller", func() {
tc.pendingPSPods, tc.activePSPods, tc.succeededPSPods, tc.failedPSPods,
nil, refs, basicLabels)

testutil.SetServicesV2(testK8sClient, tc.tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServicesV2(testK8sClient, tc.tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tc.tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tc.tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)

podList := &corev1.PodList{}
Expect(testK8sClient.List(ctx, podList, listOpt)).Should(Succeed())
Expand Down Expand Up @@ -497,8 +497,8 @@ var _ = Describe("TFJob controller", func() {
tc.pendingPSPods, tc.activePSPods, tc.succeededPSPods, tc.failedPSPods,
tc.restartCounts, refs, basicLabels)

testutil.SetServicesV2(testK8sClient, tc.tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServicesV2(testK8sClient, tc.tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tc.tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tc.tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)

podList := &corev1.PodList{}
Expect(testK8sClient.List(ctx, podList, listOpt)).Should(Succeed())
Expand Down
19 changes: 14 additions & 5 deletions pkg/controller.v1/tensorflow/status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,8 @@ func setStatusForTest(tfJob *tfv1.TFJob, rtype commonv1.ReplicaType, failed, suc
default:
fmt.Println("wrong type")
}
Expect(typ).ShouldNot(Equal(""))

refs := []metav1.OwnerReference{
*reconciler.GenOwnerReference(tfJob),
}
Expand All @@ -500,9 +502,11 @@ func setStatusForTest(tfJob *tfv1.TFJob, rtype commonv1.ReplicaType, failed, suc
pod.Labels[k] = v
}
po := &corev1.Pod{}
_ = client.Create(ctx, pod)
Expect(client.Create(ctx, pod)).Should(Succeed())

key := genKeyFromJob(pod)
Eventually(func() error {
po = &corev1.Pod{}
if err := client.Get(ctx, key, po); err != nil {
return err
}
Expand All @@ -511,7 +515,7 @@ func setStatusForTest(tfJob *tfv1.TFJob, rtype commonv1.ReplicaType, failed, suc
if worker0Completed == true && rtype == tfv1.TFReplicaTypeWorker && index == 0 {
po.Status.ContainerStatuses = []corev1.ContainerStatus{
{
Name: tfv1.DefaultContainerName,
Name: reconciler.GetDefaultContainerName(),
State: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: int32(0), // exit with 0
Expand All @@ -528,16 +532,18 @@ func setStatusForTest(tfJob *tfv1.TFJob, rtype commonv1.ReplicaType, failed, suc

index++
}

for i = 0; i < failed; i++ {
pod := testutil.NewPod(tfJob, typ, index, refs)
for k, v := range basicLabels {
pod.Labels[k] = v
}
po := &corev1.Pod{}
_ = client.Create(ctx, pod)
Expect(client.Create(ctx, pod)).Should(Succeed())

key := genKeyFromJob(pod)
Eventually(func() error {

po = &corev1.Pod{}
if err := client.Get(ctx, key, po); err != nil {
return err
}
Expand All @@ -547,7 +553,7 @@ func setStatusForTest(tfJob *tfv1.TFJob, rtype commonv1.ReplicaType, failed, suc
if po.Status.ContainerStatuses == nil {
po.Status.ContainerStatuses = []corev1.ContainerStatus{
{
Name: tfv1.DefaultContainerName,
Name: reconciler.GetDefaultContainerName(),
State: corev1.ContainerState{
Terminated: &corev1.ContainerStateTerminated{
ExitCode: int32(130), // 130 is a retryable code
Expand All @@ -564,15 +570,18 @@ func setStatusForTest(tfJob *tfv1.TFJob, rtype commonv1.ReplicaType, failed, suc
updateJobReplicaStatuses(&tfJob.Status, rtype, po)
index++
}

for i = 0; i < active; i++ {
pod := testutil.NewPod(tfJob, typ, index, refs)
for k, v := range basicLabels {
pod.Labels[k] = v
}
po := &corev1.Pod{}
Expect(client.Create(ctx, pod)).Should(Succeed())

key := genKeyFromJob(pod)
Eventually(func() error {
po = &corev1.Pod{}
if err := client.Get(ctx, key, po); err != nil {
return err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller.v1/tensorflow/tfjob_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,8 @@ var _ = Describe("TFJob controller", func() {
testutil.SetPodsStatuses(testK8sClient, tfJob, testutil.LabelWorker, tc.pendingWorkerPods, tc.activeWorkerPods, tc.succeededWorkerPods, tc.failedWorkerPods, nil, refs, basicLabels)
testutil.SetPodsStatuses(testK8sClient, tfJob, testutil.LabelPS, tc.pendingPSPods, tc.activePSPods, tc.succeededPSPods, tc.failedPSPods, nil, refs, basicLabels)

testutil.SetServicesV2(testK8sClient, tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServicesV2(testK8sClient, tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tfJob, testutil.LabelWorker, tc.activeWorkerServices, refs, basicLabels)
testutil.SetServices(testK8sClient, tfJob, testutil.LabelPS, tc.activePSServices, refs, basicLabels)

totalPodNumber := int(tc.pendingWorkerPods + tc.activeWorkerPods + tc.succeededWorkerPods + tc.failedWorkerPods + tc.pendingPSPods + tc.activePSPods + tc.succeededPSPods + tc.failedPSPods)
totalServiceNumber := int(tc.activeWorkerServices + tc.activePSServices)
Expand Down
Loading