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

support Azure data disk volume #29836

Merged
merged 1 commit into from Aug 24, 2016

Conversation

Projects
None yet
10 participants
@rootfs
Member

rootfs commented Jul 31, 2016

This is a WIP of supporting azure data disk volume. Will add test and dynamic provisioning support once #29006 is merged

replace #25915
fix #23259

@kubernetes/sig-storage
@colemickens @brendandburns


This change is Reviewable

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 1, 2016

@thockin feel free to remove yourself if you want (though feel free to review if you want too ;)

FSType string `json:"fsType,omitempty" protobuf:"bytes,4,opt,name=fsType"`
// Defaults to false (read/write). ReadOnly here will force
// the ReadOnly setting in VolumeMounts.
ReadOnly bool `json:"readOnly,omitempty" protobuf:"varint,5,opt,name=readOnly"`

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

shouldn't this be a protobuf boolean, not a varint?

This comment has been minimized.

@rootfs

rootfs Aug 8, 2016

Member

seems all bool use varint

}
disks := *vm.Properties.StorageProfile.DataDisks
for _, disk := range disks {
if disk.Lun != nil {

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

join these conditions together, there's no need for multiple if statements. You can use line breaks if you want clarity about what does what.

This comment has been minimized.

@brendandburns

brendandburns Aug 9, 2016

Contributor

please address this comment.

)
// serialize concurrent attach
var attachMutex = keymutex.NewKeyMutex()

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

move this into the azureDiskAttacher struct.

This comment has been minimized.

@rootfs

rootfs Aug 8, 2016

Member

this lock is used in both attach and detach to serialize disk attach/detach. If multiple attach/detach occur simultaneously, some will get failure response from azure portal.

func (plugin *azureDataDiskPlugin) NewAttacher() (volume.Attacher, error) {
azure, err := getAzureDiskManager(plugin.host.GetCloudProvider())
if err != nil {
glog.Infof("failed to get azure provider")

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

delete.

func (attacher *azureDiskAttacher) Attach(spec *volume.Spec, hostName string) (string, error) {
volumeSource, err := getVolumeSource(spec)
if err != nil {
glog.Infof("failed to get azure disk spec")

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

delete (and all other similar glog.Infof on errors) returning an error should be sufficient, logging additionally causes spam.

} else {
lun, err = attacher.manager.GetNextDiskLun(instanceid)
if err != nil {
glog.Warningf("no LUN available for instance %q", instanceid)

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

delete

scsiHostRescan(&osIOHandler{})
devicePath := findDiskByLun(lun, &osIOHandler{})
if devicePath != "" {
glog.Infof("Successfully found attached Azure disk %q(lun %s, device path %s).", volumeSource.DiskName, lunStr, devicePath)

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

spammy, move to V(4) or delete.

err = attacher.manager.AttachDisk(volumeSource.DiskName, volumeSource.DataDiskURI, instanceid, lun, compute.CachingTypes(volumeSource.CachingMode))
if err == nil {
glog.Infof("Attach operation successful: volume %q attached to node %q.", volumeSource.DataDiskURI, instanceid)

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

delete or move to V(4)

if err != nil {
return "", fmt.Errorf("WaitForAttach: wrong lun %q", lunStr)
}
ticker := time.NewTicker(checkSleepDuration)

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

personally, I would use time.After instead of timers for simpler code.

e.g.

select {
  case time.After(foo):
...
}
}
func (attacher *azureDiskAttacher) GetDeviceMountPath(
spec *volume.Spec) (string, error) {

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

move this up to the preceeding line.

}
return makeGlobalPDPath(attacher.host, volumeSource.DiskName), nil

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

delete empty line.

if ind := strings.LastIndex(instanceid, "/"); ind >= 0 {
instanceid = instanceid[(ind + 1):]
}
attachMutex.LockKey(instanceid)

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

ugh, this is ugly. Please turn this into a static method rather than a global variable.

This comment has been minimized.

@jingxu97

jingxu97 Aug 8, 2016

Contributor

I think the lock here is no longer needed. Since v1.3.3, attach/unattach operatinos are invoked through operation_executor (k8s.io/kubernetes/pkg/volume/util/operationexecutor) which handles syntonization problem among different operations on the same volume.

This comment has been minimized.

@rootfs

rootfs Aug 8, 2016

Member

@jingxu97 Azure seems to allow only one attach or detach at a time per instance. This is what the lock for.

attachMutex.LockKey(instanceid)
defer attachMutex.UnlockKey(instanceid)
glog.Infof("detach %v from host %q", dev, instanceid)

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

Spammy V(4) or delete.

}
func (detacher *azureDiskDetacher) WaitForDetach(devicePath string, timeout time.Duration) error {
ticker := time.NewTicker(checkSleepDuration)

This comment has been minimized.

@brendandburns

brendandburns Aug 1, 2016

Contributor

as above wrt to time.After

Also, given that you've dupliucated this code, perhaps extract a helper method?

(in fact, isn't this what util.Poll already does?)

This comment has been minimized.

@brendandburns

brendandburns Aug 9, 2016

Contributor

Address this comment please.

This comment has been minimized.

@rootfs

rootfs Aug 22, 2016

Member

fixed

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 1, 2016

Did a quick pass. This needs unit tests where possible, and also an e2e test that is marked for running on Azure.

Thanks!

@rootfs

This comment has been minimized.

Member

rootfs commented Aug 1, 2016

@brendandburns thanks for the quick review. Unit and e2e tests will be added soon.

// AzureDisk represents an Azure Data Disk mount on the host and bind mount to the pod.
type AzureDiskVolumeSource struct {
// Data Disk Name

This comment has been minimized.

@thockin

thockin Aug 2, 2016

Member

Comment should explain what the field holds. This just restates the name. can you explain more?

// AzureDisk represents an Azure Data Disk mount on the host and bind mount to the pod.
type AzureDiskVolumeSource struct {
// Data Disk Name
DiskName string `json:"diskName"`

This comment has been minimized.

@thockin

thockin Aug 2, 2016

Member

Required? it's not omitempty, so I assume it is required.

// Data Disk Name
DiskName string `json:"diskName"`
// Data Disk URI
DataDiskURI string `json:"diskURI"`

This comment has been minimized.

@thockin

thockin Aug 2, 2016

Member

Required? it's not omitempty, so I assume it is required.

This comment has been minimized.

@rootfs

rootfs Aug 8, 2016

Member

it is required

// Data Disk URI
DataDiskURI string `json:"diskURI"`
// Host Caching mode: None, Read Only, Read Write.
CachingMode AzureDataDiskCachingMode `json:"cachingMode,omitempty"`

This comment has been minimized.

@thockin

thockin Aug 2, 2016

Member

Spec what happens if this is not specified.

Since this is omitempty, it is optional, so it should be a pointer.

// Filesystem type to mount.
// Must be a filesystem type supported by the host operating system.
// Ex. "ext4", "xfs", "ntfs". Implicitly inferred to be "ext4" if unspecified.
FSType string `json:"fsType,omitempty"`

This comment has been minimized.

@thockin

thockin Aug 2, 2016

Member

should be a pointer if it is optional. Yes, I know others are not done this way, but it's a convention that came around after we started.

FSType string `json:"fsType,omitempty"`
// Defaults to false (read/write). ReadOnly here will force
// the ReadOnly setting in VolumeMounts.
ReadOnly bool `json:"readOnly,omitempty"`

This comment has been minimized.

@thockin

thockin Aug 2, 2016

Member

pointer, I think. I have asked for clarification of bool fields.

vm, exists, err := az.getVirtualMachine(vmName)
if err != nil || !exists {
// if host doesn't exist, no need to detach
glog.Errorf("cannot find node %s", vmName)

This comment has been minimized.

@saad-ali

saad-ali Aug 22, 2016

Member

Make this a warning and make it clear that detach is being skipped because of it.

}
instanceid, err := detacher.azureProvider.InstanceID(hostName)
if err != nil {
glog.V(2).Infof("no instance id for host %q, skip detaching", hostName)

This comment has been minimized.

@saad-ali

saad-ali Aug 22, 2016

Member

Should be a Warningf. Also, does err != nil always indicate that the node has been deleted here?

if err := util.UnmountPath(deviceMountPath, detacher.mounter); err != nil {
glog.Errorf("Error unmounting %q: %v", volume, err)
}
return nil

This comment has been minimized.

@saad-ali

saad-ali Aug 22, 2016

Member

Why are unmount errors swallowed?

@saad-ali

This comment has been minimized.

Member

saad-ali commented Aug 22, 2016

@saad-ali I'd like to get this in prior to code freeze. so I'm LGTM-ing. If there are further concerns let's take care of them as follow on PRs.

Ack. Will create an tracking issue for follow-up.

#31186

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 22, 2016

Thanks @saad-ali I will work to get those comments addressed asap.

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 22, 2016

Reviewed 1 of 27 files at r3, 4 of 6 files at r4, 2 of 25 files at r7, 2 of 23 files at r8, 22 of 26 files at r9, 6 of 6 files at r10.
Review status: all files reviewed at latest revision, 20 unresolved discussions.


Comments from Reviewable

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 22, 2016

Review status: all files reviewed at latest revision, 20 unresolved discussions.


pkg/volume/azure_dd/attacher.go, line 268 [r7] (raw file):

Previously, rootfs (Huamin Chen) wrote…

done

LGTM

Comments from Reviewable

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 22, 2016

Review status: all files reviewed at latest revision, 20 unresolved discussions.


pkg/api/types.go, line 759 [r1] (raw file):

Previously, thockin (Tim Hockin) wrote…

Comment should explain what the field holds. This just restates the name. can you explain more?

LGTM

pkg/api/types.go, line 760 [r1] (raw file):

Previously, thockin (Tim Hockin) wrote…

Required? it's not omitempty, so I assume it is required.

LGTM

pkg/api/types.go, line 762 [r1] (raw file):

Previously, rootfs (Huamin Chen) wrote…

it is required

LGTM

Comments from Reviewable

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 23, 2016

@rootfs PR needs rebase

support Azure data disk volume
Signed-off-by: Huamin Chen <hchen@redhat.com>
@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 23, 2016

/lgtm cancel //PR changed after LGTM, removing LGTM. @brendandburns @jingxu97 @rootfs @saad-ali

@k8s-bot

This comment has been minimized.

k8s-bot commented Aug 23, 2016

GCE e2e build/test passed for commit dea4b02.

@rootfs

This comment has been minimized.

Member

rootfs commented Aug 23, 2016

@brendandburns @saad-ali PR rebased. Can you tag it again? Thanks!

@brendandburns brendandburns added the lgtm label Aug 23, 2016

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 23, 2016

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

k8s-bot commented Aug 23, 2016

GCE e2e build/test passed for commit dea4b02.

@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Aug 24, 2016

@k8s-bot node e2e test this issue: #31327

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 24, 2016

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

k8s-bot commented Aug 24, 2016

GCE e2e build/test passed for commit dea4b02.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Aug 24, 2016

Automatic merge from submit-queue

@k8s-merge-robot k8s-merge-robot merged commit 3544f8a into kubernetes:master Aug 24, 2016

6 of 8 checks passed

Submit Queue Github CI tests are not green.
Details
code-review/reviewable 21 files, 20 discussions left (brendandburns, colemickens, jingxu97, saad-ali, thockin)
Details
Jenkins GCE Node e2e Build finished. 626 tests run, 140 skipped, 0 failed.
Details
Jenkins GCE e2e Build finished. 374 tests run, 170 skipped, 0 failed.
Details
Jenkins GKE smoke e2e Build finished. 374 tests run, 371 skipped, 0 failed.
Details
Jenkins unit/integration Build finished. 3915 tests run, 24 skipped, 0 failed.
Details
Jenkins verification Build finished.
Details
cla/google All necessary CLAs are signed

@saad-ali saad-ali referenced this pull request Aug 25, 2016

Closed

New Volume Plugin: Azure Data Disk #79

4 of 22 tasks complete
@rootfs

This comment has been minimized.

Member

rootfs commented Sep 1, 2016

@saad-ali followup PR on device path issue
#31366

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment