Add sync state loop in master's volume reconciler #34859

jingxu97 · 2016-10-14T21:24:19Z

At master volume reconciler, the information about which volumes are
attached to nodes is cached in actual state of world. However, this
information might be out of date in case that node is terminated (volume
is detached automatically). In this situation, reconciler assume volume
is still attached and will not issue attach operation when node comes
back. Pods created on those nodes will fail to mount.

This PR adds the logic to periodically sync up the truth for attached
volumes kept in the actual state cache. If the volume is no longer attached
to the node, the actual state will be updated to reflect the truth. In turn,
reconciler will take actions if needed.

To avoid issuing many concurrent operations on cloud provider, this PR
tries to add batch operation to check whether a list of volumes are
attached to the node instead of one request per volume.

Add sync state loop in master's volume reconciler

Detailed design is in #33760

This change is

saad-ali · 2016-10-24T21:31:19Z

pkg/volume/util/operationexecutor/operation_executor.go

+		return err
+	}
+	// Give an empty UniqueVolumeName so that this operation could be executed concurrently.
+	return oe.pendingOperations.Run("", "", volumesAreAttachedFunc)


nit: To improve readability, put variable name in a comment when it is not obvious what the value is for:

return oe.pendingOperations.Run("" /* volumeName */, "" /* podName */, volumesAreAttachedFunc)

saad-ali · 2016-10-24T21:43:00Z

pkg/volume/util/operationexecutor/operation_executor.go

+	// Note that this operation could be operated concurrently with other attach/detach operations.
+	// In theory (but very unlikely in practise), race condition among these operations might mark volume as detached
+	// even if it is attached. But reconciler can correct this in a short period of time.
+	CheckVolumesAreAttached(AttachedVolumes []AttachedVolume, nodeName types.NodeName, actualStateOfWorld ActualStateOfWorldAttacherUpdater) error


nit: Check -> Verify

saad-ali · 2016-10-24T21:47:07Z

pkg/volume/util/operationexecutor/operation_executor.go

+		return err
+	}
+	// Give an empty UniqueVolumeName so that this operation could be executed concurrently.
+	return oe.pendingOperations.Run("", "", volumesAreAttachedFunc)


Since we do not care about pending operations for this function, how about we execute it serially inline? That way 1) we can avoid having to make changes to nestedpendingoperations. And 2) we won't start other (potentially racy) operations until this verification step is complete.

Since I am trying to use batch operation for checking volume attached or not, there is no single unique volume id for serialize the operation.

Right, and that's fine. So the suggestion is to therefore not use nestedpendingoperations at all.

I though about using gorutineMap? but then we need to put goroutineMap into operation_executor besides nestedpendingOperations, which is not needed if choosing to use nestedpendingoperations

What I'm trying to say is that since we are not trying to coordinate with other operations. We don't need to use either nestedpendingoperations gorutineMap or operation_executor. We can do all the things that the method returned by generateVolumesAreAttachedFunc does in thes same file as syncstates serially.

Ping on this comment

Spoke offline, ok to leave as is.

saad-ali · 2016-10-24T22:13:58Z

pkg/volume/util/operationexecutor/operation_executor.go

