Skip to content
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

UPSTREAM: 23078: Check claimRef UID when processing a recycled PV #8100

Merged
merged 2 commits into from Apr 5, 2016
Merged

UPSTREAM: 23078: Check claimRef UID when processing a recycled PV #8100

merged 2 commits into from Apr 5, 2016

Conversation

ghost
Copy link

@ghost ghost commented Mar 17, 2016

@ghost ghost mentioned this pull request Mar 17, 2016
Closed
85 tasks
@childsb
Copy link
Contributor

childsb commented Mar 18, 2016

[test]

2 similar comments
@ghost
Copy link
Author

ghost commented Mar 21, 2016

[test]

@ghost
Copy link
Author

ghost commented Mar 22, 2016

[test]

@ncdc
Copy link
Contributor

ncdc commented Mar 24, 2016

This has NOT merged upstream yet

@liggitt liggitt changed the title UPSTREAM: 23078: Check claimRef UID when processing a recycled PV HOLD FOR UPSTREAM: 23078: Check claimRef UID when processing a recycled PV Mar 24, 2016
@ncdc
Copy link
Contributor

ncdc commented Mar 25, 2016

Upstream has merged.

@ncdc ncdc changed the title HOLD FOR UPSTREAM: 23078: Check claimRef UID when processing a recycled PV UPSTREAM: 23078: Check claimRef UID when processing a recycled PV Mar 25, 2016
@ncdc
Copy link
Contributor

ncdc commented Mar 28, 2016

@liggitt merge?

@ncdc ncdc changed the title UPSTREAM: 23078: Check claimRef UID when processing a recycled PV [DO NOT MERGE] UPSTREAM: 23078: Check claimRef UID when processing a recycled PV Mar 28, 2016
@ncdc
Copy link
Contributor

ncdc commented Mar 28, 2016

Need to fix a potential issue w/the upstream PR. Once that's ready, I'll close this and submit a replacement.

@ghost
Copy link
Author

ghost commented Mar 29, 2016

@ncdc whats the issue upstream ?

@liggitt
Copy link
Contributor

liggitt commented Mar 29, 2016

the code assumes a non nil claim is usable without checking if there is a non nil error

@ghost
Copy link
Author

ghost commented Mar 29, 2016

the code assumes a non nil claim is usable without checking if there is a non nil error

Hmm.. I don't think I follow

@ncdc
Copy link
Contributor

ncdc commented Mar 29, 2016

@swagiaal with this patch the way it's currently coded, you could potentially get back an error that isn't "not found" and a non-nil claim. It's more appropriate to do this:

  1. If we got an error and it's not a "not found" error, return the error
  2. Otherwise, if the error we got was "not found" or the claim's UID is different, make the volume available again

See kubernetes/kubernetes#23548 for what I did

UPSTREAM: 23548: Check claimRef UID when processing a recycled PV, ta…
8cbb8af 
…ke 2

Reorder code a bit so it doesn't allow a case where you get some error other than "not found"
combined with a non-nil Claim.

Add test case.
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 8cbb8af

@ncdc ncdc changed the title [DO NOT MERGE] UPSTREAM: 23078: Check claimRef UID when processing a recycled PV UPSTREAM: 23078: Check claimRef UID when processing a recycled PV Apr 5, 2016
@ncdc
Copy link
Contributor

ncdc commented Apr 5, 2016

LGTM. @smarterclayton merge?

@smarterclayton
Copy link
Contributor

Do you approve?

@ncdc
Copy link
Contributor

ncdc commented Apr 5, 2016

Yes. @childsb please sign off.

@childsb childsb added the lgtm Indicates that a PR is ready to be merged. label Apr 5, 2016
@childsb
Copy link
Contributor

childsb commented Apr 5, 2016

looks good, added the LGTM label

@smarterclayton
Copy link
Contributor

Approved [merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5515/) (Image: devenv-rhel7_3909)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 8cbb8af

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2695/)

@openshift-bot openshift-bot merged commit e56dfcb into openshift:master Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@childsb @ncdc @liggitt @openshift-bot @smarterclayton