-
Notifications
You must be signed in to change notification settings - Fork 453
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
feat: Add support for RDM disks #1366
base: main
Are you sure you want to change the base?
Conversation
Hello @bill-rich @koikonom , |
Hi Sumit. Thank you for your contribution, as always. I've added your request to our backlog. We have a few items ahead of this right now, but we'll start looking at it in the near future. |
Any update on when this might get looked at? |
I have taken the liberty of rebasing and making necessary changes to this PR. The provided acceptance test seems acceptable, however it's not an environment/feature we have the ability to test at the moment. In general, testing homelab specific/hardware related features is not a scalable activity for us. So after making some final changes to this PR we will be asking the community to manually test the feature for us. |
After further inspection, one thing I am struggling with on the PR is the awkward polymorphic behavior between the 2 disk types. Some things are nil for one type and not the other (and vice versa if we were to completely expand out an expose all the attributes of RDM). Are these the only 2 types of disk? (asking @tenthirtyam or any vsphere experts) If not, then it's a recipe for problems if there are eventually many "backings". If there were only ever 2, I could clean this up and live with it. UPDATE: looking through govmomi indicates there are several possible backings. For this resource to scale something more granular is going to be needed I think. |
@appilon - agreed. Perhaps we could discuss this enhancement in detail early in New Year with the intended planning and collaboration. We can invite Sumit and also Michael/Doug from our |
@tenthirtyam Coming back to this, the best way forward I believe is introducing a new nested block
Subsequent disk types added later on will also have a separate block definition. We will also rename the existing |
I will go ahead and tweak this PR to that implementation, from there, as stated earlier, I'll ask the community to test the new disk type, as we do not have the capacity to test this type of disk. |
@sumitAgrawal007 @tenthirtyam After digging into this PR further, it's unclear to me how the RDM disk relevant data made its way to the backend (vsphere)? Specifically the compatibility mode, after looking into it further, I don't think this provider can support "physical" mode, logically it will be too difficult since the existing disk resource supports the features of vmdk (cloning specifically). I am not a vmware expert so I'm uncertain on if it's even safe in "virtual" mode (quick glance seems like it is?). If we can't support physical mode at this time, is this feature still worth pursuing? Or should we wait until we can support physical mode? |
Marking as draft due to merge conflicts that need to be resolved and open concerned regarding implementation. Ryan Johnson |
I think this is needed... It would be nice to have it in the provider. RDM is quite useful, if not implemented pls. ignore it. |
Marking this pull request as stale due to inactivity in the past 180 days. This helps us focus on the active pull requests. If this pull request receives no comments in the next 30 days it will automatically be closed. |
Any progress on this PR? This is indeed useful! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
This PR adds code to enable creating VM or adding into VM, RDM disks.
Acceptance tests
Output from acceptance testing:
Link to Binary to try out this feature.
Release Note
Release note for CHANGELOG:
References
#397