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

Fix race condition in cinder attach/detach #19707

Merged
merged 1 commit into from Feb 2, 2016

Conversation

Projects
None yet
@jsafrane
Member

jsafrane commented Jan 15, 2016

Resolves #19602

Most of the changes are related to unit test. The real fix is in cinder.go. Similar change is needed in most of the volume plugins! Cinder seems to be the easiest to reproduce in real world, as it's the slowest to attach volumes (and thus the window of opportunity is the largest).

Also, I added some logs here and there to debug this issue easily next time.

@googlebot googlebot added the cla: yes label Jan 15, 2016

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Jan 15, 2016

Contributor

Labelling this PR as size/L

Contributor

k8s-merge-robot commented Jan 15, 2016

Labelling this PR as size/L

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 15, 2016

GCE e2e test build/test passed for commit d7f23bb.

k8s-bot commented Jan 15, 2016

GCE e2e test build/test passed for commit d7f23bb.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 15, 2016

GCE e2e test build/test passed for commit 074eeeb.

k8s-bot commented Jan 15, 2016

GCE e2e test build/test passed for commit 074eeeb.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Jan 15, 2016

Contributor

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

Contributor

k8s-merge-robot commented Jan 15, 2016

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented Jan 18, 2016

GCE e2e build/test failed for commit 907a1d1.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 18, 2016

GCE e2e test build/test passed for commit 907a1d1.

k8s-bot commented Jan 18, 2016

GCE e2e test build/test passed for commit 907a1d1.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 18, 2016

GCE e2e test build/test passed for commit 907a1d1.

k8s-bot commented Jan 18, 2016

GCE e2e test build/test passed for commit 907a1d1.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 19, 2016

GCE e2e test build/test passed for commit 3992a40.

k8s-bot commented Jan 19, 2016

GCE e2e test build/test passed for commit 3992a40.

Show outdated Hide outdated pkg/volume/util.go
@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Jan 20, 2016

Member

Rebased & removed custom VolumeGuard mutexes.

Member

jsafrane commented Jan 20, 2016

Rebased & removed custom VolumeGuard mutexes.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented Jan 20, 2016

GCE e2e build/test failed for commit 23fb4a5.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented Jan 20, 2016

GCE e2e build/test failed for commit 7a852ab.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 20, 2016

GCE e2e test build/test passed for commit e8ed4a1.

k8s-bot commented Jan 20, 2016

GCE e2e test build/test passed for commit e8ed4a1.

@saad-ali

This comment has been minimized.

Show comment
Hide comment
@saad-ali

saad-ali Jan 20, 2016

Member

LGTM. Thanks for the fix @jsafrane. Please squash the commits and I'll add the LGTM label.

Member

saad-ali commented Jan 20, 2016

LGTM. Thanks for the fix @jsafrane. Please squash the commits and I'll add the LGTM label.

@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Jan 21, 2016

Member

@saad-ali, I can squash them, but I actually prefer the way they are organized now. Small patches with clear purpose.

Member

jsafrane commented Jan 21, 2016

@saad-ali, I can squash them, but I actually prefer the way they are organized now. Small patches with clear purpose.

@markturansky

This comment has been minimized.

Show comment
Hide comment
@markturansky

markturansky Jan 21, 2016

Member

@jsafrane clear to review, yes, and that is helpful. But still a logical single commit that is easy to rollback if things go south. Squashing before merging a PR is standard procedure.

Member

markturansky commented Jan 21, 2016

@jsafrane clear to review, yes, and that is helpful. But still a logical single commit that is easy to rollback if things go south. Squashing before merging a PR is standard procedure.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 21, 2016

GCE e2e test build/test passed for commit fad7997.

k8s-bot commented Jan 21, 2016

GCE e2e test build/test passed for commit fad7997.

@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Jan 21, 2016

Member

Squashed

Member

jsafrane commented Jan 21, 2016

Squashed

@markturansky

This comment has been minimized.

Show comment
Hide comment
@markturansky

markturansky Jan 21, 2016

Member

Thanks, Jan. I'll add LGTM, since @saad-ali said he would have done that after a squash.

Member

markturansky commented Jan 21, 2016

Thanks, Jan. I'll add LGTM, since @saad-ali said he would have done that after a squash.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 21, 2016

GCE e2e test build/test passed for commit c00192a.

k8s-bot commented Jan 21, 2016

GCE e2e test build/test passed for commit c00192a.

@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Jan 21, 2016

Member
=== RUN   TestAttachDetachRace
I0121 15:37:58.424363   19808 cinder_test.go:301] Attaching volume
I0121 15:37:58.924502   19808 cinder_test.go:312] Detaching volume
==================
WARNING: DATA RACE
Write by goroutine 19:
  k8s.io/kubernetes/pkg/util/mount.(*FakeMounter).Mount()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/util/mount/fake.go:70 +0x3cc
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeBuilder).SetUpAt()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:271 +0x10dd
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeBuilder).SetUp()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:231 +0x7b
  k8s.io/kubernetes/pkg/volume/cinder.TestAttachDetachRace.func1()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder_test.go:302 +0x74

