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 hack/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.

@jsafrane @pmorie @childsb PTAL when you get a chance.

. fixes PV status not being changed from Released -> Bound for a pv with a pre-binding claimRef.

That's a feature, not a bug. Se line comment somewhere above for an example.

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 hack/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.

@jsafrane I wrote ". fixes PV status not being changed from Released -> Bound for a pv with a pre-binding claimRef.", and you replied:
That's a feature, not a bug. Se line comment somewhere above for an example.
Can you explain more?
I see when I create a pv with a claimRef, and its matching claim, and the pv's persistentVolumeReclaimPolicy = "Recycle" then the pv goes from Released -> Bound.
But, today if I create the same pv but it's ReclaimPolicy == Retain then the pv remains in the Released phase. Is this by design?

But, today if I create the same pv but it's ReclaimPolicy == Retain then the pv remains in the Released phase. Is this by design?

Yes, volumes with Retain policy should stay Released forever and never get bound again. There is some old data on the volume.

@jsafrane you replied: Yes, volumes with Retain policy should stay Released forever and never get bound again. There is some old data on the volume.
But, in my example the PV was never bound. Neither the pv nor claim ever went to the Bound phase. That's what I think is a problem and what I was attempting to address in the PR.
Example from latest k8s on master:

# kc version
Client Version: version.Info{Major:"1", Minor:"3+", GitVersion:"v1.3.0-alpha.3.107+5fc596705967cc", GitCommit:"5fc596705967cc09d116a85ccced70bd6f661b5e", GitTreeState:"clean", BuildDate:"2016-04-28T19:39:37Z", GoVersion:"go1.5.3", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"3+", GitVersion:"v1.3.0-alpha.3.107+5fc596705967cc", GitCommit:"5fc596705967cc09d116a85ccced70bd6f661b5e", GitTreeState:"clean", BuildDate:"2016-04-28T19:39:37Z", GoVersion:"go1.5.3", Compiler:"gc", Platform:"linux/amd64"}

Create claim3 (volumeName="") and create PV3 (claimRef -> "claim3" and ReclaimPolicy=Retain):

### PVC:
# kc get pvc nfs-claim3 -o yaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  creationTimestamp: 2016-04-28T19:45:24Z
  name: nfs-claim3
  namespace: default
  resourceVersion: "30"
  selfLink: /api/v1/namespaces/default/persistentvolumeclaims/nfs-claim3
  uid: c005ebc3-0d79-11e6-be20-5254008f071b
spec:
  accessModes:
  - ReadWriteOnce
  resources:
    requests:
      storage: 1Gi
status:
  phase: Pending

### PV:
# kc get pv nfs-pv3 -o yaml
apiVersion: v1
kind: PersistentVolume
metadata:
  creationTimestamp: 2016-04-28T19:47:12Z
  name: nfs-pv3
  resourceVersion: "43"
  selfLink: /api/v1/persistentvolumes/nfs-pv3
  uid: 007602bc-0d7a-11e6-be20-5254008f071b
spec:
  accessModes:
  - ReadWriteOnce
  capacity:
    storage: 1Gi
  claimRef:
    name: nfs-claim3
    namespace: default
  nfs:
    path: /opt/nfs
    server: 192.168.122.73
  persistentVolumeReclaimPolicy: Retain
status:
  phase: Released

20 minutes later...

# kc get pv
NAME      CAPACITY   ACCESSMODES   STATUS     CLAIM                REASON    AGE
nfs-pv3   1Gi        RWO           Released   default/nfs-claim3             18m
[root@rhel7/nfs]# kc get pvc
NAME         STATUS    VOLUME    CAPACITY   ACCESSMODES   AGE
nfs-claim3   Pending                                      20m

PV remains Released and claim remains Pending and no pod can use the claim.

If I change PV3's ClaimPolicy from Retain to Recycle then PV3 quickly binds:

