Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom sort function for InitContainersStatuses #26829

Merged
merged 1 commit into from
Jun 8, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion pkg/kubelet/kubelet.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/kubelet/status/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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()
}
Expand Down Expand Up @@ -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
}

Expand Down
9 changes: 6 additions & 3 deletions pkg/kubelet/status/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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.")
}
Expand Down Expand Up @@ -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)
Expand Down
16 changes: 16 additions & 0 deletions pkg/kubelet/types/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -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++ {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hrm, if someone passes a statuses where container.Name is not in statuses, will this loop forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't see how. Could you provide test case for unit test?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, it looks fine. Just wanted to be extra sure.

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.
Expand Down
64 changes: 64 additions & 0 deletions pkg/kubelet/types/types_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}