Issue 3502 - removing label propagation to build pods in build

controller.

UPGRADE.md

@@ -0,0 +1,14 @@
+# Upgrading
+
+This document describes future changes that will affect your current resources used
+inside of OpenShift. Each change contains description of the change and information
+when that change will happen.
+
+
+## Origin 1.0.x / OSE 3.0.x
+
+* Currently all build pods have a label named `build`. This label is being deprecated
+  in favor of `openshift.io/build.name` in Origin 1.0.x / OSE 3.1.x at which point both
+  labels will be supported. All the newly created builds will have just the new label.
+  In Origin 1.y / OSE 3.y the support for the old label (`build`) will be removed entirely.
+  See #3502.

pkg/build/api/types.go

@@ -10,9 +10,10 @@ import (
 const (
 	// BuildAnnotation is an annotation that identifies a Pod as being for a Build
 	BuildAnnotation = "openshift.io/build.name"
-
+	// DeprecatedBuildLabel is old value of BuildLabel, it'll be removed in OpenShift 3.1.
+	DeprecatedBuildLabel = "build"
 	// BuildLabel is the key of a Pod label whose value is the Name of a Build which is run.
-	BuildLabel = "build"
+	BuildLabel = "openshift.io/build.name"
 )
 
 // Build encapsulates the inputs needed to produce a new deployable image, as well as

pkg/build/api/v1/types.go

@@ -7,9 +7,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
-// BuildLabel is the key of a Pod label whose value is the Name of a Build which is run.
-const BuildLabel = "build"
-
 // Build encapsulates the inputs needed to produce a new deployable image, as well as
 // the status of the execution and a reference to the Pod which executed the build.
 type Build struct {

pkg/build/api/v1beta3/types.go

@@ -7,9 +7,6 @@ import (
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
-// BuildLabel is the key of a Pod label whose value is the Name of a Build which is run.
-const BuildLabel = "build"
-
 // Build encapsulates the inputs needed to produce a new deployable image, as well as
 // the status of the execution and a reference to the Pod which executed the build.
 type Build struct {

pkg/build/controller/controller.go

@@ -303,8 +303,8 @@ func (bc *BuildDeleteController) HandleBuildDeletion(build *buildapi.Build) erro
 		glog.V(2).Infof("Did not find pod with name %s for Build %s in namespace %s", podName, build.Name, build.Namespace)
 		return nil
 	}
-	if pod.Labels[buildapi.BuildLabel] != build.Name {
-		glog.V(2).Infof("Not deleting pod %s/%s because the build label %s does not match the build name %s", pod.Namespace, podName, pod.Labels[buildapi.BuildLabel], build.Name)
+	if buildName, _ := buildutil.GetBuildLabel(pod); buildName != build.Name {
+		glog.V(2).Infof("Not deleting pod %s/%s because the build label %s does not match the build name %s", pod.Namespace, podName, buildName, build.Name)
 		return nil
 	}
 	err = bc.PodManager.DeletePod(build.Namespace, pod)

pkg/build/controller/controller_test.go

@@ -108,7 +108,7 @@ func (*errNotFoundImageStreamClient) GetImageStream(namespace, name string) (*im
 	return nil, kerrors.NewNotFound("ImageStream", name)
 }
 
-func mockBuild(status buildapi.BuildPhase, output buildapi.BuildOutput) *buildapi.Build {
+func mockBuild(phase buildapi.BuildPhase, output buildapi.BuildOutput) *buildapi.Build {
 	return &buildapi.Build{
 		ObjectMeta: kapi.ObjectMeta{
 			Name:      "data-build",
@@ -132,7 +132,7 @@ func mockBuild(status buildapi.BuildPhase, output buildapi.BuildOutput) *buildap
 			Output: output,
 		},
 		Status: buildapi.BuildStatus{
-			Phase: status,
+			Phase: phase,
 		},
 	}
 }
@@ -665,7 +665,7 @@ func (c *customPodManager) GetPod(namespace, name string) (*kapi.Pod, error) {
 
 func TestHandleHandleBuildDeletionOK(t *testing.T) {
 	deleteWasCalled := false
-	build := mockBuild(buildapi.BuildStatusComplete, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseComplete, buildapi.BuildOutput{})
 	ctrl := BuildDeleteController{&customPodManager{
 		GetPodFunc: func(namespace, names string) (*kapi.Pod, error) {
 			return &kapi.Pod{ObjectMeta: kapi.ObjectMeta{Labels: map[string]string{buildapi.BuildLabel: build.Name}}}, nil
@@ -685,8 +685,30 @@ func TestHandleHandleBuildDeletionOK(t *testing.T) {
 	}
 }
 
+func TestHandleHandleBuildDeletionOKDeprecatedLabel(t *testing.T) {
+	deleteWasCalled := false
+	build := mockBuild(buildapi.BuildPhaseComplete, buildapi.BuildOutput{})
+	ctrl := BuildDeleteController{&customPodManager{
+		GetPodFunc: func(namespace, names string) (*kapi.Pod, error) {
+			return &kapi.Pod{ObjectMeta: kapi.ObjectMeta{Labels: map[string]string{buildapi.DeprecatedBuildLabel: build.Name}}}, nil
+		},
+		DeletePodFunc: func(namespace string, pod *kapi.Pod) error {
+			deleteWasCalled = true
+			return nil
+		},
+	}}
+
+	err := ctrl.HandleBuildDeletion(build)
+	if err != nil {
+		t.Errorf("Unexpected error %v", err)
+	}
+	if !deleteWasCalled {
+		t.Error("DeletePod was not called when it should!")
+	}
+}
+
 func TestHandleHandleBuildDeletionFailGetPod(t *testing.T) {
-	build := mockBuild(buildapi.BuildStatusComplete, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseComplete, buildapi.BuildOutput{})
 	ctrl := BuildDeleteController{&customPodManager{
 		GetPodFunc: func(namespace, name string) (*kapi.Pod, error) {
 			return nil, errors.New("random")
@@ -701,7 +723,7 @@ func TestHandleHandleBuildDeletionFailGetPod(t *testing.T) {
 
 func TestHandleHandleBuildDeletionGetPodNotFound(t *testing.T) {
 	deleteWasCalled := false
-	build := mockBuild(buildapi.BuildStatusComplete, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseComplete, buildapi.BuildOutput{})
 	ctrl := BuildDeleteController{&customPodManager{
 		GetPodFunc: func(namespace, name string) (*kapi.Pod, error) {
 			return nil, kerrors.NewNotFound("Pod", name)
@@ -723,7 +745,7 @@ func TestHandleHandleBuildDeletionGetPodNotFound(t *testing.T) {
 
 func TestHandleHandleBuildDeletionMismatchedLabels(t *testing.T) {
 	deleteWasCalled := false
-	build := mockBuild(buildapi.BuildStatusComplete, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseComplete, buildapi.BuildOutput{})
 	ctrl := BuildDeleteController{&customPodManager{
 		GetPodFunc: func(namespace, names string) (*kapi.Pod, error) {
 			return &kapi.Pod{}, nil
@@ -744,7 +766,7 @@ func TestHandleHandleBuildDeletionMismatchedLabels(t *testing.T) {
 }
 
 func TestHandleHandleBuildDeletionDeletePodError(t *testing.T) {
-	build := mockBuild(buildapi.BuildStatusComplete, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseComplete, buildapi.BuildOutput{})
 	ctrl := BuildDeleteController{&customPodManager{
 		GetPodFunc: func(namespace, names string) (*kapi.Pod, error) {
 			return &kapi.Pod{ObjectMeta: kapi.ObjectMeta{Labels: map[string]string{buildapi.BuildLabel: build.Name}}}, nil
@@ -778,7 +800,7 @@ func mockBuildPodDeleteController(build *buildapi.Build, buildUpdater *customBui
 func TestHandleBuildPodDeletionOK(t *testing.T) {
 	updateWasCalled := false
 	// only not finished build (buildutil.IsBuildComplete) should be handled
-	build := mockBuild(buildapi.BuildStatusRunning, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseRunning, buildapi.BuildOutput{})
 	ctrl := mockBuildPodDeleteController(build, &customBuildUpdater{
 		UpdateFunc: func(namespace string, build *buildapi.Build) error {
 			updateWasCalled = true
@@ -799,7 +821,7 @@ func TestHandleBuildPodDeletionOK(t *testing.T) {
 func TestHandleBuildPodDeletionOKFinishedBuild(t *testing.T) {
 	updateWasCalled := false
 	// finished build buildutil.IsBuildComplete should not be handled
-	build := mockBuild(buildapi.BuildStatusComplete, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseComplete, buildapi.BuildOutput{})
 	ctrl := mockBuildPodDeleteController(build, &customBuildUpdater{
 		UpdateFunc: func(namespace string, build *buildapi.Build) error {
 			updateWasCalled = true
@@ -820,7 +842,7 @@ func TestHandleBuildPodDeletionOKFinishedBuild(t *testing.T) {
 func TestHandleBuildPodDeletionOKErroneousBuild(t *testing.T) {
 	updateWasCalled := false
 	// erroneous builds should not be handled
-	build := mockBuild(buildapi.BuildStatusError, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseError, buildapi.BuildOutput{})
 	ctrl := mockBuildPodDeleteController(build, &customBuildUpdater{
 		UpdateFunc: func(namespace string, build *buildapi.Build) error {
 			updateWasCalled = true
@@ -868,7 +890,7 @@ func TestHandleBuildPodDeletionBuildNotExists(t *testing.T) {
 }
 
 func TestHandleBuildPodDeletionBuildUpdateError(t *testing.T) {
-	build := mockBuild(buildapi.BuildStatusRunning, buildapi.BuildOutput{})
+	build := mockBuild(buildapi.BuildPhaseRunning, buildapi.BuildOutput{})
 	ctrl := mockBuildPodDeleteController(build, &customBuildUpdater{
 		UpdateFunc: func(namespace string, build *buildapi.Build) error {
 			return errors.New("random")

pkg/build/controller/factory/factory.go

@@ -323,13 +323,53 @@ type podLW struct {
 
 // List lists all Pods that have a build label.
 func (lw *podLW) List() (runtime.Object, error) {
-	sel, _ := labels.Parse(buildapi.BuildLabel)
-	return lw.client.Pods(kapi.NamespaceAll).List(sel, fields.Everything())
+	return listPods(lw.client)
+}
+
+func listPods(client kclient.Interface) (*kapi.PodList, error) {
+	// get builds with new label
+	sel, err := labels.Parse(buildapi.BuildLabel)
+	if err != nil {
+		return nil, err
+	}
+	listNew, err := client.Pods(kapi.NamespaceAll).List(sel, fields.Everything())
+	if err != nil {
+		return nil, err
+	}
+	// FIXME: get builds with old label - remove this when depracated label will be removed
+	selOld, err := labels.Parse(buildapi.DeprecatedBuildLabel)
+	if err != nil {
+		return nil, err
+	}
+	listOld, err := client.Pods(kapi.NamespaceAll).List(selOld, fields.Everything())
+	if err != nil {
+		return nil, err
+	}
+	listNew.Items = mergeWithoutDuplicates(listNew.Items, listOld.Items)
+	return listNew, nil
+}
+
+func mergeWithoutDuplicates(arrays ...[]kapi.Pod) []kapi.Pod {
+	tmpMap := make(map[string]kapi.Pod)
+	for _, array := range arrays {
+		for _, v := range array {
+			tmpMap[fmt.Sprintf("%s/%s", v.Namespace, v.Name)] = v
+		}
+	}
+	var result []kapi.Pod
+	for _, v := range tmpMap {
+		result = append(result, v)
+	}
+	return result
 }
 
 // Watch watches all Pods that have a build label.
 func (lw *podLW) Watch(resourceVersion string) (watch.Interface, error) {
-	sel, _ := labels.Parse(buildapi.BuildLabel)
+	// FIXME: since we cannot have OR on label name we'll just get builds with new label
+	sel, err := labels.Parse(buildapi.BuildLabel)
+	if err != nil {
+		return nil, err
+	}
 	return lw.client.Pods(kapi.NamespaceAll).Watch(sel, fields.Everything(), resourceVersion)
 }
 
@@ -357,31 +397,32 @@ type buildDeleteLW struct {
 // List returns an empty list but adds delete events to the store for all Builds that have been deleted but still have pods.
 func (lw *buildDeleteLW) List() (runtime.Object, error) {
 	glog.V(5).Info("Checking for deleted builds")
-	sel, _ := labels.Parse(buildapi.BuildLabel)
-	podList, err := lw.KubeClient.Pods(kapi.NamespaceAll).List(sel, fields.Everything())
+	podList, err := listPods(lw.KubeClient)
 	if err != nil {
 		glog.V(4).Infof("Failed to find any pods due to error %v", err)
 		return nil, err
 	}
+
 	for _, pod := range podList.Items {
-		if len(pod.Labels[buildapi.BuildLabel]) == 0 {
+		buildName, exists := buildutil.GetBuildLabel(&pod)
+		if !exists {
 			continue
 		}
 		glog.V(5).Infof("Found build pod %s/%s", pod.Namespace, pod.Name)
 
-		build, err := lw.Client.Builds(pod.Namespace).Get(pod.Labels[buildapi.BuildLabel])
+		build, err := lw.Client.Builds(pod.Namespace).Get(buildName)
 		if err != nil && !kerrors.IsNotFound(err) {
 			glog.V(4).Infof("Error getting build for pod %s/%s: %v", pod.Namespace, pod.Name, err)
 			return nil, err
 		}
 		if err != nil && kerrors.IsNotFound(err) {
 			build = nil
-		}
 
+		}
 		if build == nil {
 			deletedBuild := &buildapi.Build{
 				ObjectMeta: kapi.ObjectMeta{
-					Name:      pod.Labels[buildapi.BuildLabel],
+					Name:      buildName,
 					Namespace: pod.Namespace,
 				},
 			}
@@ -399,7 +440,6 @@ func (lw *buildDeleteLW) List() (runtime.Object, error) {
 
 // Watch watches all Builds.
 func (lw *buildDeleteLW) Watch(resourceVersion string) (watch.Interface, error) {
-	//return lw.client.Client.Builds(kapi.NamespaceAll).Watch(labels.Everything(), fields.Everything(), resourceVersion)
 	return lw.Client.Builds(kapi.NamespaceAll).Watch(labels.Everything(), fields.Everything(), resourceVersion)
 }
 
@@ -454,12 +494,17 @@ func (lw *buildPodDeleteLW) List() (runtime.Object, error) {
 			continue
 		}
 		pod, err := lw.KubeClient.Pods(build.Namespace).Get(buildutil.GetBuildPodName(&build))
-		if err != nil && !kerrors.IsNotFound(err) {
-			glog.V(4).Infof("Error getting pod for build %s/%s: %v", build.Namespace, build.Name, err)
-			return nil, err
-		}
-		if (err != nil && kerrors.IsNotFound(err)) || pod.Labels[buildapi.BuildLabel] != build.Name {
-			pod = nil
+		if err != nil {
+			if !kerrors.IsNotFound(err) {
+				glog.V(4).Infof("Error getting pod for build %s/%s: %v", build.Namespace, build.Name, err)
+				return nil, err
+			} else {
+				pod = nil
+			}
+		} else {
+			if buildName, _ := buildutil.GetBuildLabel(pod); buildName != build.Name {
+				pod = nil
+			}
 		}
 		if pod == nil {
 			deletedPod := &kapi.Pod{
@@ -482,7 +527,11 @@ func (lw *buildPodDeleteLW) List() (runtime.Object, error) {
 
 // Watch watches all Pods that have a build label, for deletion
 func (lw *buildPodDeleteLW) Watch(resourceVersion string) (watch.Interface, error) {
-	sel, _ := labels.Parse(buildapi.BuildLabel)
+	// FIXME: since we cannot have OR on label name we'll just get builds with new label
+	sel, err := labels.Parse(buildapi.BuildLabel)
+	if err != nil {
+		return nil, err
+	}
 	return lw.KubeClient.Pods(kapi.NamespaceAll).Watch(sel, fields.Everything(), resourceVersion)
 }
 

pkg/build/controller/strategy/custom_test.go

@@ -35,12 +35,7 @@ func TestCustomCreateBuildPod(t *testing.T) {
 	if expected, actual := buildutil.GetBuildPodName(expected), actual.ObjectMeta.Name; expected != actual {
 		t.Errorf("Expected %s, but got %s!", expected, actual)
 	}
-	expectedLabels := make(map[string]string)
-	for k, v := range expected.Labels {
-		expectedLabels[k] = v
-	}
-	expectedLabels[buildapi.BuildLabel] = expected.Name
-	if !reflect.DeepEqual(expectedLabels, actual.Labels) {
+	if !reflect.DeepEqual(map[string]string{buildapi.BuildLabel: expected.Name}, actual.Labels) {
 		t.Errorf("Pod Labels does not match Build Labels!")
 	}
 	container := actual.Spec.Containers[0]

pkg/build/controller/strategy/docker_test.go

@@ -27,12 +27,7 @@ func TestDockerCreateBuildPod(t *testing.T) {
 	if expected, actual := buildutil.GetBuildPodName(expected), actual.ObjectMeta.Name; expected != actual {
 		t.Errorf("Expected %s, but got %s!", expected, actual)
 	}
-	expectedLabels := make(map[string]string)
-	for k, v := range expected.Labels {
-		expectedLabels[k] = v
-	}
-	expectedLabels[buildapi.BuildLabel] = expected.Name
-	if !reflect.DeepEqual(expectedLabels, actual.Labels) {
+	if !reflect.DeepEqual(map[string]string{buildapi.BuildLabel: expected.Name}, actual.Labels) {
 		t.Errorf("Pod Labels does not match Build Labels!")
 	}
 	container := actual.Spec.Containers[0]

pkg/build/controller/strategy/sti_test.go

@@ -34,12 +34,7 @@ func TestSTICreateBuildPod(t *testing.T) {
 	if expected, actual := buildutil.GetBuildPodName(expected), actual.ObjectMeta.Name; expected != actual {
 		t.Errorf("Expected %s, but got %s!", expected, actual)
 	}
-	expectedLabels := make(map[string]string)
-	for k, v := range expected.Labels {
-		expectedLabels[k] = v
-	}
-	expectedLabels[buildapi.BuildLabel] = expected.Name
-	if !reflect.DeepEqual(expectedLabels, actual.Labels) {
+	if !reflect.DeepEqual(map[string]string{buildapi.BuildLabel: expected.Name}, actual.Labels) {
 		t.Errorf("Pod Labels does not match Build Labels!")
 	}
 	container := actual.Spec.Containers[0]

pkg/build/controller/strategy/util.go

@@ -184,12 +184,7 @@ func getContainerVerbosity(containerEnv []kapi.EnvVar) (verbosity string) {
 	return
 }
 
-// getPodLabels copies build labels and adds additional one with build name itself
+// getPodLabels creates labels for the Build Pod
 func getPodLabels(build *buildapi.Build) map[string]string {
-	podLabels := make(map[string]string)
-	for k, v := range build.Labels {
-		podLabels[k] = v
-	}
-	podLabels[buildapi.BuildLabel] = build.Name
-	return podLabels
+	return map[string]string{buildapi.BuildLabel: build.Name}
 }

pkg/build/util/util.go

@@ -65,3 +65,13 @@ func NameFromImageStream(namespace string, ref *kapi.ObjectReference, tag string
 func IsBuildComplete(build *buildapi.Build) bool {
 	return build.Status.Phase != buildapi.BuildPhaseRunning && build.Status.Phase != buildapi.BuildPhasePending && build.Status.Phase != buildapi.BuildPhaseNew
 }
+
+// GetBuildLabel retrieves build label from a Pod returning its value and
+// a boolean value informing whether the value was found
+func GetBuildLabel(pod *kapi.Pod) (string, bool) {
+	value, exists := pod.Labels[buildapi.BuildLabel]
+	if !exists {
+		value, exists = pod.Labels[buildapi.DeprecatedBuildLabel]
+	}
+	return value, exists
+}