# kc get pvc
NAME         STATUS    VOLUME    CAPACITY   ACCESSMODES   AGE
nfs-claim3   Bound     nfs-pv3   1Gi        RWO           30s
[root@rhel7/nfs]# kc get pv
NAME      CAPACITY   ACCESSMODES   STATUS    CLAIM                REASON    AGE
nfs-pv3   1Gi        RWO           Bound     default/nfs-claim3             28s

This seems like a bug related to Retain?

@jsafrane you replied: Yes, volumes with Retain policy should stay Released forever and never get bound again. There is some old data on the volume.
But, in my example the PV was never bound. Neither the pv nor claim ever went to the Bound phase. That's what I think is a problem and what I was attempting to address in the PR.

Volumes get to Released phase only when they were bound to an old claim and the claim was deleted. So, the volume was bound, the claim just does not exist. There may be old data on it and admin said we need to retain the data (by ReclaimPolicy: Retain).

@jsafrane you wrote:

Volumes get to Released phase only when they were bound to an old claim and the claim was deleted. So, the volume was bound, the claim just does not exist. There may be old data on it and admin said we need to retain the data (by ReclaimPolicy: Retain).

Yes, several days ago the volume was bound. But since then the volume was deleted, the claim was deleted, k8s was restarted, etc. Are you saying that if a Retained pv is ever bound in it's life then it can never be claimed again? And, if so, is there a mechanism to reset/release this pv so that it can be a candidate for being claimed again?

@jeffvance:

Are you saying that if a Retained pv is ever bound in it's life then it can never be claimed again? And, if so, is there a mechanism to reset/release this pv so that it can be a candidate for being claimed again?

Exactly, admin wants to Retain the volume, so Kubernetes does exactly that. If admin wants the PV to be available for binding again, he/she can use Recycle policy or use dynamic provisioning. The only mechanism to get away from Released phase is manual modification of the PV object (assuming that appropriate storage asset is "recycled" too - that's admin decision if data are leaked to subsequent users or not). This can be automated using scripts, of course, but the whole process is external to Kubernetes.

There were some ideas to have a timer on Released PVs - have them to be retained for say 1 week and recycle/delete them automatically after that, so users have one week to change their mind and ask the admin to get the data they released by mistake. This was never implemented as we had other priorities.

@jsafrane you wrote:

If admin wants the PV to be available for binding again, he/she can use Recycle policy or use dynamic provisioning.

It looks like rm -rf is issued as part of the default reycling code, and, unless I've missed something, this would be a disaster for shared (nfs, glusterfs) storage. So Recycle (how I understand it to be implemented) is not an option becuse the data must be preserved. Retain is also not an option because the storage admin wants the pv to be re-claimable at some point. [That point could be triggered by deleting the pv, or by a new command (eg kubectl release pv) -- neither of which works/is-implemented today.] Lastly, dynamic provisioning is not an option for existing, shared storage.

The only mechanism to get away from Released phase is manual modification of the PV object (assuming that appropriate storage asset is "recycled" too

Are you saying kubeclt edit pv? Which field(s) needs to be modified?

Jan, I feel that the use case for 1) preserving data in a shared pv, 2) being able to re-claim a pv that was previously claimed but has since been deleted or somehow released is important in the world of static provisioned, legacy, shared storage. Do you think this would be a worthwhile topic for the next k8s storage community SIG meeting?

Also, I really appreciate your comments/feedback. I know how busy you are and your comments have been helpful!

The only mechanism to get away from Released phase is manual modification of the PV object (assuming that appropriate storage asset is "recycled" too

Are you saying kubeclt edit pv? Which field(s) needs to be modified?

For the old controller, you set pv.Status.Phase=Available and pv.StatusClaimRef=nil and it gets available for binding to anyone. For the new one (in #24331), only pv.StatusClaimRef=nil is enough.

Jan, I feel that the use case for 1) preserving data in a shared pv, 2) being able to re-claim a pv that was previously claimed but has since been deleted or somehow released is important in the world of static provisioned, legacy, shared storage. Do you think this would be a worthwhile topic for the next k8s storage community SIG meeting?

