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

Counting pod volume towards PV limit even if PV/PVC is missing #27227

Merged
merged 2 commits into from
Jun 17, 2016
Merged

Counting pod volume towards PV limit even if PV/PVC is missing #27227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07bc06b
Counting pod volume towards PV limit even if PV/PVC is missing
Jun 11, 2016
20ce8b7
Improving error messages and naming to be clear
Jun 13, 2016
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
21 changes: 19 additions & 2 deletions plugin/pkg/scheduler/algorithm/predicates/predicates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,17 @@ package predicates

import (
"fmt"
"math/rand"
"strconv"
"time"

"github.com/golang/glog"
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/client/cache"
qosutil "k8s.io/kubernetes/pkg/kubelet/qos/util"
"k8s.io/kubernetes/pkg/labels"
utilruntime "k8s.io/kubernetes/pkg/util/runtime"
"k8s.io/kubernetes/plugin/pkg/scheduler/algorithm"
priorityutil "k8s.io/kubernetes/plugin/pkg/scheduler/algorithm/priorities/util"
"k8s.io/kubernetes/plugin/pkg/scheduler/schedulercache"
Expand Down Expand Up @@ -156,7 +160,13 @@ func (c *MaxPDVolumeCountChecker) filterVolumes(volumes []api.Volume, namespace
}
pvc, err := c.pvcInfo.GetPersistentVolumeClaimInfo(namespace, pvcName)
if err != nil {
return err
// if the PVC is not found, log the error and count the PV towards the PV limit
// generate a random volume ID since its required for de-dup
utilruntime.HandleError(fmt.Errorf("Unable to look up PVC info for %s/%s, assuming PVC matches predicate when counting limits: %v", namespace, pvcName, err))
source := rand.NewSource(time.Now().UnixNano())
generatedID := "missingPVC" + strconv.Itoa(rand.New(source).Intn(1000000))
filteredVolumes[generatedID] = true
return nil
}

pvName := pvc.Spec.VolumeName
Expand All @@ -166,7 +176,14 @@ func (c *MaxPDVolumeCountChecker) filterVolumes(volumes []api.Volume, namespace

pv, err := c.pvInfo.GetPersistentVolumeInfo(pvName)
if err != nil {
return err
// if the PV is not found, log the error
// and count the PV towards the PV limit
// generate a random volume ID since its required for de-dup
utilruntime.HandleError(fmt.Errorf("Unable to look up PV info for %s/%s/%s, assuming PV matches predicate when counting limits: %v", namespace, pvcName, pvName, err))
source := rand.NewSource(time.Now().UnixNano())
generatedID := "missingPV" + strconv.Itoa(rand.New(source).Intn(1000000))
filteredVolumes[generatedID] = true
return nil
}

if id, ok := c.filter.FilterPersistentVolume(pv); ok {
Expand Down
60 changes: 59 additions & 1 deletion plugin/pkg/scheduler/algorithm/predicates/predicates_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1392,6 +1392,32 @@ func TestEBSVolumeCountConflicts(t *testing.T) {
},
},
}
deletedPVCPod := &api.Pod{
Copy link
Member

Choose a reason for hiding this comment

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

I was confused about the test. Both deletedPVCPod and deletedPVPod seem to be basically the same (pod with a volume with a volume source with a PVC with a name that doesn't resolve), so how are you testing both the

c.pvcInfo.GetPersistentVolumeClaimInfo(namespace, pvcName) != nil
and
c.pvInfo.GetPersistentVolumeInfo(pvName) != nil
paths?

Copy link
Author

@abhgupta abhgupta Jun 11, 2016 •
edited

Choose a reason for hiding this comment

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

The pod deletedPVCPod refers to a PVC that is missing and exercises the condition below.
c.pvcInfo.GetPersistentVolumeClaimInfo(namespace, pvcName) != nil

The pod deletedPVPod refers to a PVC that exists but references a PV that is missing and hence exercises the condition below.
c.pvInfo.GetPersistentVolumeInfo(pvName) != nil

I have added test cases/scenarios to verify that having existing pods with missing PV or missing PVC result in these PVs being counted towards the node limit. I have verified the that the test cases execute both code paths by manually modifying the predicate to return error vs nil for both these conditions - this caused the valid test cases to fail, thereby confirming the code path execution.

Copy link
Contributor

Choose a reason for hiding this comment

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

if, for some reason, you end up doing another revision, it might make sense to rename "deletedPV" to "pvcWithDeletedPV" (or something similar), which I think would make it a bit clearer.

Copy link
Member

Choose a reason for hiding this comment

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

if, for some reason, you end up doing another revision, it might make sense to rename "deletedPV" to "pvcWithDeletedPV" (or something similar), which I think would make it a bit clearer.

+10

Spec: api.PodSpec{
Volumes: []api.Volume{
{
VolumeSource: api.VolumeSource{
PersistentVolumeClaim: &api.PersistentVolumeClaimVolumeSource{
ClaimName: "deletedPVC",
},
},
},
},
},
}
deletedPVPod := &api.Pod{
Spec: api.PodSpec{
Volumes: []api.Volume{
{
VolumeSource: api.VolumeSource{
PersistentVolumeClaim: &api.PersistentVolumeClaimVolumeSource{
ClaimName: "pvcWithDeletedPV",
},
},
},
},
},
}
emptyPod := &api.Pod{
Spec: api.PodSpec{},
}
Expand Down Expand Up @@ -1466,14 +1492,42 @@ func TestEBSVolumeCountConflicts(t *testing.T) {
fits: true,
test: "the same EBS volumes are not counted multiple times",
},
{
newPod: ebsPVCPod,
existingPods: []*api.Pod{oneVolPod, deletedPVCPod},
maxVols: 2,
fits: false,
test: "pod with missing PVC is counted towards the PV limit",
},
{
newPod: ebsPVCPod,
existingPods: []*api.Pod{oneVolPod, deletedPVCPod},
maxVols: 3,
fits: true,
test: "pod with missing PVC is counted towards the PV limit",
},
{
newPod: ebsPVCPod,
existingPods: []*api.Pod{oneVolPod, deletedPVPod},
maxVols: 2,
fits: false,
test: "pod with missing PV is counted towards the PV limit",
},
{
newPod: ebsPVCPod,
existingPods: []*api.Pod{oneVolPod, deletedPVPod},
maxVols: 3,
fits: true,
test: "pod with missing PV is counted towards the PV limit",
},
}

pvInfo := FakePersistentVolumeInfo{
{
ObjectMeta: api.ObjectMeta{Name: "someEBSVol"},
Spec: api.PersistentVolumeSpec{
PersistentVolumeSource: api.PersistentVolumeSource{
AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{},
AWSElasticBlockStore: &api.AWSElasticBlockStoreVolumeSource{VolumeID: "ebsVol"},
},
},
},
Expand All @@ -1494,6 +1548,10 @@ func TestEBSVolumeCountConflicts(t *testing.T) {
ObjectMeta: api.ObjectMeta{Name: "someNonEBSVol"},
Spec: api.PersistentVolumeClaimSpec{VolumeName: "someNonEBSVol"},
},
{
ObjectMeta: api.ObjectMeta{Name: "pvcWithDeletedPV"},
Spec: api.PersistentVolumeClaimSpec{VolumeName: "pvcWithDeletedPV"},
},
}

filter := VolumeFilter{
Expand Down