Previous read by goroutine 18:
  k8s.io/kubernetes/pkg/util/mount.(*FakeMounter).IsLikelyNotMountPoint()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/util/mount/fake.go:95 +0x64
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeCleaner).TearDownAt()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:328 +0x210
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeCleaner).TearDown()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:321 +0x7b
  k8s.io/kubernetes/pkg/volume/cinder.TestAttachDetachRace()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder_test.go:313 +0x1479
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

I'll add locks around but come on, it's fake mounter...

Member

jsafrane commented Jan 21, 2016

=== RUN   TestAttachDetachRace
I0121 15:37:58.424363   19808 cinder_test.go:301] Attaching volume
I0121 15:37:58.924502   19808 cinder_test.go:312] Detaching volume
==================
WARNING: DATA RACE
Write by goroutine 19:
  k8s.io/kubernetes/pkg/util/mount.(*FakeMounter).Mount()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/util/mount/fake.go:70 +0x3cc
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeBuilder).SetUpAt()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:271 +0x10dd
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeBuilder).SetUp()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:231 +0x7b
  k8s.io/kubernetes/pkg/volume/cinder.TestAttachDetachRace.func1()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder_test.go:302 +0x74

Previous read by goroutine 18:
  k8s.io/kubernetes/pkg/util/mount.(*FakeMounter).IsLikelyNotMountPoint()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/util/mount/fake.go:95 +0x64
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeCleaner).TearDownAt()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:328 +0x210
  k8s.io/kubernetes/pkg/volume/cinder.(*cinderVolumeCleaner).TearDown()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder.go:321 +0x7b
  k8s.io/kubernetes/pkg/volume/cinder.TestAttachDetachRace()
      /workspace/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/volume/cinder/cinder_test.go:313 +0x1479
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:456 +0xdc

I'll add locks around but come on, it's fake mounter...

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Jan 21, 2016

Contributor

PR changed after LGTM, removing LGTM.

Contributor

k8s-merge-robot commented Jan 21, 2016

PR changed after LGTM, removing LGTM.

@k8s-merge-robot k8s-merge-robot removed the lgtm label Jan 21, 2016

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 21, 2016

GCE e2e test build/test passed for commit 3e67165.

k8s-bot commented Jan 21, 2016

GCE e2e test build/test passed for commit 3e67165.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 22, 2016

GCE e2e test build/test passed for commit 2c95117.

k8s-bot commented Jan 22, 2016

GCE e2e test build/test passed for commit 2c95117.

// Find Cinder volumeID to lock the right volume
// TODO: refactor VolumePlugin.NewCleaner to get full volume.Spec just like
// NewBuilder. We could then find volumeID there without probing MountRefs.

This comment has been minimized.

@markturansky

markturansky Jan 22, 2016

Member

Tim has this same kind of TODO in kubelet's volume code: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumes.go#L216

Not for this PR, of course, but it's worth thinking about a solution.

@markturansky

markturansky Jan 22, 2016

Member

Tim has this same kind of TODO in kubelet's volume code: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/volumes.go#L216

Not for this PR, of course, but it's worth thinking about a solution.

This comment has been minimized.

@jsafrane

jsafrane Jan 22, 2016

Member

I think kubelet has whole VolumeSpec available when it creates cleaners, so it's fairly simple change. On the other hand, all the volume plugins needs to be updated with new NewCleaner signature.

@jsafrane

jsafrane Jan 22, 2016

Member

I think kubelet has whole VolumeSpec available when it creates cleaners, so it's fairly simple change. On the other hand, all the volume plugins needs to be updated with new NewCleaner signature.

c.plugin.volumeLocks.LockKey(c.pdName)
defer c.plugin.volumeLocks.UnlockKey(c.pdName)
// Reload list of references, there might be SetUpAt finished in the meantime

This comment has been minimized.

@markturansky

markturansky Jan 22, 2016

Member

if the lock is at the beginning of the function, would you still need to reload the list here?

@markturansky

markturansky Jan 22, 2016

Member

if the lock is at the beginning of the function, would you still need to reload the list here?

This comment has been minimized.

@jsafrane

jsafrane Jan 22, 2016

Member

All the code before LockKey(c.pdName) is there to actually find c.pdName. That's why passing whole VolumeSpec to NewCleaner() would be much nicer - c.pdName is actually VolumeSpec.Cinder.VolumeID.

@jsafrane

jsafrane Jan 22, 2016

Member

All the code before LockKey(c.pdName) is there to actually find c.pdName. That's why passing whole VolumeSpec to NewCleaner() would be much nicer - c.pdName is actually VolumeSpec.Cinder.VolumeID.

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Jan 22, 2016

GCE e2e test build/test passed for commit 2c95117.

k8s-bot commented Jan 22, 2016

GCE e2e test build/test passed for commit 2c95117.

@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Jan 28, 2016

Member

Rebased

