From 2b46aea4956e70b20e5f305eca6735447f2093be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Ole=C5=9B?= Date: Mon, 6 Jun 2016 12:30:56 +0200 Subject: [PATCH] Custom sort function for InitContainersStatuses Order in init container statuses should be the same as defined in pod. Statues shoudln't be sorted by name. --- pkg/kubelet/kubelet.go | 6 ++- pkg/kubelet/status/manager.go | 8 ++-- pkg/kubelet/status/manager_test.go | 9 +++-- pkg/kubelet/types/types.go | 16 ++++++++ pkg/kubelet/types/types_test.go | 64 ++++++++++++++++++++++++++++++ 5 files changed, 95 insertions(+), 8 deletions(-) create mode 100644 pkg/kubelet/types/types_test.go diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 2e14419f971..37b758fc387 100644 --- a/pkg/kubelet/kubelet.go +++ b/pkg/kubelet/kubelet.go @@ -3830,7 +3830,11 @@ func (kl *Kubelet) convertToAPIContainerStatuses(pod *api.Pod, podStatus *kubeco // Sort the container statuses since clients of this interface expect the list // of containers in a pod has a deterministic order. - sort.Sort(kubetypes.SortedContainerStatuses(containerStatuses)) + if isInitContainer { + kubetypes.SortInitContainerStatuses(pod, containerStatuses) + } else { + sort.Sort(kubetypes.SortedContainerStatuses(containerStatuses)) + } return containerStatuses } diff --git a/pkg/kubelet/status/manager.go b/pkg/kubelet/status/manager.go index c9b5b1e4e2c..337058df676 100644 --- a/pkg/kubelet/status/manager.go +++ b/pkg/kubelet/status/manager.go @@ -306,7 +306,7 @@ func (m *manager) updateStatusInternal(pod *api.Pod, status api.PodStatus, force status.StartTime = &now } - normalizeStatus(&status) + normalizeStatus(pod, &status) // The intent here is to prevent concurrent updates to a pod's status from // clobbering each other so the phase of a pod progresses monotonically. if isCached && isStatusEqual(&cachedStatus.status, &status) && !forceUpdate { @@ -484,7 +484,7 @@ func (m *manager) needsReconcile(uid types.UID, status api.PodStatus) bool { if err != nil { return false } - normalizeStatus(&podStatus) + normalizeStatus(pod, &podStatus) if isStatusEqual(&podStatus, &status) { // If the status from the source is the same with the cached status, @@ -504,7 +504,7 @@ func (m *manager) needsReconcile(uid types.UID, status api.PodStatus) bool { // In fact, the best way to solve this is to do it on api side. However for now, we normalize the status locally in // kubelet temporarily. // TODO(random-liu): Remove timestamp related logic after apiserver supports nanosecond or makes it consistent. -func normalizeStatus(status *api.PodStatus) *api.PodStatus { +func normalizeStatus(pod *api.Pod, status *api.PodStatus) *api.PodStatus { normalizeTimeStamp := func(t *unversioned.Time) { *t = t.Rfc3339Copy() } @@ -543,7 +543,7 @@ func normalizeStatus(status *api.PodStatus) *api.PodStatus { normalizeContainerState(&cstatus.LastTerminationState) } // Sort the container statuses, so that the order won't affect the result of comparison - sort.Sort(kubetypes.SortedContainerStatuses(status.InitContainerStatuses)) + kubetypes.SortInitContainerStatuses(pod, status.InitContainerStatuses) return status } diff --git a/pkg/kubelet/status/manager_test.go b/pkg/kubelet/status/manager_test.go index a9875549928..e88de8bcfe7 100644 --- a/pkg/kubelet/status/manager_test.go +++ b/pkg/kubelet/status/manager_test.go @@ -452,6 +452,9 @@ func shuffle(statuses []api.ContainerStatus) []api.ContainerStatus { } func TestStatusEquality(t *testing.T) { + pod := api.Pod{ + Spec: api.PodSpec{}, + } containerStatus := []api.ContainerStatus{} for i := 0; i < 10; i++ { s := api.ContainerStatus{ @@ -466,8 +469,8 @@ func TestStatusEquality(t *testing.T) { oldPodStatus := api.PodStatus{ ContainerStatuses: shuffle(podStatus.ContainerStatuses), } - normalizeStatus(&oldPodStatus) - normalizeStatus(&podStatus) + normalizeStatus(&pod, &oldPodStatus) + normalizeStatus(&pod, &podStatus) if !isStatusEqual(&oldPodStatus, &podStatus) { t.Fatalf("Order of container statuses should not affect normalized equality.") } @@ -498,7 +501,7 @@ func TestStaticPodStatus(t *testing.T) { m.SetPodStatus(staticPod, status) retrievedStatus := expectPodStatus(t, m, staticPod) - normalizeStatus(&status) + normalizeStatus(staticPod, &status) assert.True(t, isStatusEqual(&status, &retrievedStatus), "Expected: %+v, Got: %+v", status, retrievedStatus) retrievedStatus, _ = m.GetPodStatus(mirrorPod.UID) assert.True(t, isStatusEqual(&status, &retrievedStatus), "Expected: %+v, Got: %+v", status, retrievedStatus) diff --git a/pkg/kubelet/types/types.go b/pkg/kubelet/types/types.go index 7776ee9e366..d9102a592f0 100644 --- a/pkg/kubelet/types/types.go +++ b/pkg/kubelet/types/types.go @@ -68,6 +68,22 @@ func (s SortedContainerStatuses) Less(i, j int) bool { return s[i].Name < s[j].Name } +// SortInitContainerStatuses ensures that statuses are in the order that their +// init container appears in the pod spec +func SortInitContainerStatuses(p *api.Pod, statuses []api.ContainerStatus) { + containers := p.Spec.InitContainers + current := 0 + for _, container := range containers { + for j := current; j < len(statuses); j++ { + if container.Name == statuses[j].Name { + statuses[current], statuses[j] = statuses[j], statuses[current] + current++ + break + } + } + } +} + // Reservation represents reserved resources for non-pod components. type Reservation struct { // System represents resources reserved for non-kubernetes components. diff --git a/pkg/kubelet/types/types_test.go b/pkg/kubelet/types/types_test.go new file mode 100644 index 00000000000..6b12e62534c --- /dev/null +++ b/pkg/kubelet/types/types_test.go @@ -0,0 +1,64 @@ +/* +Copyright 2016 The Kubernetes Authors All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package types + +import ( + "reflect" + "testing" + + "k8s.io/kubernetes/pkg/api" +) + +func TestSortInitContainerStatuses(t *testing.T) { + pod := api.Pod{ + Spec: api.PodSpec{}, + } + var cases = []struct { + containers []api.Container + statuses []api.ContainerStatus + sortedStatuses []api.ContainerStatus + }{ + { + containers: []api.Container{{Name: "first"}, {Name: "second"}, {Name: "third"}, {Name: "fourth"}}, + statuses: []api.ContainerStatus{{Name: "first"}, {Name: "second"}, {Name: "third"}, {Name: "fourth"}}, + sortedStatuses: []api.ContainerStatus{{Name: "first"}, {Name: "second"}, {Name: "third"}, {Name: "fourth"}}, + }, + { + containers: []api.Container{{Name: "first"}, {Name: "second"}, {Name: "third"}, {Name: "fourth"}}, + statuses: []api.ContainerStatus{{Name: "second"}, {Name: "first"}, {Name: "fourth"}, {Name: "third"}}, + sortedStatuses: []api.ContainerStatus{{Name: "first"}, {Name: "second"}, {Name: "third"}, {Name: "fourth"}}, + }, + { + containers: []api.Container{{Name: "first"}, {Name: "second"}, {Name: "third"}, {Name: "fourth"}}, + statuses: []api.ContainerStatus{{Name: "fourth"}, {Name: "first"}}, + sortedStatuses: []api.ContainerStatus{{Name: "first"}, {Name: "fourth"}}, + }, + { + containers: []api.Container{{Name: "first"}, {Name: "second"}, {Name: "third"}, {Name: "fourth"}}, + statuses: []api.ContainerStatus{{Name: "first"}, {Name: "third"}}, + sortedStatuses: []api.ContainerStatus{{Name: "first"}, {Name: "third"}}, + }, + } + for _, data := range cases { + pod.Spec.InitContainers = data.containers + SortInitContainerStatuses(&pod, data.statuses) + if !reflect.DeepEqual(data.statuses, data.sortedStatuses) { + t.Errorf("SortInitContainerStatuses result wrong:\nContainers order: %v\nExpected order: %v\nReturne order: %v", + data.containers, data.sortedStatuses, data.statuses) + } + } +}