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

Fix panic when choosing zone or zones for volume #67745

Merged
merged 1 commit into from
Aug 29, 2018
Merged

Fix panic when choosing zone or zones for volume #67745

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
12 changes: 11 additions & 1 deletion pkg/volume/util/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,11 @@ func GetPath(mounter volume.Mounter) (string, error) {
// This means that a StatefulSet's volumes (`claimname-statefulsetname-id`) will spread across available zones,
// assuming the id values are consecutive.
func ChooseZoneForVolume(zones sets.String, pvcName string) string {
Copy link
Member

Choose a reason for hiding this comment

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

Should we migrate azure disks to the new SelectZoneForVolume() util method and get rid of this ChooseZoneForVolume method?

Copy link
Member

Choose a reason for hiding this comment

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

Good point. It looks like Azure Disks is already using SelectZoneForVolume. However when this patch was submitted SelectZoneForVolume was evolving and was using ChooseZoneForVolume (which is no longer the case). So only the fix further down for ChooseZonesForVolume is necessary for the Azure Disks scenario. Cinder volume does appear to still have a dependency on ChooseZonesForVolume which we can evaluate and refactor to get rid of ChooseZoneForVolume.

Copy link
Member

Choose a reason for hiding this comment

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

Don't we also have a SelectZonesForVolume too?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. SelectZoneForVolume is just a wrapper around SelectZonesForVolume for single zone volumes today. EBS, Azure and non regional PD invokes SelectZoneForVolume

Copy link
Member Author

Choose a reason for hiding this comment

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

@msau42 @ddebroy Thanks. Let me address this in a separate PR.

// No zones available, return empty string.
if zones.Len() == 0 {
return ""
}

// We create the volume in a zone determined by the name
// Eventually the scheduler will coordinate placement into an available zone
hash, index := getPVCNameHashAndIndexOffset(pvcName)
Expand All @@ -541,6 +546,12 @@ func ChooseZoneForVolume(zones sets.String, pvcName string) string {

// ChooseZonesForVolume is identical to ChooseZoneForVolume, but selects a multiple zones, for multi-zone disks.
func ChooseZonesForVolume(zones sets.String, pvcName string, numZones uint32) sets.String {
// No zones available, return empty set.
replicaZones := sets.NewString()
if zones.Len() == 0 {
return replicaZones
}

// We create the volume in a zone determined by the name
// Eventually the scheduler will coordinate placement into an available zone
hash, index := getPVCNameHashAndIndexOffset(pvcName)
Expand All @@ -554,7 +565,6 @@ func ChooseZonesForVolume(zones sets.String, pvcName string, numZones uint32) se
// PVC placement (which could also e.g. avoid putting volumes in overloaded or
// unhealthy zones)
zoneSlice := zones.List()
replicaZones := sets.NewString()

startingIndex := index * numZones
for index = startingIndex; index < startingIndex+numZones; index++ {
Expand Down
22 changes: 22 additions & 0 deletions pkg/volume/util/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,17 @@ func TestChooseZoneForVolume(t *testing.T) {
VolumeName: "medium-henley--4",
Expected: "c", // hash("") + 4 == 2 mod 3
},
// Test for no zones
{
Zones: sets.NewString(),
VolumeName: "medium-henley--1",
Expected: "",
},
{
Zones: nil,
VolumeName: "medium-henley--2",
Expected: "",
},
}

for _, test := range tests {
Expand Down Expand Up @@ -992,6 +1003,17 @@ func TestChooseZonesForVolume(t *testing.T) {
NumZones: 3,
Expected: sets.NewString("a" /* hash("henley") == 0 + 3 + 6(startingIndex) */, "b", "c"),
},
// Test for no zones
{
Zones: sets.NewString(),
VolumeName: "henley-1",
Expected: sets.NewString(),
},
{
Zones: nil,
VolumeName: "henley-2",
Expected: sets.NewString(),
},
}

for _, test := range tests {
Expand Down