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 startup type error in initializeCaches #28002

Merged

k8s-github-robot merged 1 commit into kubernetes:master from asalkeld:init-cache-error

Jun 25, 2016

asalkeld commented Jun 24, 2016

The following error was getting logged:
PersistentVolumeController can't initialize caches, expected list of volumes, got:
&{TypeMeta:{Kind: APIVersion:} ListMeta:{SelfLink:/api/v1/persistentvolumes ResourceVersion:11} Items:[]}

The tests make extensive use of NewFakeControllerSource which uses api.List
instead of api.PersistentVolumeList. So use reflect to help iterate over the
items then assert the item type.

fixes #27757

googlebot added the cla: yes label

k8s-bot commented Jun 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

4 similar comments

k8s-bot commented Jun 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented Jun 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented Jun 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot commented Jun 24, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-github-robot assigned saad-ali

k8s-github-robot added size/M release-note-label-needed labels

saad-ali assigned jsafrane and unassigned saad-ali

Member

jsafrane commented Jun 24, 2016

@k8s-bot ok to test

jsafrane added sig/storage team/cluster release-note-none and removed release-note-label-needed labels

jsafrane reviewed

View reviewed changes

pkg/controller/persistentvolume/controller_base.go

    
              		return

              	}

              	for _, volume := range volumeList.Items {

              	volumeListVal := reflect.Indirect(reflect.ValueOf(volumeListObj))

Member

jsafrane Jun 24, 2016

Please add a comment why reflect is needed as both List and PersistentVolumeList need to be processed.

Member

jsafrane commented Jun 24, 2016

@k8s-bot e2e test this issue: #IGNORE

Member

jsafrane commented Jun 24, 2016

LGTM

Member

jsafrane commented Jun 24, 2016

@k8s-bot e2e test this issue: #IGNORE

jsafrane added the lgtm label

jsafrane added this to the v1.3 milestone

Member

jsafrane commented Jun 24, 2016 •

edited

Loading

labeling into 1.3, this basically fixes controller initialization after crash, without this patch the controller may release (and recycle!) bound volumes!

jsafrane removed the lgtm label

Member

jsafrane commented Jun 24, 2016

@asalkeld, please squash your commits into one and I'll merge it.

k8s-bot commented Jun 24, 2016

GCE e2e build/test passed for commit 035f9e6a916f5b80fa5fb5938aa2d301ea86302e.

Member

jsafrane commented Jun 24, 2016

Ok, so @asalkeld is probably sleeping by now. @saad-ali, can you please coordinate with him to either squash the patches or merge them as they are? I'd really like to have them in Kube 1.3. This patch won't break volume controller more than it already is.

Nobody noticed this error earlier, as the volume sources behaves differently in unit tests and we don't really test controller startup with existing PVs/PVCs anywhere else. I'll write an integration test for this.

Member

idvoretskyi commented Jun 24, 2016

@jsafrane we have enough time (~7-8 hours) to wait for @asalkeld squashing the commits and we'll be ready to merge this item (I'm against of merging as-is).

Member

jsafrane commented Jun 24, 2016

I'm testing the patch properly and it seems it does not work as expected. Problem is that volumeSource.List in unit tests returns api.List with *api.PersistentVolume items, while real controller (and informer below) returns api.PersistentVolumeList with api.PersistentVolume as items, i.e. the items are not pointers.

volume, ok := volumeListItems.Index(i).Interface().(*api.PersistentVolume) will then fail.

eparis added the priority/critical-urgent label

k8s-github-robot commented Jun 25, 2016

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

k8s-bot commented Jun 25, 2016

GCE e2e build/test passed for commit b4f7e67.

k8s-github-robot commented Jun 25, 2016

Automatic merge from submit-queue

k8s-github-robot merged commit 1effc5a into kubernetes:master

Member

lavalamp commented Jun 25, 2016

Would it be a good idea for the PVC controller, when it sees a dangling reference, to do a cache-bypassing read to confirm the dangling reference, before recycling?

Possibly. Need to understand the locking mechanism to say for sure.

asalkeld deleted the init-cache-error branch

June 25, 2016 23:13

Member

jsafrane commented Jun 26, 2016

Since the PVCs and PVs are watched using separate watch operations, it seems possible that one watch could fall behind the other by an arbitrary amount of time, due to delays in various parts of the system. So, isn't it possible for the controller to get in this same kind of state, even after syncing?

It's not likely. Once the controller has complete view of the world (i.e. after initial startup), it can receive events from PVs and PVCs at different pace as long as the controller is the only one who binds volumes. Bad things can happen when a bound PV appears and the controller does not see appropriate counterpart PVC. The PV may get recycled or deleted, which is exactly what happened in this bug.

