-
Notifications
You must be signed in to change notification settings - Fork 251
fix: adopt restored pvc in instanceset #9882
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
Conversation
|
Auto Cherry-pick Instructions |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9882 +/- ##
==========================================
+ Coverage 50.77% 52.54% +1.77%
==========================================
Files 541 543 +2
Lines 58733 60784 +2051
==========================================
+ Hits 29820 31940 +2120
+ Misses 25967 25855 -112
- Partials 2946 2989 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| pvcObj := copyAndMerge(oldPvc, pvc) | ||
| if pvcObj != nil { | ||
| // pvc can be created by restore controller, restore controller reference for it | ||
| if err := controllerutil.SetControllerReference(its, pvcObj, model.GetScheme()); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot implicitly take over the PVC's controller Reference at the implementation level, unless there is a clear interface in the API indicating it's allowed.
A hacking behavior is even less acceptable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, consider a scenario where this PVC is created and managed by the user. What would happen then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot implicitly take over the PVC's controller Reference
We can't. If the PVC already has a controller reference, SetControllerReference returns an error. Maybe we can simply ignore this error. Or we can fallback to set an owner reference to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there is no instanceset at the time of restore phase, we cannot create ownerReference. Can we add an annotation on PVC to indicate that it can be taken over by instanceSet?
|
This PR is a temporary solution. Further API discussion will happen in #9974 |
|
/cherry-pick release-1.0 |
|
🤖 says: cherry pick action finished successfully 🎉! |
Co-authored-by: Leon <[email protected]> (cherry picked from commit 8217833)
fixes #9841
Because of the missing ownerRefernece, its controller does not get the event when the pvc gets deleted.