From b09804066e71f1516201b53caa5b50920f303218 Mon Sep 17 00:00:00 2001 From: gabemontero Date: Tue, 21 May 2019 18:28:59 -0400 Subject: [PATCH] avoid builds on non-linux nodes --- .../controller/build/defaults/defaults.go | 19 ++++++++- .../build/defaults/defaults_test.go | 12 ++++++ .../controller/build/overrides/overrides.go | 4 ++ .../build/overrides/overrides_test.go | 14 ++++++- .../controller/build/podcreationstrategy.go | 5 +++ .../build/podcreationstrategy_test.go | 6 ++- test/extended/builds/start.go | 41 +++++++++++++++---- test/integration/buildpod_admission_test.go | 4 +- 8 files changed, 91 insertions(+), 14 deletions(-) diff --git a/pkg/build/controller/build/defaults/defaults.go b/pkg/build/controller/build/defaults/defaults.go index adb498e3433f..badf880c6466 100644 --- a/pkg/build/controller/build/defaults/defaults.go +++ b/pkg/build/controller/build/defaults/defaults.go @@ -1,6 +1,8 @@ package defaults import ( + "strings" + "k8s.io/klog" "k8s.io/kubernetes/pkg/api/legacyscheme" @@ -108,11 +110,24 @@ func (b BuildDefaults) applyPodProxyDefaults(pod *corev1.Pod, isCustomBuild bool } func (b BuildDefaults) applyPodDefaults(pod *corev1.Pod, isCustomBuild bool) { - if len(b.Config.NodeSelector) != 0 && pod.Spec.NodeSelector == nil { + nodeSelectorAppliable := pod.Spec.NodeSelector == nil + if !nodeSelectorAppliable && len(pod.Spec.NodeSelector) == 1 { + v, ok := pod.Spec.NodeSelector[corev1.LabelOSStable] + if ok && v == "linux" { + nodeSelectorAppliable = true + } + } + if len(b.Config.NodeSelector) != 0 && nodeSelectorAppliable { // only apply nodeselector defaults if the pod has no nodeselector labels // already. - pod.Spec.NodeSelector = map[string]string{} + if pod.Spec.NodeSelector == nil { + pod.Spec.NodeSelector = map[string]string{} + } for k, v := range b.Config.NodeSelector { + // can't override kubernetes.io/os + if strings.TrimSpace(k) == corev1.LabelOSStable { + continue + } addDefaultNodeSelector(k, v, pod.Spec.NodeSelector) } } diff --git a/pkg/build/controller/build/defaults/defaults_test.go b/pkg/build/controller/build/defaults/defaults_test.go index 74051cf32297..295a22df2d3f 100644 --- a/pkg/build/controller/build/defaults/defaults_test.go +++ b/pkg/build/controller/build/defaults/defaults_test.go @@ -516,6 +516,18 @@ func TestBuildDefaultsNodeSelector(t *testing.T) { defaults: map[string]string{"key1": "default1", "key2": "default2"}, expected: map[string]string{"key1": "default1", "key2": "default2"}, }, + { + name: "build - non empty linux node only", + build: testutil.Build().WithNodeSelector(map[string]string{corev1.LabelOSStable: "linux"}).AsBuild(), + defaults: map[string]string{"key1": "default1"}, + expected: map[string]string{"key1": "default1", corev1.LabelOSStable: "linux"}, + }, + { + name: "build - try to change linux node only", + build: testutil.Build().WithNodeSelector(map[string]string{corev1.LabelOSStable: "linux"}).AsBuild(), + defaults: map[string]string{corev1.LabelOSStable: "windows"}, + expected: map[string]string{corev1.LabelOSStable: "linux"}, + }, { name: "build - ignored", build: testutil.Build().WithNodeSelector(map[string]string{"key1": "value1"}).AsBuild(), diff --git a/pkg/build/controller/build/overrides/overrides.go b/pkg/build/controller/build/overrides/overrides.go index c5b3b20a6674..b42b047d6fc0 100644 --- a/pkg/build/controller/build/overrides/overrides.go +++ b/pkg/build/controller/build/overrides/overrides.go @@ -2,6 +2,7 @@ package overrides import ( "fmt" + "strings" "k8s.io/klog" @@ -64,6 +65,9 @@ func (b BuildOverrides) ApplyOverrides(pod *corev1.Pod) error { pod.Spec.NodeSelector = map[string]string{} } for k, v := range b.Config.NodeSelector { + if strings.TrimSpace(k) == corev1.LabelOSStable { + continue + } klog.V(5).Infof("Adding override nodeselector %s=%s to build pod %s/%s", k, v, pod.Namespace, pod.Name) pod.Spec.NodeSelector[k] = v } diff --git a/pkg/build/controller/build/overrides/overrides_test.go b/pkg/build/controller/build/overrides/overrides_test.go index 03c3a2b4393e..77c48c80a124 100644 --- a/pkg/build/controller/build/overrides/overrides_test.go +++ b/pkg/build/controller/build/overrides/overrides_test.go @@ -4,7 +4,7 @@ import ( "reflect" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apiserver/pkg/admission" buildv1 "github.com/openshift/api/build/v1" @@ -216,6 +216,18 @@ func TestBuildOverrideNodeSelector(t *testing.T) { overrides: map[string]string{"key2": "override2"}, expected: map[string]string{"key1": "value1", "key2": "override2"}, }, + { + name: "build - non empty linux node only", + build: testutil.Build().WithNodeSelector(map[string]string{v1.LabelOSStable: "linux"}).AsBuild(), + overrides: map[string]string{"key1": "default1"}, + expected: map[string]string{"key1": "default1", v1.LabelOSStable: "linux"}, + }, + { + name: "build - try to change linux node only", + build: testutil.Build().WithNodeSelector(map[string]string{v1.LabelOSStable: "linux"}).AsBuild(), + overrides: map[string]string{v1.LabelOSStable: "windows"}, + expected: map[string]string{v1.LabelOSStable: "linux"}, + }, } for _, test := range tests { diff --git a/pkg/build/controller/build/podcreationstrategy.go b/pkg/build/controller/build/podcreationstrategy.go index e3d842b122ad..fee27f40fcb8 100644 --- a/pkg/build/controller/build/podcreationstrategy.go +++ b/pkg/build/controller/build/podcreationstrategy.go @@ -42,6 +42,11 @@ func (f *typeBasedFactoryStrategy) CreateBuildPod(build *buildv1.Build, addition pod.Annotations = map[string]string{} } pod.Annotations[buildutil.BuildAnnotation] = build.Name + + if pod.Spec.NodeSelector == nil { + pod.Spec.NodeSelector = map[string]string{} + } + pod.Spec.NodeSelector[corev1.LabelOSStable] = "linux" } return pod, err } diff --git a/pkg/build/controller/build/podcreationstrategy_test.go b/pkg/build/controller/build/podcreationstrategy_test.go index 140d44422747..e92fdfe7df3b 100644 --- a/pkg/build/controller/build/podcreationstrategy_test.go +++ b/pkg/build/controller/build/podcreationstrategy_test.go @@ -4,7 +4,7 @@ import ( "fmt" "testing" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" buildv1 "github.com/openshift/api/build/v1" ) @@ -111,5 +111,9 @@ func TestStrategyCreateBuildPod(t *testing.T) { if pod != test.expectedPod { t.Errorf("did not get expected pod with build %#v", test.build) } + osType, _ := pod.Spec.NodeSelector[v1.LabelOSStable] + if osType != "linux" { + t.Errorf("did not get expected node selector in pod %#v", test.build) + } } } diff --git a/test/extended/builds/start.go b/test/extended/builds/start.go index 9515d60cbc6a..31be241b5a4d 100644 --- a/test/extended/builds/start.go +++ b/test/extended/builds/start.go @@ -12,6 +12,7 @@ import ( g "github.com/onsi/ginkgo" o "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" e2e "k8s.io/kubernetes/test/e2e/framework" @@ -24,14 +25,21 @@ import ( var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { defer g.GinkgoRecover() var ( - buildFixture = exutil.FixturePath("testdata", "builds", "test-build.yaml") - bcWithPRRef = exutil.FixturePath("testdata", "builds", "test-bc-with-pr-ref.yaml") - exampleGemfile = exutil.FixturePath("testdata", "builds", "test-build-app", "Gemfile") - exampleBuild = exutil.FixturePath("testdata", "builds", "test-build-app") - symlinkFixture = exutil.FixturePath("testdata", "builds", "test-symlink-build.yaml") - exampleGemfileURL = "https://raw.githubusercontent.com/openshift/ruby-hello-world/master/Gemfile" - exampleArchiveURL = "https://github.com/openshift/ruby-hello-world/archive/master.zip" - oc = exutil.NewCLI("cli-start-build", exutil.KubeConfigPath()) + buildFixture = exutil.FixturePath("testdata", "builds", "test-build.yaml") + bcWithPRRef = exutil.FixturePath("testdata", "builds", "test-bc-with-pr-ref.yaml") + exampleGemfile = exutil.FixturePath("testdata", "builds", "test-build-app", "Gemfile") + exampleBuild = exutil.FixturePath("testdata", "builds", "test-build-app") + symlinkFixture = exutil.FixturePath("testdata", "builds", "test-symlink-build.yaml") + exampleGemfileURL = "https://raw.githubusercontent.com/openshift/ruby-hello-world/master/Gemfile" + exampleArchiveURL = "https://github.com/openshift/ruby-hello-world/archive/master.zip" + oc = exutil.NewCLI("cli-start-build", exutil.KubeConfigPath()) + verifyNodeSelector = func(oc *exutil.CLI, name string) { + pod, err := oc.KubeClient().CoreV1().Pods(oc.Namespace()).Get(name+"-build", metav1.GetOptions{}) + o.Expect(err).NotTo(o.HaveOccurred()) + os, ok := pod.Spec.NodeSelector[corev1.LabelOSStable] + o.Expect(ok).To(o.BeTrue()) + o.Expect(os).To(o.Equal("linux")) + } ) g.Context("", func() { @@ -57,6 +65,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { br, err := exutil.StartBuildAndWait(oc, "sample-build", "--wait") o.Expect(err).NotTo(o.HaveOccurred()) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) }) g.It("should start a build and wait for the build to fail", func() { @@ -65,6 +74,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { br.AssertFailure() o.Expect(br.StartBuildErr).To(o.HaveOccurred()) // start-build should detect the build error with --wait flag o.Expect(br.StartBuildStdErr).Should(o.ContainSubstring(`status is "Failed"`)) + verifyNodeSelector(oc, br.BuildName) }) }) @@ -82,9 +92,11 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { br, err := exutil.StartBuildAndWait(oc, "bc-with-pr-ref-docker") o.Expect(err).NotTo(o.HaveOccurred()) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) br, err = exutil.StartBuildAndWait(oc, "bc-with-pr-ref") o.Expect(err).NotTo(o.HaveOccurred()) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) out, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) @@ -111,6 +123,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build with -e FOO=bar,VAR=test") br, err := exutil.StartBuildAndWait(oc, "sample-build", "-e", "FOO=bar,VAR=test") br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) @@ -127,6 +140,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build with buildconfig strategy env BUILD_LOGLEVEL=5") br, err := exutil.StartBuildAndWait(oc, "sample-verbose-build") br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) g.By(fmt.Sprintf("verifying the build output is verbose")) @@ -141,6 +155,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build with buildconfig strategy env BUILD_LOGLEVEL=5 but build-loglevel=1") br, err := exutil.StartBuildAndWait(oc, "sample-verbose-build", "--build-loglevel=1") br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) g.By(fmt.Sprintf("verifying the build output is not verbose")) @@ -163,6 +178,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build with a Dockerfile") br, err := exutil.StartBuildAndWait(oc, "sample-build", fmt.Sprintf("--from-file=%s", exampleGemfile)) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) g.By(fmt.Sprintf("verifying the build %q status", br.BuildPath)) @@ -176,6 +192,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build with a directory") br, err := exutil.StartBuildAndWait(oc, "sample-build", fmt.Sprintf("--from-dir=%s", exampleBuild)) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) g.By(fmt.Sprintf("verifying the build %q status", br.BuildPath)) @@ -189,6 +206,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { tryRepoInit(exampleBuild) br, err := exutil.StartBuildAndWait(oc, "sample-build", fmt.Sprintf("--from-repo=%s", exampleBuild)) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) @@ -208,6 +226,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { o.Expect(err).NotTo(o.HaveOccurred()) br, err := exutil.StartBuildAndWait(oc, "sample-build", fmt.Sprintf("--commit=%s", commit), fmt.Sprintf("--from-repo=%s", exampleBuild)) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) @@ -223,6 +242,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting a valid build with a directory") br, err := exutil.StartBuildAndWait(oc, "sample-build-binary", "--follow", fmt.Sprintf("--from-dir=%s", exampleBuild)) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) o.Expect(br.StartBuildStdErr).To(o.ContainSubstring("Uploading directory")) @@ -244,6 +264,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting a valid build with input file served by https") br, err := exutil.StartBuildAndWait(oc, "sample-build", fmt.Sprintf("--from-file=%s", exampleGemfileURL)) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) o.Expect(br.StartBuildStdErr).To(o.ContainSubstring(fmt.Sprintf("Uploading file from %q as binary input for the build", exampleGemfileURL))) @@ -255,6 +276,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { // can't use sample-build-binary because we need contextDir due to github archives containing the top-level directory br, err := exutil.StartBuildAndWait(oc, "sample-build-github-archive", fmt.Sprintf("--from-archive=%s", exampleArchiveURL)) br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) o.Expect(br.StartBuildStdErr).To(o.ContainSubstring(fmt.Sprintf("Uploading archive from %q as binary input for the build", exampleArchiveURL))) @@ -329,6 +351,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build without --build-arg flag") br, _ := exutil.StartBuildAndWait(oc, "sample-build-docker-args-preset") br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) g.By("verifying the build output contains the build args from the BuildConfig.") @@ -338,6 +361,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build with --build-arg flag") br, _ := exutil.StartBuildAndWait(oc, "sample-build-docker-args", "--build-arg=foo=bar") br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) g.By("verifying the build output contains the changes.") @@ -347,6 +371,7 @@ var _ = g.Describe("[Feature:Builds][Slow] starting a build using CLI", func() { g.By("starting the build with --build-arg flag") br, _ := exutil.StartBuildAndWait(oc, "sample-build-docker-args", "--build-arg=bar=foo") br.AssertSuccess() + verifyNodeSelector(oc, br.BuildName) buildLog, err := br.Logs() o.Expect(err).NotTo(o.HaveOccurred()) g.By("verifying the build completed with a warning.") diff --git a/test/integration/buildpod_admission_test.go b/test/integration/buildpod_admission_test.go index a2f9638f04a6..32df490f1776 100644 --- a/test/integration/buildpod_admission_test.go +++ b/test/integration/buildpod_admission_test.go @@ -96,7 +96,7 @@ func TestBuildDefaultLabels(t *testing.T) { } func TestBuildDefaultNodeSelectors(t *testing.T) { - selectors := map[string]string{"KEY": "VALUE"} + selectors := map[string]string{"KEY": "VALUE", v1.LabelOSStable: "linux"} oclient, kclientset, fn := setupBuildDefaultsAdmissionTest(t, &configapi.BuildDefaultsConfig{ NodeSelector: selectors, }) @@ -205,7 +205,7 @@ func TestBuildOverrideLabels(t *testing.T) { } func TestBuildOverrideNodeSelectors(t *testing.T) { - selectors := map[string]string{"KEY": "VALUE"} + selectors := map[string]string{"KEY": "VALUE", v1.LabelOSStable: "linux"} oclient, kclientset, fn := setupBuildOverridesAdmissionTest(t, &configapi.BuildOverridesConfig{ NodeSelector: selectors, })