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.

Sign up for GitHub

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

Jump to bottom

kubelet: record an event with a clear reason on host port conflict #4915

Merged
merged 1 commit into from
Feb 28, 2015
Merged

kubelet: record an event with a clear reason on host port conflict #4915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions pkg/kubelet/kubelet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,15 +1378,35 @@ func updateBoundPods(changed []api.BoundPod, current []api.BoundPod) []api.Bound
return updated
}

type podsByCreationTime []api.BoundPod

func (s podsByCreationTime) Len() int {
return len(s)
}

func (s podsByCreationTime) Swap(i, j int) {
s[i], s[j] = s[j], s[i]
}

func (s podsByCreationTime) Less(i, j int) bool {
return s[i].CreationTimestamp.Before(s[j].CreationTimestamp)
}

// filterHostPortConflicts removes pods that conflict on Port.HostPort values
func filterHostPortConflicts(pods []api.BoundPod) []api.BoundPod {
filtered := []api.BoundPod{}
ports := map[int]bool{}
extract := func(p *api.Port) int { return p.HostPort }

// Respect the pod creation order when resolving conflicts.
sort.Sort(podsByCreationTime(pods))

for i := range pods {
pod := &pods[i]
if errs := validation.AccumulateUniquePorts(pod.Spec.Containers, ports, extract); len(errs) != 0 {
glog.Warningf("Pod %q: HostPort is already allocated, ignoring: %v", GetPodFullName(pod), errs)
record.Eventf(pod, "hostPortConflict", "Cannot start the pod due to host port conflict.")
Copy link
Member

Choose a reason for hiding this comment

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

Sending events is definitely necessary, but should we also reject such pod request somewhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we not reject today?

Copy link
Member

Choose a reason for hiding this comment

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

We are not reject this today because we believe we should't run into this issue except someone is going to use cadvisor's port (which we talked about to make it publicly registered); otherwise apiserver will reject it. But current discussion is that we plan to remove that check in apiserver, only have this one in kubelet. In this case, reject request is important now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@yujuhong has a TODO which should be okay for now. Can the Kubelet reject workload from the master? I thought the answer to that was no (or not yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I need some clarification on "rejections", that's why I left a TODO in the code :)

The only way to reject that I am aware of is to set the pod status to fail. Given that kubelet still doesn't post pod status to the apiserver yet, we'd need to save this information somewhere and wait until next round of pod status polling (GetPodStatus()) to return it.

Am I missing something here?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, my earlier comment missed TODO there.

// TODO: Set the pod status to fail.
continue
}
filtered = append(filtered, *pod)
Expand Down
35 changes: 35 additions & 0 deletions pkg/kubelet/kubelet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3086,3 +3086,38 @@ func TestPortForward(t *testing.T) {
t.Fatalf("stream: expected %v, got %v", e, a)
}
}

// Tests that upon host port conflict, the newer pod is removed.
func TestFilterHostPortConflicts(t *testing.T) {
// Reuse the pod spec with the same port to create a conflict.
spec := api.PodSpec{Containers: []api.Container{{Ports: []api.Port{{HostPort: 80}}}}}
var pods = []api.BoundPod{
{
ObjectMeta: api.ObjectMeta{
UID: "123456789",
Name: "newpod",
Namespace: "foo",
},
Spec: spec,
},
{
ObjectMeta: api.ObjectMeta{
UID: "987654321",
Name: "oldpod",
Namespace: "foo",
},
Spec: spec,
},
}
// Make sure the BoundPods are in the reverse order of creation time.
pods[1].CreationTimestamp = util.NewTime(time.Now())
pods[0].CreationTimestamp = util.NewTime(time.Now().Add(1 * time.Second))
filteredPods := filterHostPortConflicts(pods)
if len(filteredPods) != 1 {
t.Fatalf("Expected one pod. Got pods %#v", filteredPods)
}
if filteredPods[0].Name != "oldpod" {
t.Fatalf("Expected pod %#v. Got pod %#v", pods[1], filteredPods[0])
}

}
5 changes: 5 additions & 0 deletions pkg/util/time.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,11 @@ func Now() Time {
return Time{time.Now()}
}

// Before reports whether the time instant t is before u.
func (t Time) Before(u Time) bool {
return t.Time.Before(u.Time)
}

// Unix returns the local time corresponding to the given Unix time
// by wrapping time.Unix.
func Unix(sec int64, nsec int64) Time {
Expand Down