Member

jsafrane commented Jan 28, 2016

Rebased

@markturansky

This comment has been minimized.

Show comment
Hide comment
@markturansky

markturansky Jan 29, 2016

Member

LGTM.

Since @saad-ali also LGTM'd it, I will add the label.

Member

markturansky commented Jan 29, 2016

LGTM.

Since @saad-ali also LGTM'd it, I will add the label.

@markturansky markturansky added the lgtm label Jan 29, 2016

@eparis

This comment has been minimized.

Show comment
Hide comment
@eparis

eparis Feb 1, 2016

Member

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

Member

eparis commented Feb 1, 2016

@k8s-bot test this issue: #IGNORE

Tests have been pending for 24 hours

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Feb 1, 2016

GCE e2e test build/test passed for commit af9c9c5.

k8s-bot commented Feb 1, 2016

GCE e2e test build/test passed for commit af9c9c5.

Fixed races in Cinder volume attach/detach.
Add a mutex to guard SetUpAt() and TearDownAt() calls - they should not
run in parallel.  There is a race in these calls when there are two pods
using the same volume, one of them is dying and the other one starting.

TearDownAt() checks that a volume is not needed by any pods and detaches the
volume. It does so by counting how many times is the volume mounted
(GetMountRefs() call below).

When SetUpAt() of the starting pod already attached the volume and did not mount
it yet, TearDownAt() of the dying pod will detach it - GetMountRefs() does not
count with this volume.

These two threads run in parallel:

 dying pod.TearDownAt("myVolume")          starting pod.SetUpAt("myVolume")
   |                                       |
   |                                       AttachDisk("myVolume")
   refs, err := mount.GetMountRefs()       |
   Unmount("myDir")                        |
   if refs == 1 {                          |
   |  |                                    Mount("myVolume", "myDir")
   |  |                                    |
   |  DetachDisk("myVolume")               |
   |                                       start containers - OOPS! The volume is detached!
   |
   finish the pod cleanup


Also, add some logs to cinder plugin for easier debugging in the future, add
a test and update the fake mounter to know about bind mounts.
@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Feb 2, 2016

Contributor

PR changed after LGTM, removing LGTM.

Contributor

k8s-merge-robot commented Feb 2, 2016

PR changed after LGTM, removing LGTM.

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 2, 2016

@jsafrane

This comment has been minimized.

Show comment
Hide comment
@jsafrane

jsafrane Feb 2, 2016

Member

@markturansky, I had to rebase and I lost LGTH label, can you please add it again?

Member

jsafrane commented Feb 2, 2016

@markturansky, I had to rebase and I lost LGTH label, can you please add it again?

@markturansky markturansky added the lgtm label Feb 2, 2016

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Feb 2, 2016

GCE e2e test build/test passed for commit 220163f.

k8s-bot commented Feb 2, 2016

GCE e2e test build/test passed for commit 220163f.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Feb 2, 2016

Contributor

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

Contributor

k8s-merge-robot commented Feb 2, 2016

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

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot Feb 2, 2016

GCE e2e test build/test passed for commit 220163f.

k8s-bot commented Feb 2, 2016

GCE e2e test build/test passed for commit 220163f.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Feb 2, 2016

Contributor

Automatic merge from submit-queue

Contributor

k8s-merge-robot commented Feb 2, 2016

Automatic merge from submit-queue

k8s-merge-robot added a commit that referenced this pull request Feb 2, 2016

@k8s-merge-robot k8s-merge-robot merged commit b731d82 into kubernetes:master Feb 2, 2016

4 of 5 checks passed

Submit Queue Github CI tests are not green.
Details
Jenkins GCE e2e 220 tests run, 104 skipped, 0 failed.
Details
Jenkins unit/integration 2657 tests run, 9 skipped, 0 failed.
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@k8s-teamcity-mesosphere

This comment has been minimized.

Show comment
Hide comment
@k8s-teamcity-mesosphere

k8s-teamcity-mesosphere Feb 2, 2016

TeamCity OSS :: Kubernetes Mesos :: 4 - Smoke Tests Build 14132 outcome was FAILURE
Summary: Tests failed: 1 (1 new), passed: 0, ignored: 219 Build time: 00:07:14

Failed tests

null: Kubernetes e2e suite.Kubectl client Guestbook application should create and stop a working application [Conformance]: <no details avaliable>

k8s-teamcity-mesosphere commented on 220163f Feb 2, 2016

TeamCity OSS :: Kubernetes Mesos :: 4 - Smoke Tests Build 14132 outcome was FAILURE
Summary: Tests failed: 1 (1 new), passed: 0, ignored: 219 Build time: 00:07:14

Failed tests

null: Kubernetes e2e suite.Kubectl client Guestbook application should create and stop a working application [Conformance]: <no details avaliable>

@jsafrane jsafrane deleted the jsafrane:devel/fix-cinder-teardown branch Aug 19, 2016

dims pushed a commit to dims/kubernetes that referenced this pull request Feb 8, 2018

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