@asalkeld, @lavalamp thanks for your patches and reviews!

This was referenced Jun 27, 2016

TestControllerSync {persistentvolume} #28076

Closed

Fix initialization of volume controller caches. #28108

Merged

jsafrane mentioned this pull request

Add integration test for volume controller startup. #28116

Merged

Member

erictune commented Jun 27, 2016

If events from PVs and PVCs arrive at very different paces, doesn't that mean a bound PV could show up before the create for a PVC ever shows up?

erictune added the cherry-pick-approved label

Member

lavalamp commented Jun 27, 2016

@erictune no, because the controller must have observed the PVC in order to make the binding in the first place. (Assumption: there's only one controller in operation, and no other actor can cause a binding to occur.)

@jsafrane I recall talking locking methods to make all of these operations safe even with multiple controllers with a group of red hatters and googlers a few months ago. I take it that has not been implemented yet?

eparis mentioned this pull request

Batch update for 1.3 #28119

Merged

Member

jsafrane commented Jun 28, 2016

@lavalamp, even with locking as suggested on the meeting we were able to find races when two controllers were trying to bind two PVs/PVCs at the same time.

eparis pushed a commit to eparis/kubernetes that referenced this pull request


          Merge pull request kubernetes#28002 from asalkeld/init-cache-error

9d6c32f

Automatic merge from submit-queue

Fix startup type error in initializeCaches

The following error was getting logged:
PersistentVolumeController can't initialize caches, expected list of volumes, got:
&{TypeMeta:{Kind: APIVersion:} ListMeta:{SelfLink:/api/v1/persistentvolumes ResourceVersion:11} Items:[]}

The tests make extensive use of NewFakeControllerSource which uses api.List
instead of api.PersistentVolumeList. So use reflect to help iterate over the
items then assert the item type.

fixes kubernetes#27757
(cherry picked from commit 1effc5a)

k8s-github-robot pushed a commit that referenced this pull request


          Merge pull request #28119 from eparis/release-1.3

28925df

Automatic merge from submit-queue

Batch update for 1.3

#28030: Revert "Federation e2e supports aws"
#28026: Address outstanding review comments in #27999.
#28034: Adding lock files for kubeconfig updating
#28004: return nil from NewClientConfig instead of empty struct
#28032: Increase pod CPU/memory for fluentd, dns and kube-proxy.
#27208: Bump minimum API version for docker to 1.21
#28061: Remove extra double quotes in --federations.
#28060: rkt: Fix the 'privileged' check when stage1 annotation is provided.
#27996: Image GC logic should compensate for reserved blocks
#28044: rkt: Bump required rkt version to 1.9.1.
#28040: Tracked addition of federation, sed support in kube DNS
#28043: Set grace period to 0 when deleting namespaces after the test.
#28002: Fix startup type error in initializeCaches
#28087: Hotfix: Fixup the hyperkube dns manifest from a breaking federation PR
#28108: Fix initialization of volume controller caches.
#28056: Increase kube-dns requirements on CoreOS.
#28147: Fix error checks after cloning.
#28159: Use : as seccomp security option operator for Docker 1.10
#28165: Refactored, expanded and fixed federated-services e2e tests.
#28095: Kubelet should mark VolumeInUse before checking if it is Attached
#28172: Build: Add KUBE_GCS_RELEASE_BUCKET_MIRROR option to push-ci-build.sh
#28207: Bump cluster autoscaler to 0.2.2
#28160: Volume manager must verify containers terminated before deleting for ungracefully terminated pods
#28211: Fix federation e2e tests by correctly managing cluster clients
#27944: Fix pvc label selector validation error
#28186: federation: Upgrading the groupversion to v1beta1

k8s-cherrypick-bot commented Jun 29, 2016

Commit found in the "release-1.3" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-cherrypick-bot removed the cherrypick-candidate label

jsafrane added a commit to jsafrane/kubernetes that referenced this pull request


          Add integration test for volume controller startup.

36c607b

Tests kubernetes#28002 with real etcd (unit tests have a fake one with different
behavior).

k8s-github-robot pushed a commit that referenced this pull request


          Merge pull request #28116 from jsafrane/integration-startup

10121b4

Automatic merge from submit-queue

Add integration test for volume controller startup.

Tests #28002 with real etcd (unit tests have a fake one with different behavior).

@kubernetes/sig-storage

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

Labels

cherry-pick-approved lgtm priority/critical-urgent release-note-none sig/storage size/L