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

fog-core 2.x, fog-json 1.x #433

Merged
merged 4 commits into from
Apr 16, 2018
+77 −72
Merged

fog-core 2.x, fog-json 1.x #433

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec620d8
fog-core 2.x, fog-json 1.x
lanej Feb 26, 2018
199cbf8
fix(elb): override #all_associations_and_attributes
lanej Mar 14, 2018
770629e
style(elb): reduce branch conditions and increase readability
lanej Mar 14, 2018
24ea467
fix: remove Volume#server= to attach volumes
lanej Apr 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 14 additions & 27 deletions lib/fog/aws/models/compute/volume.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,25 +59,19 @@ def save
requires :availability_zone
requires_one :size, :snapshot_id

if type == 'io1'
requires :iops
end
requires :iops if type == 'io1'

data = service.create_volume(availability_zone, size, create_params).body
merge_attributes(data)

if tags = self.tags
# expect eventual consistency
Fog.wait_for { self.reload rescue nil }
service.create_tags(
self.identity,
tags
)
Fog.wait_for { service.volumes.get(identity) }
service.create_tags(identity, tags)
end

if @server
self.server = @server
end
attach(@service, device) if @server

true
end
end
Expand All @@ -87,13 +81,7 @@ def server
service.servers.get(server_id)
end

def server=(new_server)
if new_server
attach(new_server)
else
detach
end
end
attr_writer :server

def snapshots
requires :id
Expand All @@ -109,22 +97,15 @@ def force_detach
detach(true)
end

private

def attachmentSet=(new_attachment_set)
merge_attributes(new_attachment_set.first || {})
end

def attach(new_server)
def attach(new_server, new_device)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate on why the new argument is needed? The rest looks good, but I don't think I'm quite groking this. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests asserted this procedure

  • v = service.volumes.create(device: '/dev/sda1', **)
  • v.server = service.instances.create(**) # requires :device

In step 1, a device is provided to a volume instance. In step 2, the model calls Fog.wait_for { ready? } (implicit reload) and then attaches the server.

The issue is between step 1 and 2, there is a property of the local model (device = '/dev/sda1') that does not match the remote model (device = nil).

On reload:

  • fog-core 1.x, the device attribute retained it's value.
  • fog-core 2.x, device is set the correct remote value (nil).

IMO a reload should put the local model in the same state as the remote model. To enforce that, I've removed the ability for volume.server = server to attach volume and instead require calling volume.attach(server, device).

if !persisted?
@server = new_server
self.availability_zone = new_server.availability_zone
elsif new_server
requires :device
wait_for { ready? }
@server = nil
self.server_id = new_server.id
service.attach_volume(server_id, id, device)
service.attach_volume(server_id, id, new_device)
reload
end
end
Expand All @@ -138,6 +119,12 @@ def detach(force = false)
end
end

private

def attachmentSet=(new_attachment_set)
merge_attributes(new_attachment_set.first || {})
end

def create_params
{
'Encrypted' => encrypted,
Expand Down
25 changes: 17 additions & 8 deletions tests/models/compute/volume_tests.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
Shindo.tests("Fog::Compute[:aws] | volume", ['aws']) do

Shindo.tests('Fog::Compute[:aws] | volume', ['aws']) do
@server = Fog::Compute[:aws].servers.create
@server.wait_for { ready? }

model_tests(Fog::Compute[:aws].volumes, {:availability_zone => @server.availability_zone, :size => 1, :device => '/dev/sdz1', :tags => {"key" => "value"}, :type => 'gp2'}, true) do
model_tests(
Fog::Compute[:aws].volumes,
{
availability_zone: @server.availability_zone,
size: 1,
tags: { 'key' => 'value' },
type: 'gp2'
},
true
) do

@instance.wait_for { ready? }

tests('#server = @server').succeeds do
@instance.server = @server
tests('#attach(server, device)').succeeds do
@instance.attach(@server, '/dev/sdz1')
end

@instance.wait_for { state == 'in-use' }
Expand All @@ -17,13 +25,14 @@
@instance.server.id == @server.id
end

tests('#server = nil').succeeds do
(@instance.server = nil).nil?
tests('#detach').succeeds do
@instance.detach
@instance.server.nil?
end

@instance.wait_for { ready? }

@instance.server = @server
@instance.attach(@server, '/dev/sdz1')
@instance.wait_for { state == 'in-use' }

tests('#force_detach').succeeds do
Expand Down