{"action":"created","issue":{"url":"https://api.github.com/repos/kubernetes/kubernetes/issues/24331","repository_url":"https://api.github.com/repos/kubernetes/kubernetes","labels_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/24331/labels{/name}","comments_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/24331/comments","events_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/24331/events","html_url":"https://github.com/kubernetes/kubernetes/pull/24331","id":148680776,"number":24331,"title":"Refactor persistent volume controller","user":{"login":"jsafrane","id":1745006,"avatar_url":"https://avatars.githubusercontent.com/u/1745006?v=3","gravatar_id":"","url":"https://api.github.com/users/jsafrane","html_url":"https://github.com/jsafrane","followers_url":"https://api.github.com/users/jsafrane/followers","following_url":"https://api.github.com/users/jsafrane/following{/other_user}","gists_url":"https://api.github.com/users/jsafrane/gists{/gist_id}","starred_url":"https://api.github.com/users/jsafrane/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jsafrane/subscriptions","organizations_url":"https://api.github.com/users/jsafrane/orgs","repos_url":"https://api.github.com/users/jsafrane/repos","events_url":"https://api.github.com/users/jsafrane/events{/privacy}","received_events_url":"https://api.github.com/users/jsafrane/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/area/storage","name":"area/storage","color":"006b75"},{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/cla:%20yes","name":"cla: yes","color":"bfe5bf"},{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/kind/api-change","name":"kind/api-change","color":"c7def8"},{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/release-note","name":"release-note","color":"c2e0c6"},{"url":"https://api.github.com/repos/kubernetes/kubernetes/labels/size/XXL","name":"size/XXL","color":"ee0000"}],"state":"open","locked":false,"assignee":{"login":"thockin","id":5595220,"avatar_url":"https://avatars.githubusercontent.com/u/5595220?v=3","gravatar_id":"","url":"https://api.github.com/users/thockin","html_url":"https://github.com/thockin","followers_url":"https://api.github.com/users/thockin/followers","following_url":"https://api.github.com/users/thockin/following{/other_user}","gists_url":"https://api.github.com/users/thockin/gists{/gist_id}","starred_url":"https://api.github.com/users/thockin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/thockin/subscriptions","organizations_url":"https://api.github.com/users/thockin/orgs","repos_url":"https://api.github.com/users/thockin/repos","events_url":"https://api.github.com/users/thockin/events{/privacy}","received_events_url":"https://api.github.com/users/thockin/received_events","type":"User","site_admin":false},"milestone":null,"comments":24,"created_at":"2016-04-15T14:49:19Z","updated_at":"2016-05-09T13:36:23Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/kubernetes/kubernetes/pulls/24331","html_url":"https://github.com/kubernetes/kubernetes/pull/24331","diff_url":"https://github.com/kubernetes/kubernetes/pull/24331.diff","patch_url":"https://github.com/kubernetes/kubernetes/pull/24331.patch"},"body":"Here is complete persistent controller as designed in https://github.com/pmorie/pv-haxxz/blob/master/controller.go\r\n\r\nIt's feature complete and compatible with current binder/recycler/provisioner. No new features, it *should* be much more stable and predictable.\r\n\r\nTesting\r\n--\r\nThe unit test framework is quite complicated, still it was necessary to reach reasonable coverage (78% in `persistentvolume_controller.go`). The untested part are error cases, which are quite hard to test in reasonable way - sure, I can inject a VersionConflictError on any object update and check the error bubbles up to appropriate places, but the real test would be to run `syncClaim`/`syncVolume` again and check it recovers appropriately from the error in the next periodic sync. That's the hard part.\r\n\r\nOrganization\r\n---\r\nThe PR starts with `rm -rf kubernetes/pkg/controller/persistentvolume`. I find it easier to read when I see only the new controller without old pieces scattered around.\r\n[`types.go` from the old controller is reused to speed up matching a bit, the code looks solid and has 95% unit test coverage].\r\n\r\nI tried to split the PR into smaller patches, let me know what you think.\r\n\r\n~~TODO~~\r\n--\r\n\r\n* ~~Missing: provisioning, recycling~~.\r\n* ~~Fix integration tests~~\r\n* ~~Fix e2e tests~~\r\n\r\n@kubernetes/sig-storage "},"comment":{"url":"https://api.github.com/repos/kubernetes/kubernetes/issues/comments/217865754","html_url":"https://github.com/kubernetes/kubernetes/pull/24331#issuecomment-217865754","issue_url":"https://api.github.com/repos/kubernetes/kubernetes/issues/24331","id":217865754,"user":{"login":"k8s-bot","id":12551979,"avatar_url":"https://avatars.githubusercontent.com/u/12551979?v=3","gravatar_id":"","url":"https://api.github.com/users/k8s-bot","html_url":"https://github.com/k8s-bot","followers_url":"https://api.github.com/users/k8s-bot/followers","following_url":"https://api.github.com/users/k8s-bot/following{/other_user}","gists_url":"https://api.github.com/users/k8s-bot/gists{/gist_id}","starred_url":"https://api.github.com/users/k8s-bot/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/k8s-bot/subscriptions","organizations_url":"https://api.github.com/users/k8s-bot/orgs","repos_url":"https://api.github.com/users/k8s-bot/repos","events_url":"https://api.github.com/users/k8s-bot/events{/privacy}","received_events_url":"https://api.github.com/users/k8s-bot/received_events","type":"User","site_admin":false},"created_at":"2016-05-09T13:36:23Z","updated_at":"2016-05-09T13:36:23Z","body":"GCE e2e build/test **passed** for commit 26b617b308290d21e312562b51084973ffabfad6.\n* [Build Log](http://pr-test.k8s.io/24331/kubernetes-pull-build-test-e2e-gce/39084/build-log.txt)\n* [Test Artifacts](https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/24331/kubernetes-pull-build-test-e2e-gce/39084/artifacts/)\n* [Internal Jenkins Results](http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//39084)\n"}}