Support non-boot disk

[whynick1]

Based on the discuss here vagrant-google, to allow mounting additional (non-boot) disks, we need to make some change accordingly.

Could you take a look at this patch? @Temikus

[criccomini]

NOTE: This is for mitchellh/vagrant-google#102

[criccomini]

ping @Temikus .. was hoping to get this in quickly per discussion on other GH issue.

[Temikus]

@criccomini Sorry for the delay, this is a free time project and it’s been a very busy week. I’ll address this Saturday AEDT

[criccomini]

Thanks!

[codecov]

Codecov Report

Merging #434 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   84.94%   84.92%   -0.02%     
==========================================
  Files         339      339              
  Lines        5825     5826       +1     
==========================================
  Hits         4948     4948              
- Misses        877      878       +1

Impacted Files	Coverage Δ
lib/fog/compute/google/models/disk.rb	82.08% <50%> (-0.99%)	⬇️
lib/fog/storage/google_json/requests/get_object.rb	90.47% <0%> (-0.44%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0713914...39596bd. Read the comment docs.

[Temikus]

Overall LGTM. See 2 small comments.

[Temikus]

auto_delete and autoDelete should be interchangeable in google-api-client.

Or does auto_delete not function correctly anymore?

[whynick1]

Good catch! Changed to auto_delete .

[Temikus]

Since we already have disks.get, I think it'll make more sense to name this get_as_hash or something similar to avoid confusion, WDYT?

[whynick1]

As there is a get_as_boot_disk, how about get_as_disk?

[Temikus]

@criccomini @whynick1 Ok, looking closer this is already implemented in the lib. The method seems to have just moved to collection method attached_disk_obj

Example:

[6] pry(#<TestDisks>)> disk.get_as_boot_disk
=> {:auto_delete=>false,
 :boot=>true,
 :source=>"https://www.googleapis.com/compute/v1/projects/graphite-sandbox/zones/us-central1-f/disks/fog-test-1-testdisks-test-get-as-configs",
 :mode=>"READ_WRITE",
 :type=>"PERSISTENT"}
[7] pry(#<TestDisks>)> disk.attached_disk_obj(boot:true,writable:true)
=> {:auto_delete=>false,
 :boot=>true,
 :mode=>"READ_WRITE",
 :source=>"https://www.googleapis.com/compute/v1/projects/graphite-sandbox/zones/us-central1-f/disks/fog-test-1-testdisks-test-get-as-configs",
 :type=>"PERSISTENT"}
[8] pry(#<TestDisks>)> assert_equal(disk.get_as_boot_disk, disk.attached_disk_obj(boot:true,writable:true))
=> true

This is probably what you're looking for, right?

I'll be in my shame cube...

[whynick1]

Hi @Temikus , sorry for the late reply as I was on my vocation😅

I have updated PR regarding your comment. Thanks!

For disk.attached_disk_obj, based on what you provided, looks it still cannot return a non-boot disk.
I think a get_as_disk is what we need according to vagrant-google
cc @criccomini

[Temikus]

@whynick1 No worries!

So, I may be misunderstanding something here but I double-checked and am able to create an instance with an attached non-boot disk using this method.

Example code:

  connection = Fog::Compute.new(:provider => "Google")

  disk = connection.disks.create(
    :name => "fog-smoke-test-#{Time.now.to_i}",
    :size_gb => 10,
    :zone => "us-central1-f",
    :source_image => "debian-9-stretch-v20180611"
  )

  attached_disk = connection.disks.create(
      :name => "fog-smoke-test-#{Time.now.to_i}",
      :size_gb => 10,
      :zone => "us-central1-f",
  )

  disk.wait_for { ready? }
  attached_disk.wait_for { ready? }

  server = connection.servers.create(
    :name => "fog-smoke-test-#{Time.now.to_i}",
    :disks => [disk.attached_disk_obj(boot: true),
               attached_disk.attached_disk_obj(boot: false)],
    :machine_type => "n1-standard-1",
    :private_key_path => File.expand_path("~/.ssh/id_rsa"),
    :public_key_path => File.expand_path("~/.ssh/id_rsa.pub"),
    :zone => "us-central1-f",
    :network_interfaces => [{ :network => "global/networks/default",
                              :access_configs => [{
                                :name => "External NAT",
                                :type => "ONE_TO_ONE_NAT"
                              }] }],
    :username => ENV["USER"]
  )

Return:

server.disks
=> [{:auto_delete=>false,
  :boot=>true,
  :mode=>"READ_WRITE",
  :source=>"https://www.googleapis.com/compute/v1/projects/graphite-sandbox/zones/us-central1-f/disks/fog-smoke-test-1546488803",
  :type=>"PERSISTENT",
  :device_name=>"persistent-disk-0",
  :guest_os_features=>[{:type=>"VIRTIO_SCSI_MULTIQUEUE"}],
  :index=>0,
  :interface=>"SCSI",
  :kind=>"compute#attachedDisk",
  :licenses=>["https://www.googleapis.com/compute/v1/projects/debian-cloud/global/licenses/debian-9-stretch"]},
 {:auto_delete=>false,
  :boot=>false,
  :mode=>"READ_WRITE",
  :source=>"https://www.googleapis.com/compute/v1/projects/graphite-sandbox/zones/us-central1-f/disks/fog-smoke-test-1546488937",
  :type=>"PERSISTENT",
  :device_name=>"persistent-disk-1",
  :index=>1,
  :interface=>"SCSI",
  :kind=>"compute#attachedDisk"}]

Or am I missing something here?

[whynick1]

@Temikus You are right! 👍 disk.attached_disk_obj would work! I think we can close this PR.

Meanwhile, I made some change to vagrant-google to support additional_disks option, please refer to this PR, mitchellh/vagrant-google@c397202

Please let me know what you think! Thanks!

cc @criccomini

[Temikus]

@whynick1 Great! I'll close out this PR in favor of the vagrant one.