From 005df7ed6b34f5f1b361afbc2c0509c258a508a8 Mon Sep 17 00:00:00 2001 From: Ce Gao Date: Sun, 12 Jan 2020 10:31:58 +0800 Subject: [PATCH 1/3] feat: Do not inject sh -c when it exists Signed-off-by: Ce Gao --- .gitignore | 3 +++ pkg/webhook/v1alpha3/pod/inject_webhook.go | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/.gitignore b/.gitignore index c34dca5e785..e1c74f5da87 100644 --- a/.gitignore +++ b/.gitignore @@ -64,3 +64,6 @@ ehthumbs_vista.db ## Recycle Bin used on file shares $RECYCLE.BIN/ + +/katib-controller +/katib-manager diff --git a/pkg/webhook/v1alpha3/pod/inject_webhook.go b/pkg/webhook/v1alpha3/pod/inject_webhook.go index b564c8a770b..c90d4e53932 100644 --- a/pkg/webhook/v1alpha3/pod/inject_webhook.go +++ b/pkg/webhook/v1alpha3/pod/inject_webhook.go @@ -307,6 +307,13 @@ func wrapWorkerContainer( if err != nil { return err } + // If the first two commands are sh -c, we do not inject command. + if args[0] == "sh" || args[0] == "bash" { + if args[1] == "-c" { + command = args[0:2] + args = args[2:] + } + } if mc.Collector.Kind == common.StdOutCollector { redirectStr := fmt.Sprintf("1>%s 2>&1", metricsFile) args = append(args, redirectStr) From bdfa606c1bb6e4148007eef9aaecbef847fc6d35 Mon Sep 17 00:00:00 2001 From: Ce Gao Date: Sun, 12 Jan 2020 10:42:54 +0800 Subject: [PATCH 2/3] fix: Add test cases Signed-off-by: Ce Gao --- .../v1alpha3/pod/inject_webhook_test.go | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 pkg/webhook/v1alpha3/pod/inject_webhook_test.go diff --git a/pkg/webhook/v1alpha3/pod/inject_webhook_test.go b/pkg/webhook/v1alpha3/pod/inject_webhook_test.go new file mode 100644 index 00000000000..f3e937464a1 --- /dev/null +++ b/pkg/webhook/v1alpha3/pod/inject_webhook_test.go @@ -0,0 +1,34 @@ +package pod + +import ( + "testing" + + common "github.com/kubeflow/katib/pkg/apis/controller/common/v1alpha3" + v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" +) + +func TestWrapWorkerContainer(t *testing.T) { + testCases := []struct { + Pod *v1.Pod + Namespace string + JobKind string + MetricsFile string + PathKind common.FileSystemKind + MC common.MetricsCollectorSpec + Expected *v1.Pod + ExpectedError error + }{} + + for _, c := range testCases { + err := wrapWorkerContainer(c.Pod, c.Namespace, c.JobKind, c.MetricsFile, c.PathKind, c.MC) + if err != c.ExpectedError { + t.Errorf("Expected error %v, got %v", c.ExpectedError, err) + } + if err == nil { + if !equality.Semantic.DeepEqual(c.Pod.Spec, c.Expected.Spec) { + t.Errorf("Expected pod %v, got %v", c.Pod.Spec, c.Expected.Spec) + } + } + } +} From bc60a1509bfb188ab350d688ebaac890a04da7d1 Mon Sep 17 00:00:00 2001 From: Ce Gao Date: Mon, 13 Jan 2020 14:09:20 +0800 Subject: [PATCH 3/3] fix: Add test cases Signed-off-by: Ce Gao --- .../v1alpha3/pod/inject_webhook_test.go | 127 +++++++++++++++++- 1 file changed, 124 insertions(+), 3 deletions(-) diff --git a/pkg/webhook/v1alpha3/pod/inject_webhook_test.go b/pkg/webhook/v1alpha3/pod/inject_webhook_test.go index f3e937464a1..51fcc9ee785 100644 --- a/pkg/webhook/v1alpha3/pod/inject_webhook_test.go +++ b/pkg/webhook/v1alpha3/pod/inject_webhook_test.go @@ -18,7 +18,127 @@ func TestWrapWorkerContainer(t *testing.T) { MC common.MetricsCollectorSpec Expected *v1.Pod ExpectedError error - }{} + Name string + }{ + { + Pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "tensorflow", + Command: []string{ + "python main.py", + }, + }, + }, + }, + }, + Namespace: "nohere", + JobKind: "TFJob", + MetricsFile: "testfile", + PathKind: common.FileKind, + MC: common.MetricsCollectorSpec{ + Collector: &common.CollectorSpec{ + Kind: common.StdOutCollector, + }, + }, + Expected: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "tensorflow", + Command: []string{ + "sh", "-c", + }, + Args: []string{ + "python main.py 1>testfile 2>&1 && echo completed > $$$$.pid", + }, + }, + }, + }, + }, + ExpectedError: nil, + Name: "tensorflow container without sh -c", + }, + { + Pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "test", + Command: []string{ + "python main.py", + }, + }, + }, + }, + }, + Namespace: "nohere", + JobKind: "TFJob", + MetricsFile: "testfile", + PathKind: common.FileKind, + MC: common.MetricsCollectorSpec{ + Collector: &common.CollectorSpec{ + Kind: common.StdOutCollector, + }, + }, + Expected: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "test", + Command: []string{ + "python main.py", + }, + }, + }, + }, + }, + ExpectedError: nil, + Name: "test container without sh -c", + }, + { + Pod: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "tensorflow", + Command: []string{ + "sh", "-c", + "python main.py", + }, + }, + }, + }, + }, + Namespace: "nohere", + JobKind: "TFJob", + MetricsFile: "testfile", + PathKind: common.FileKind, + MC: common.MetricsCollectorSpec{ + Collector: &common.CollectorSpec{ + Kind: common.StdOutCollector, + }, + }, + Expected: &v1.Pod{ + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: "tensorflow", + Command: []string{ + "sh", "-c", + }, + Args: []string{ + "python main.py 1>testfile 2>&1 && echo completed > $$$$.pid", + }, + }, + }, + }, + }, + ExpectedError: nil, + Name: "Tensorflow container with sh -c", + }, + } for _, c := range testCases { err := wrapWorkerContainer(c.Pod, c.Namespace, c.JobKind, c.MetricsFile, c.PathKind, c.MC) @@ -26,8 +146,9 @@ func TestWrapWorkerContainer(t *testing.T) { t.Errorf("Expected error %v, got %v", c.ExpectedError, err) } if err == nil { - if !equality.Semantic.DeepEqual(c.Pod.Spec, c.Expected.Spec) { - t.Errorf("Expected pod %v, got %v", c.Pod.Spec, c.Expected.Spec) + if !equality.Semantic.DeepEqual(c.Pod.Spec.Containers, c.Expected.Spec.Containers) { + t.Errorf("Case %s: Expected pod %v, got %v", + c.Name, c.Expected.Spec.Containers, c.Pod.Spec.Containers) } } }