Merge pull request #22885 from gabemontero/bld-linux-nodes-only

avoid builds on non-linux nodes

Author: openshift-merge-robot

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)
 		}
 	}

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(),

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
 	}

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 {

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
 }

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)
+		}
 	}
 }

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.")

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,
 	})