+			for spec, check := range attached {
+				if !check {
+					actualStateOfWorld.MarkVolumeAsDetached(volumeSpecMap[spec], nodeName)
+					glog.V(2).Infof("Volume %q is no longer attached, mark it as detached", volumeSpecMap[spec])


Let's make this V(1). And make the message more explicit: VerifyVolumesAreAttached determined volume %q (spec.Name: %q) is no longer attached to node %q, therefore it was marked as detached.

saad-ali · 2016-10-24T22:20:02Z

pkg/volume/util/operationexecutor/operation_executor.go

+	volumesPerPlugin := make(map[string][]*volume.Spec)
+	// volumeSpecMap maps from a volume spec to its unique volumeName which will be used
+	// when calling MarkVolumeAsDetached
+	volumeSpecMap := make(map[*volume.Spec]api.UniqueVolumeName)


You are using a pointer as a key in a map. This means that the map will compare memory address for equality--not the contents of volume.Spec. Is that what you want? Looks like you are doing this in multiple places.

Up to you how you want to handle it. I suggest passing around api.UniqueVolumeName as needed and using that as the key where possible.

Yes, I am try to compare whether they are the same object(volume.Spec), not the content. The functions implemented for Attacher interface are all using volume.Spec as parameters. So it is not very convenient here since operations for actual/desired states use volume id.

jingxu97 · 2016-10-24T23:01:40Z

@saad-ali PTAL

jingxu97 · 2016-10-25T16:22:24Z

@saad-ali PTAL

saad-ali · 2016-10-25T18:37:13Z

pkg/cloudprovider/providers/aws/aws.go

@@ -321,6 +321,9 @@ type Volumes interface {

 	// Check if the volume is already attached to the node with the specified NodeName
 	DiskIsAttached(diskName string, nodeName types.NodeName) (bool, error)
+
+	// Check if a list of volumes are attached to the node with the specified NodeName
+	DisksAreAttached(diskNames []string, nodeName types.NodeName) ([]bool, error)


I'm a little uncomfortable with this method returning an array where the value of the element corresponds to the value of the same index in the input array. I'd prefer the return value to be able to be understood (contain all necessary information) independent of the input. But that's just a queasy feeling, no hard fast rule--so it's fine if you want to leave it like this.

I changed to a map.

saad-ali · 2016-10-25T18:41:26Z

pkg/cloudprovider/providers/openstack/openstack_volumes.go

@@ -240,6 +240,21 @@ func (os *OpenStack) DiskIsAttached(diskName, instanceID string) (bool, error) {
 	return false, nil
 }

+// query if a list of volumes are attached to a compute instance
+func (os *OpenStack) DisksAreAttached(diskNames []string, instanceID string) ([]bool, error) {


@anguslees @dagnello Can you verify this logic for openstack. The purpose of this method is to verify whether the specified disks are still attached to the specified node. If this check determines the disk is no longer attached when it should be, the controller will try to reattach the volume.

@anguslees or @dagnello Can you take a look at the OpenStack changes by EOD?

saad-ali · 2016-10-25T18:45:11Z

pkg/cloudprovider/providers/rackspace/rackspace.go

@@ -660,3 +660,21 @@ func (rs *Rackspace) DiskIsAttached(diskName, instanceID string) (bool, error) {
 	}
 	return false, nil
 }
+
+// query if a list volumes are attached to a compute instance
+func (rs *Rackspace) DisksAreAttached(diskNames []string, instanceID string) ([]bool, error) {


@anguslees, @jamiehannaford, @jrperritt @mkulke Can you verify this logic for rackspace. The purpose of this method is to verify whether the specified disks are still attached to the specified node. If this check determines the disk is no longer attached when it should be, the controller will try to reattach the volume.

@anguslees, @jamiehannaford, @jrperritt @mkulke: can one of you verify the rackspace changes by EOD?

saad-ali · 2016-10-25T18:46:06Z

pkg/cloudprovider/providers/aws/aws.go

@@ -1774,6 +1777,38 @@ func (c *Cloud) DiskIsAttached(diskName string, nodeName types.NodeName) (bool,
 	return false, nil
 }

+func (c *Cloud) DisksAreAttached(diskNames []string, nodeName types.NodeName) ([]bool, error) {


@justinsb Can you take a look at the AWS bits here. The purpose of this method is to verify whether the specified disks are still attached to the specified node. If this check determines the disk is no longer attached when it should be, the controller will try to reattach the volume.

@justinsb Could you take a look at the AWS bits by EOD?

saad-ali · 2016-10-25T18:48:11Z

pkg/cloudprovider/providers/vsphere/vsphere.go

@@ -937,6 +941,61 @@ func (vs *VSphere) DiskIsAttached(volPath string, nodeName k8stypes.NodeName) (b
 	return attached, err
 }

+// DisksAreAttached returns if disks are attached to the VM using controllers supported by the plugin.
+func (vs *VSphere) DisksAreAttached(volPaths []string, nodeName k8stypes.NodeName) ([]bool, error) {


VSphere maintainers (@dagnello @abithap @imkin @abrarshivani @kerneltime @luomiao): Can one of you verify this logic for VSphere. The purpose of this method is to verify whether the specified disks are still attached to the specified node. If this check determines the disk is no longer attached, then the controller will try to reattach the volume.

LGTM. Thanks for doing this.

Thanks @abrarshivani

jingxu97 · 2016-10-25T21:02:33Z

@saad-ali PTAL

jingxu97 · 2016-10-25T21:54:46Z

But this way, syncStates will block reconcile function.

On Mon, Oct 24, 2016 at 4:11 PM, Saad Ali notifications@github.com wrote:

@saad-ali commented on this pull request.

In pkg/volume/util/operationexecutor/operation_executor.go
#34859:

@@ -397,6 +404,19 @@ func (oe operationExecutor) DetachVolume(
volumeToDetach.VolumeName, "" / podName */, detachFunc)
}

+func (oe *operationExecutor) CheckVolumesAreAttached(
attachedVolumes []AttachedVolume,

nodeName types.NodeName,

actualStateOfWorld ActualStateOfWorldAttacherUpdater) error {

volumesAreAttachedFunc, err :=
oe.generateVolumesAreAttachedFunc(attachedVolumes, nodeName, actualStateOfWorld)
if err != nil {
return err
}

// Give an empty UniqueVolumeName so that this operation could be executed concurrently.

return oe.pendingOperations.Run("", "", volumesAreAttachedFunc)
What I'm trying to say is that since we are not trying to coordinate with
other operations. We don't need to use either nestedpendingoperations
gorutineMap or operation_executor. We can do all the things that the
method returned by generateVolumesAreAttachedFunc does in thes same file
as syncstates serially.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#34859, or mute the thread
https://github.com/notifications/unsubscribe-auth/ASSNxdfDPhBsh50ABui1gvLgvIeynrLlks5q3TsHgaJpZM4KXdGc
.

Jing

jingxu97 · 2016-10-26T17:03:34Z

@saad-ali PTAL

saad-ali · 2016-10-26T17:46:47Z

Will LGTM by EOD. Giving volume plugin owners a chance to take a look in the meantime.

justinsb · 2016-10-26T18:42:21Z

pkg/cloudprovider/providers/aws/aws.go

+	for _, blockDevice := range info.BlockDeviceMappings {
+		volumeId := aws.StringValue(blockDevice.Ebs.VolumeId)
+		for _, diskName := range diskNames {
+			if volumeId == diskName {


I worry this isn't going to be quite right... I think a diskname normally looks like aws:///vol-abcdef

But I follow the same logic in existing DiskIsAttached function? Is that function fine?

I believe not, unless I am missing something.

Some background here: https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/aws.go#L1277-L1312

A volumeid from AWS is going to look like vol-abcdef0 (or longer). A volumeid in the volume spec could look like that, or could be aws:///vol-abcdef0. We are supposed to normalize before comparing to raw API ids, but it looks like I missed that check.

If you want, we can merge this as is, and then I might do the same thing I did with types.NodeName, with aws.VolumeNameSpec or something, so I can catch any more of these.

I don't think we gate on AWS e2e failures?

Let's do it in a follow-up. Created issue #35746 to followup

justinsb

This is greatly needed, and I think will solve a lot of problems :-)

A few comments ... I'll check on the big one which is whether that is a volume id in the aws provider.

justinsb · 2016-10-26T18:42:58Z

pkg/cloudprovider/providers/gce/gce.go

+				"Instance %q does not exist. DisksAreAttached will assume PD %v are not attached to it.",
+				instanceName,
+				diskNames)
+			return attached, nil


Do we want to require that we return InstanceNotFound here, as we do for ExternalID?

Could you please point to me where ExternalID you mean? Here the logic is if instance does not exist from cloud provider, we assume volumes are all detached. Please let me know if you think this is any issue in this assumption.

What you're doing is fine. In ExternalID in the cloudprovider, we just always return cloudprovider.InstanceNotFound if the instance is not found. Here it would avoid the duplication of code for this case across the providers.

justinsb · 2016-10-26T18:43:40Z

pkg/controller/volume/attachdetach/attach_detach_controller.go

@@ -56,6 +56,10 @@ const (
 	// desiredStateOfWorldPopulatorLoopSleepPeriod is the amount of time the
 	// DesiredStateOfWorldPopulator loop waits between successive executions
 	desiredStateOfWorldPopulatorLoopSleepPeriod time.Duration = 1 * time.Minute
+
+	// reconcilerSyncDuration is the amount of time the reconciler sync states loop
+	// wait between successive exeutions


typo: executions

justinsb · 2016-10-26T18:47:17Z

pkg/controller/volume/attachdetach/reconciler/reconciler.go

-				// If it fails to update node status, skip detach volume
-				rc.actualStateOfWorld.RemoveVolumeFromReportAsAttached(attachedVolume.VolumeName, attachedVolume.NodeName)
+func (rc *reconciler) sync() {
+	defer rc.updateSyncTime()


Any reason to use defer?

I think I tried to use the similar implementation in kubelet reconciler. Any reason defer is a concern?

Just a little surprising to see a 2 line function with a defer :-) If it's consistent with usage elsewhere though, that feels right!

justinsb · 2016-10-26T18:48:17Z

pkg/controller/volume/attachdetach/reconciler/reconciler.go

+func (rc *reconciler) syncStates() {
+	volumesPerNode := rc.actualStateOfWorld.GetAttachedVolumesPerNode()
+	for nodeName, volumes := range volumesPerNode {
+		err := rc.attacherDetacher.VerifyVolumesAreAttached(volumes, nodeName, rc.actualStateOfWorld)


Cloud providers can probably implement this more efficiently if you allow them to do a bulk query...

you mean to query multiple nodes with one call?

Yes exactly ... we can add this later - I suspect we'll hit rate limits on AWS with big clusters

(and GCE, probably!)

Opened #35748 for follow up

justinsb · 2016-10-26T18:54:45Z

pkg/volume/gce_pd/attacher.go

@@ -96,6 +96,40 @@ func (attacher *gcePersistentDiskAttacher) Attach(spec *volume.Spec, nodeName ty
 	return path.Join(diskByIdPath, diskGooglePrefix+pdName), nil
 }

+func (attacher *gcePersistentDiskAttacher) VolumesAreAttached(specs []*volume.Spec, nodeName types.NodeName) (map[*volume.Spec]bool, error) {


Might be time to create the Volumes interface on the cloudprovider :-) The objection last time was that there could be different types; maybe if the volumes interface took a string argument for the "volume type" that would overcome that objection? cc @thockin

justinsb · 2016-10-26T18:58:39Z

pkg/volume/util/operationexecutor/operation_executor.go

+
+			for spec, check := range attached {
+				if !check {
+					actualStateOfWorld.MarkVolumeAsDetached(volumeSpecMap[spec], nodeName)


What is the thread safety guarantee here (genuinely asking - I just don't know how we're keeping things sane here)? Do we serialize operations on a node?

actualStateOfWorld has sync.RWMutex to pretect multiple read/writes. Mounting/Umounting attachable volumes are serialized by nestedpendingoperations for the same volume. But checking volumes are attached can be operated concurrently with other operations. Please let me know if there is any concern.

My concern is that we do asw.GetAttachedVolumesPerNode, which has a read-lock, but that state of the world is then immediately out of date. I think what you're doing here is safe, but I find it hard to think through all the edge cases :-)

This code is racy, like you said after doing GetAttachedVolumesPerNode state could immediately be stale (there could be pending operations that complete, or if sync takes long enough other operations could be started and complete in the meantime). That said, after speaking with @jingxu97 we think that should be ok, because in the worst case scenario, a volume is incorrectly reported as detached when it is actually attached, in which case the controller will try to reattach it--since the operation is idempotent, that should succeed. There shouldn't be any cases where a volume is detached, and falsely reported as attached due to this code.

jingxu97 · 2016-10-28T18:46:55Z

Updated, @saad-ali PTAL

saad-ali · 2016-10-28T22:22:14Z

LGTM

Thanks Jing

saad-ali · 2016-10-28T22:24:03Z

@jessfraz This is not a 1.4.x release blocker. Will be ok to get this cherry picked later next week.

jessfraz · 2016-10-28T22:25:27Z

ok cool thanks!

k8s-github-robot · 2016-10-29T00:32:07Z

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

k8s-github-robot · 2016-10-29T01:33:28Z

Automatic merge from submit-queue

saad-ali · 2016-11-07T06:11:55Z

@jingxu97 Can you please cherrypick this to the 1.4 branch for the next 1.4.x release.

jessfraz · 2016-11-09T18:45:23Z

@jingxu97 can you open the cherry pick so this will be in the release on friday

#35883-upstream-release-1.4 Automated cherry pick of #34859 #35883 upstream release 1.4

…ck-of-#34859-kubernetes#35883-upstream-release-1.4 Automated cherry pick of kubernetes#34859 kubernetes#35883 upstream release 1.4

Automatic merge from submit-queue Add sync state loop in master's volume reconciler At master volume reconciler, the information about which volumes are attached to nodes is cached in actual state of world. However, this information might be out of date in case that node is terminated (volume is detached automatically). In this situation, reconciler assume volume is still attached and will not issue attach operation when node comes back. Pods created on those nodes will fail to mount. This PR adds the logic to periodically sync up the truth for attached volumes kept in the actual state cache. If the volume is no longer attached to the node, the actual state will be updated to reflect the truth. In turn, reconciler will take actions if needed. To avoid issuing many concurrent operations on cloud provider, this PR tries to add batch operation to check whether a list of volumes are attached to the node instead of one request per volume.

googlebot added the cla: yes label Oct 14, 2016

jingxu97 assigned saad-ali and matchstick Oct 14, 2016

jingxu97 added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Oct 14, 2016

jingxu97 added this to the v1.4 milestone Oct 14, 2016

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels Oct 14, 2016

jingxu97 force-pushed the syncAttach-10-15 branch from ee29dfd to 73f5e7b Compare October 14, 2016 21:39

jingxu97 added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Oct 17, 2016

jingxu97 assigned thockin Oct 17, 2016

jingxu97 mentioned this pull request Oct 17, 2016

Mounting/unmounting volume error on GKE #29903

Closed

jingxu97 force-pushed the syncAttach-10-15 branch from 73f5e7b to 58e2601 Compare October 17, 2016 17:33

k8s-github-robot mentioned this pull request Oct 17, 2016

[k8s.io] Kubectl client [k8s.io] Simple pod should support exec {Kubernetes e2e suite} #33756

Closed

saad-ali suggested changes Oct 24, 2016

View reviewed changes

jingxu97 force-pushed the syncAttach-10-15 branch from 58e2601 to 14c596a Compare October 24, 2016 23:01

jingxu97 force-pushed the syncAttach-10-15 branch from 14c596a to 0ef3370 Compare October 24, 2016 23:10

jingxu97 force-pushed the syncAttach-10-15 branch from 0ef3370 to feec268 Compare October 25, 2016 17:53

saad-ali suggested changes Oct 25, 2016

View reviewed changes

jingxu97 force-pushed the syncAttach-10-15 branch from feec268 to 4629322 Compare October 25, 2016 21:02

jingxu97 force-pushed the syncAttach-10-15 branch from 4629322 to 038d611 Compare October 25, 2016 23:08

justinsb reviewed Oct 26, 2016

View reviewed changes

jingxu97 unassigned thockin and saad-ali Oct 26, 2016

saad-ali added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Oct 28, 2016

saad-ali added the cherrypick-candidate label Oct 28, 2016

k8s-github-robot merged commit 3e7172d into kubernetes:master Oct 29, 2016

jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 29, 2016

saad-ali mentioned this pull request Nov 7, 2016

EBS volume unmounting is stalling #36269

Closed

jingxu97 mentioned this pull request Nov 11, 2016

Automated cherry pick of #34859 #35883 #36676

Closed

saad-ali mentioned this pull request Nov 12, 2016

Automated cherry pick of #34859 #35883 upstream release 1.4 #36683

Merged

jessfraz added a commit that referenced this pull request Nov 12, 2016

Merge pull request #36683 from saad-ali/automated-cherry-pick-of-#34859-

b953db2

#35883-upstream-release-1.4 Automated cherry pick of #34859 #35883 upstream release 1.4

jessfraz removed the cherrypick-candidate label Nov 12, 2016

jingxu97 mentioned this pull request Nov 17, 2016

Volume attached/detached info out of sync at master when node is terminated. #33760

Closed

saad-ali mentioned this pull request Nov 23, 2016

Volume stuck in attaching state when using multiple PersistentVolumeClaim #36450

Closed

chentao1596 mentioned this pull request Dec 5, 2016

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

jingxu97 mentioned this pull request Dec 27, 2016

vSphere Cloud provider: Volume detach is not issued when VM is not present #33061

Closed

saad-ali mentioned this pull request Jan 6, 2017

AWS PV Storage Causing API Timeouts. #39526

Closed

jingxu97 mentioned this pull request Mar 13, 2017

Ungraceful node deletion handling #31014

Closed

saad-ali mentioned this pull request Jun 7, 2017

Failed volume attach/detach operations leave volume state uncertain #32727

Closed

This was referenced Sep 12, 2017

disable adc reconciler sync for aws openshift/openshift-ansible#5387

Merged

Add support for verifying attached but desired to be detached PVCs #52573

Closed

Add sync state loop in master's volume reconciler #34859

Add sync state loop in master's volume reconciler #34859

Conversation

jingxu97 commented Oct 14, 2016 • edited by jessfraz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Oct 24, 2016

jingxu97 commented Oct 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Oct 25, 2016

jingxu97 commented Oct 25, 2016

@saad-ali commented on this pull request.

jingxu97 commented Oct 26, 2016

saad-ali commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb left a comment

Choose a reason for hiding this comment

justinsb Oct 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Oct 28, 2016

saad-ali commented Oct 28, 2016

saad-ali commented Oct 28, 2016

jessfraz commented Oct 28, 2016

k8s-github-robot commented Oct 29, 2016

k8s-github-robot commented Oct 29, 2016

saad-ali commented Nov 7, 2016

jessfraz commented Nov 9, 2016

jingxu97 commented Oct 14, 2016 •

edited by jessfraz

Loading

justinsb Oct 26, 2016 •

edited

Loading