Sure. I'd be interested if this is useful use case. We can implement it, however I am not sure we should and what is priority of this request.

I still did not get an answer how it should work in reality. What objects should a user in project A create/modify/delete to "pass" a PV to project B + what objects should a user in project B create. And how would we prevent user in project C to steal this PV with potentially sensitive data.

@jsafrane you wrote:

For the old controller, you set pv.Status.Phase=Available and pv.StatusClaimRef=nil and it gets available for binding to anyone.

Note: I don't see pv.StatusClaimRef (or pv.Status.ClaimRef) in the kubectl get pv -o yaml output nor in the edit, so instead I commented out the Spec.ClaimRef section.

I did the above edit to a live PV with a pre-bound claimRef and reclaimPolicy=Retain. The matching claim can either be running (status=Pending), or the claim can be deleted and re-created after the pv edit. In both cases, the pv Phase immediately went back to Released but the claim's status is Bound . See:

# kc version
Client Version: version.Info{Major:"1", Minor:"3+", GitVersion:"v1.3.0-alpha.3.194+be21fe786bffcb", GitCommit:"be21fe786bffcb4a6d2e1402d7ed436b35b997c9", GitTreeState:"clean", BuildDate:"2016-05-03T21:52:12Z", GoVersion:"go1.5.3", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"3+", GitVersion:"v1.3.0-alpha.3.194+be21fe786bffcb", GitCommit:"be21fe786bffcb4a6d2e1402d7ed436b35b997c9", GitTreeState:"clean", BuildDate:"2016-05-03T21:52:12Z", GoVersion:"go1.5.3", Compiler:"gc", Platform:"linux/amd64"}

# kc get pvc
NAME         STATUS    VOLUME    CAPACITY   ACCESSMODES   AGE
nfs-claim3   Bound     nfs-pv3   1Gi        RWO           9m
# kc get pv
NAME      CAPACITY   ACCESSMODES   STATUS     CLAIM                REASON    AGE
nfs-pv3   1Gi        RWO           Released   default/nfs-claim3             9m

So, for me, this edit did not work to cause the PV to be re-claimable.

@jsafrane you asked:

how it should work in reality. What objects should a user in project A create/modify/delete to "pass" a PV to project B + what objects should a user in project B create. And how would we prevent user in project C to steal this PV with potentially sensitive data.

Today, if I 1) create a PV which is not pre-bound and has a ReclaimPolicy of Retain, 2) create a matching claim (both are Bound), 3) delete both: I can re-create the pv and its status.Phase is Available. So, for the non-prebinding case the delete action allows the retained pv to become claimable again.

If I do the same except that the pv is pre-bound to the matching claim, the pv is stuck in Released and cannot be claimed again. The only difference is pre-binding vs controller binding. So delete is no longer a trigger to make a pv re-claimable if it was pre-bound.

You've been discussing the concern of making Retained pvs re-claimable (who has access to the data, etc), yet this is exactly what is supported today for controller-bound pvs. My question really is why does pre-binding affect the ability to re-claim a deleted pv?

But to answer your question quoted above: I'd say we need to do exactly what is done today for the controller-binding case. Is that reasonable?

@jsafrane I think my arguments above are not good. I'm coming from the perspective that a PVs' claimRef should not be required to include the target claim's UID -- and this pr tries to address that. If you do include the claim's UID in the pv spec then there is no difference between controller-bound vs. pre-bound PVs in terms of them being re-claimable after being deleted.

@jsafrane given the refactor, should we just close this?

@thockin one aspect of this pr is not requiring the pv to have the uid of the claim in it's claimRef. I'm not sure if this is idea is supported by the community, or if @jsafrane 's pr implements this. From reviewing his code it looks like claim matching (findBestMatchForClaim) will bind even if the pv does not define the claim's uid, but there are uid checks in other parts of the code so I don't know for certain.

Jan's PR implements the "correct" binding. No UID needed if the user specified the binding. I'll close this for now. Once his PR is in, if this doesn't work, please reopen!