diff --git a/UPGRADE.md b/UPGRADE.md new file mode 100644 index 000000000000..565497dfb9c1 --- /dev/null +++ b/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. diff --git a/pkg/build/api/types.go b/pkg/build/api/types.go index 2c7d7c0c6e22..0c8492cbd615 100644 --- a/pkg/build/api/types.go +++ b/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 diff --git a/pkg/build/api/v1/types.go b/pkg/build/api/v1/types.go index 406897c45c90..3f8d95e2ad40 100644 --- a/pkg/build/api/v1/types.go +++ b/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 { diff --git a/pkg/build/api/v1beta3/types.go b/pkg/build/api/v1beta3/types.go index 9c184c769373..ba2d1531fcdc 100644 --- a/pkg/build/api/v1beta3/types.go +++ b/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 { diff --git a/pkg/build/controller/controller.go b/pkg/build/controller/controller.go index eebcdbdb89b5..655a708c0d12 100644 --- a/pkg/build/controller/controller.go +++ b/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) diff --git a/pkg/build/controller/controller_test.go b/pkg/build/controller/controller_test.go index fa1f091c13e1..20fb4c5fd4df 100644 --- a/pkg/build/controller/controller_test.go +++ b/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") diff --git a/pkg/build/controller/factory/factory.go b/pkg/build/controller/factory/factory.go index b1f84e11fa97..4b56816e9c9b 100644 --- a/pkg/build/controller/factory/factory.go +++ b/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) } diff --git a/pkg/build/controller/strategy/custom_test.go b/pkg/build/controller/strategy/custom_test.go index 5aa3d16f7f57..9daf3ee8da22 100644 --- a/pkg/build/controller/strategy/custom_test.go +++ b/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] diff --git a/pkg/build/controller/strategy/docker_test.go b/pkg/build/controller/strategy/docker_test.go index e33ace7d9887..4077a1a97e88 100644 --- a/pkg/build/controller/strategy/docker_test.go +++ b/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] diff --git a/pkg/build/controller/strategy/sti_test.go b/pkg/build/controller/strategy/sti_test.go index d11c7bb85998..5a7c995b37e9 100644 --- a/pkg/build/controller/strategy/sti_test.go +++ b/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] diff --git a/pkg/build/controller/strategy/util.go b/pkg/build/controller/strategy/util.go index a0e160d6148c..92d09845b04d 100644 --- a/pkg/build/controller/strategy/util.go +++ b/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} } diff --git a/pkg/build/util/util.go b/pkg/build/util/util.go index b835745686ae..b5d4d2831622 100644 --- a/pkg/build/util/util.go +++ b/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 +} diff --git a/pkg/build/util/util_test.go b/pkg/build/util/util_test.go index e52f1ceb36cc..441702a7e3af 100644 --- a/pkg/build/util/util_test.go +++ b/pkg/build/util/util_test.go @@ -12,3 +12,41 @@ func TestGetBuildPodName(t *testing.T) { t.Errorf("Expected %s, got %s", expected, actual) } } + +func TestGetBuildLabel(t *testing.T) { + type getBuildLabelTest struct { + labels map[string]string + expectedValue string + expectedExists bool + } + + tests := []getBuildLabelTest{ + { + // 0 - new label + labels: map[string]string{buildapi.BuildLabel: "value"}, + expectedValue: "value", + expectedExists: true, + }, + { + // 1 - deprecated label + labels: map[string]string{buildapi.DeprecatedBuildLabel: "value"}, + expectedValue: "value", + expectedExists: true, + }, + { + // 2 - deprecated label + labels: map[string]string{}, + expectedValue: "", + expectedExists: false, + }, + } + for i, tc := range tests { + value, exists := GetBuildLabel(&kapi.Pod{ObjectMeta: kapi.ObjectMeta{Labels: tc.labels}}) + if value != tc.expectedValue { + t.Errorf("(%d) unexpected value, expected %s, got %s", i, tc.expectedValue, value) + } + if exists != tc.expectedExists { + t.Errorf("(%d) unexpected exists flag, expected %v, got %v", i, tc.expectedExists, exists) + } + } +}