Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Changes the disk schema from TypeSet to TypeList #115

Closed
wants to merge 2 commits into from

Conversation

r2bit
Copy link

@r2bit r2bit commented Jul 31, 2017

My rather crude way to fix (or rather change the behaviour) #64.

@grubernaut grubernaut added the bug Type: Bug label Jul 31, 2017
@catsby
Copy link

catsby commented Jul 31, 2017

For users with existing disks, what do they get when they upgrade to this new code? Do they get a diff in their plan?

@catsby catsby added the waiting-response Status: Waiting on a Response label Jul 31, 2017
@girishramnani
Copy link

@catsby probably not and as this is a schema change implementing a StateMigrateFunc could be required to support users who already have resources.

More about MigrateFunc is in Source Code

@r2bit
Copy link
Author

r2bit commented Aug 1, 2017

@catsby Unfortunately it will already crash in the planning state - it will attempt to read the old TypeSet state in as TypeList (as defined in the schema change) and crash on nil. The crash itself can be quite easily handled, but it does not give a smooth migration path, still.. I will look into StateMigrateFunc, although at first glance it does not look like it would help, since i'm not changing elements, but the schema itself.

@girishramnani
Copy link

@r2bit according to the doc of migrateFunc

// MigrateState is responsible for updating an InstanceState with an old
// version to the format expected by the current version of the Schema.

The function is responsible for transforming the local state ( which was created using older schema ) to the newer state ( newer schema ). This will be called before refresh and hence the local state will be updated before terraform reaches the ResourceRead method.

So I think StateMigrateFunc could solve the problem.

@r2bit
Copy link
Author

r2bit commented Aug 1, 2017

@girishramnani Thanks, I'll give it a shot in the evening :)

@grubernaut grubernaut removed the waiting-response Status: Waiting on a Response label Aug 1, 2017
@r2bit
Copy link
Author

r2bit commented Aug 1, 2017

I added migration. However, there's still one open issue: how to handle reordering? Reorder disks on VM by force (i.e. detach and attach all unordered disks) or simply ignore the positions of existing disks.
The current solution will do the latter to avoid any data loss during forceful disk detach (and FS unmount by extension). But i feel that is not the correct approach - TF state should reflect the exact state after successful apply (?)

@vancluever
Copy link
Contributor

Hey all,

This seems to be a rather old PR (sorry about that), and how has been largely superseded by #244. The rewrite does not move disks to lists, but rather exposes a unit_number attribute that can be used to set the unit number directly on the SCSI bus, allowing for 60 disks to possibly be attached across 4 SCSI controllers.

The only real issue remaining with disks still being sets are that diffs are rather ugly as we don't have any way of intelligently lining up sets in a diff in TF core. However, I think this was not necessarily an objective of this PR as it seems to attempt to address #64, which unit_number should sufficiently do.

I still want to explore moving disks to lists at a later date to address the diff user experience issues, however for v1.0.0, we will be sticking with sets, as order has been addressed now and we do have documentation on how to read the disk diff in the new version of the docs, which should help mitigate any confusion.

Closing this now considering all of the above. Thanks for all the effort on this!

@vancluever vancluever closed this Nov 18, 2017
@ghost ghost locked and limited conversation to collaborators Apr 19, 2020
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
bug Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants