Correct and update disk information #158

djberg96 · 2017-11-03T18:03:55Z

We inadvertently used the host disk (flavor) size when we really wanted to use the OS/data disk size. This updates it so that it only falls back to flavor size if it can't find the OS/data disk size.

In addition, we need to include the disk_type (managed or unmanaged) and storage type (standard LRS, premium, etc) for the mode.

~~https://bugzilla.redhat.com/show_bug.cgi?id=1503866~~
New BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1512569

bronaghs · 2017-11-06T20:23:28Z

I'm not sure we should even fall back to the flavor disk size if that is the maximum size that can be allocated to the OS Disk, not the actual.

djberg96 · 2017-11-06T20:26:04Z

@bronaghs ok, will just default to 0 if it doesn't have the expected property then.

blomquisg · 2017-11-08T19:36:03Z

Looks like this need VCR updates

(I saw WIP, but thought I'd be proactive 😸)

bronaghs · 2017-11-09T16:32:09Z

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

+      end
+
+      def add_azure_instance_disk(array, instance, os_disk = nil)
+        os_disk ||= instance.properties.storage_profile.os_disk



the right side will only parse for OS Disks, can you pass in the OS Disk like you pass in the data disk so this isn't needed?

bronaghs · 2017-11-09T16:50:56Z

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

+              storage_key  = storage_keys['key1'] || storage_keys['key2']
+              blob_props   = storage_acct.blob_properties(container_name, blob_name, storage_key)
+              disk_size    = blob_props.content_length.to_i
+            end


why would disk size be nil? I would love to avoid the complexities around lines 389 - 392

I haven't completely nailed down why some vm's include the disk size in their storage profile, and why some don't, but it must have something to do with the way they were provisioned, or possibly even when they were provisioned. Or it could have something to do with the OS. In any case, I cannot predict it at this time.

You can see the difference by running the following commands on the CLI:

az vm show -n miq-test-win12 -g miq-azure-test1 # This doesn't show a disk size. az vm show -n dberger1 -g dberger1 # This does show a disk size

bronaghs · 2017-11-09T16:54:30Z

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb


-        # No data availbale on swap disk? Called temp or resource disk.
+        array << disk
      end


@djberg96
Until I dug deep, it looked like this method add_azure_instance_disk accepts a data disk as the third parameter yet only parses OS disks. Upon closer review, it looks like data disks will be parsed correctly. Can you rename the osDisk variable to disk to avoid confusion?

Sure thing.

bronaghs · 2017-11-09T16:55:18Z

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

+      end
+
+      def add_azure_instance_disk(array, instance, os_disk = nil)
+        os_disk ||= instance.properties.storage_profile.os_disk


Why did you insert the string azure into the method name? Its implied that this applies to Azure.

I can rename it. I didn't want to use add_instance_disk since it's inherited, but I guess there's no harm in just redefining it.

Yea you could override it.

bronaghs · 2017-11-09T16:56:29Z

app/models/manageiq/providers/azure/refresh_helper_methods.rb

+    filter = "resourceType eq '#{resource_type}'"
+    @res.list_all(:filter => filter)
+  end
+


is this method used?

Oh, I thought I removed it. The original plan was to use the ResourceService since it has a smaller footprint, until I discovered that it did not include the information that I needed.

bronaghs · 2017-11-09T16:57:10Z

app/models/manageiq/providers/azure/refresh_helper_methods.rb

+    ::Azure::Armrest::ResourceService.new(config).tap do |service|
+      service.api_version = Settings.ems.ems_azure.api_versions.resource.to_s
+    end
+  end


is this method used?

Ditto as above, will remove.

djberg96 · 2017-11-09T18:27:45Z

@bronaghs Ok, I think I've made all the requested changes.

bronaghs · 2017-11-09T19:02:52Z

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

+      # Redefine the inherited method for our purposes
+      def add_instance_disk(array, instance, disk = nil)
+        disk ||= instance.properties.storage_profile.os_disk
+


can you pass the OS Disk in as a parameter so its the same as you do for data disks? This line is OS disk specific yet the method is used for all disks.

bronaghs · 2017-11-09T19:04:01Z

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

-        vhd_loc = os_disk.try(:vhd).try(:uri) || os_disk.try(:managed_disk).try(:id)
+        add_instance_disk(hardware_hash[:disks], instance)
+
+        # No data available on swap disk? Called temp or resource disk.


Can you delete this comment?

bronaghs · 2017-11-09T19:11:29Z

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

@@ -350,13 +353,55 @@ def populate_hardware_hash_with_series_attributes(hardware_hash, instance, serie
        hardware_hash[:memory_mb]       = series[:memory] / 1.megabyte
        hardware_hash[:disk_capacity]   = series[:root_disk_size] + series[:swap_disk_size]



Is the disk capacity correct? I do not think we should get this value from the series.

I would have to know the difference between size and disk_capacity. And that's not part of the disks table. It may actually be correct.

bronaghs · 2017-11-09T19:15:32Z

I think, in a separate PR, a disk should be added for a resource disk. Thoughts?
A description of a resource disk:

Resource disk is a locally attached disk, typically an SSD
It is a host disk (or rather part of one on the host depending on the VM size) and is not backed by any storage account. The contents of this drive is lost upon host reallocation. Agent creates a temp drive on it with a warning txt file with this message.

djberg96 · 2017-11-09T19:21:32Z

@bronaghs I'd like to get a storage manager implemented, and we can take a more holistic view of what and how we would like stuff integrated.

bronaghs · 2017-11-09T20:47:04Z

@djberg96 Spoke to PM (Brad Ascar). He recommends adding a setting to the settings.yml file that allows the user to enable or disable setting the tier on unmanaged disks. Similar to how we enable/disable the gathering of public images. The default would be true. A documentation BZ would have to be filed describing this.

bronaghs · 2017-11-09T21:10:53Z

👍
Once commits are squashed I'll merge once green

Added api-version for ResourceService. Added a resource_service and resources_for method. Added add_azure_instance_disk, and modified hardware population methods to use it. Add an api-version for storage_disk. Added the storage_disk_service method. Collect managed disk information and use that to set managed disk size. Get information for unmanaged storage. Add storage type as mode. Remove unused methods. Redefine add_instance_disk instead of adding a new method. Update disk size specs and flavor count. Rebuilt cassette. Make disk argument to add_instance_disk mandatory, update method to pass it explicitly, and remove an old comment. Add the :get_unmanaged_disk_space: property. Only collect unmanaged disk details based on settings.

miq-bot · 2017-11-09T21:28:20Z

Checked commit https://github.com/djberg96/manageiq-providers-azure/commit/106ff47459aa1f2565b9a0dc7f0ac6ef97a92b56 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 3 offenses detected

app/models/manageiq/providers/azure/cloud_manager/refresh_parser.rb

❗ - Line 60, Col 11 - Style/AccessorMethodName - Do not prefix reader method names with get_.
❗ - Line 64, Col 11 - Style/AccessorMethodName - Do not prefix reader method names with get_.

spec/models/manageiq/providers/azure/cloud_manager/refresher_spec.rb

❗ - Line 498, Col 20 - Style/NumericLiterals - Separate every 3 digits in the integer portion of a number with underscores(_).

simaishi · 2017-11-13T23:58:44Z

@djberg96 Tried to backport to Fine branch, but spec/vcr_cassettes/manageiq/providers/azure/cloud_manager/refresher.yml is conflicting. Should I take the yml file from this PR as is, ignoring all the conflicts?

djberg96 · 2017-11-14T00:24:33Z

@simaishi yes, just use the .yml file from this PR please.

Correct and update disk information (cherry picked from commit f8d65fc) https://bugzilla.redhat.com/show_bug.cgi?id=1512728

simaishi · 2017-11-14T01:01:01Z

Fine backport details:

$ git log -1
commit b19e9c1844c16ece5c8bcb352ff15eb06f545e31
Author: Bronagh Sorota <[email protected]>
Date:   Thu Nov 9 16:40:21 2017 -0500

    Merge pull request #158 from djberg96/disk_information
    
    Correct and update disk information
    (cherry picked from commit f8d65fcd1ca49ab286e980d62f2a9ad2d62ae7b2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1512728

Correct and update disk information (cherry picked from commit f8d65fc) https://bugzilla.redhat.com/show_bug.cgi?id=1512727

simaishi · 2017-11-14T23:18:16Z

Gaprindashvili backport details:

$ git log -1
commit e29a5fb43f8c3f55baca86cf8f2d60660a630e26
Author: Bronagh Sorota <[email protected]>
Date:   Thu Nov 9 16:40:21 2017 -0500

    Merge pull request #158 from djberg96/disk_information
    
    Correct and update disk information
    (cherry picked from commit f8d65fcd1ca49ab286e980d62f2a9ad2d62ae7b2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1512727

miq-bot added the wip label Nov 3, 2017

bronaghs suggested changes Nov 9, 2017

View reviewed changes

djberg96 changed the title ~~[WIP] Correct and update disk information~~ Correct and update disk information Nov 9, 2017

miq-bot removed the wip label Nov 9, 2017

bronaghs reviewed Nov 9, 2017

View reviewed changes

bronaghs approved these changes Nov 9, 2017

View reviewed changes

bronaghs self-assigned this Nov 9, 2017

bronaghs merged commit f8d65fc into ManageIQ:master Nov 9, 2017

bronaghs added this to the Sprint 73 Ending Nov 13, 2017 milestone Nov 9, 2017

bronaghs added bug enhancement fine/yes labels Nov 9, 2017

simaishi pushed a commit that referenced this pull request Nov 14, 2017

Merge pull request #158 from djberg96/disk_information

b19e9c1

Correct and update disk information (cherry picked from commit f8d65fc) https://bugzilla.redhat.com/show_bug.cgi?id=1512728

simaishi added the fine/backported label Nov 14, 2017

simaishi removed the fine/yes label Nov 14, 2017

simaishi pushed a commit that referenced this pull request Nov 14, 2017

Merge pull request #158 from djberg96/disk_information

e29a5fb

Correct and update disk information (cherry picked from commit f8d65fc) https://bugzilla.redhat.com/show_bug.cgi?id=1512727

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct and update disk information #158

Correct and update disk information #158

djberg96 commented Nov 3, 2017 •

edited by simaishi

Loading

bronaghs commented Nov 6, 2017

djberg96 commented Nov 6, 2017

blomquisg commented Nov 8, 2017 •

edited

Loading

bronaghs Nov 9, 2017

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017

bronaghs Nov 9, 2017

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017 •

edited

Loading

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017

djberg96 commented Nov 9, 2017

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017

bronaghs Nov 9, 2017

djberg96 Nov 9, 2017

bronaghs commented Nov 9, 2017

djberg96 commented Nov 9, 2017

bronaghs commented Nov 9, 2017

bronaghs commented Nov 9, 2017 •

edited

Loading

miq-bot commented Nov 9, 2017

simaishi commented Nov 13, 2017

djberg96 commented Nov 14, 2017

simaishi commented Nov 14, 2017

simaishi commented Nov 14, 2017

		@@ -350,13 +353,55 @@ def populate_hardware_hash_with_series_attributes(hardware_hash, instance, serie
		hardware_hash[:memory_mb] = series[:memory] / 1.megabyte
		hardware_hash[:disk_capacity] = series[:root_disk_size] + series[:swap_disk_size]

Correct and update disk information #158

Correct and update disk information #158

Conversation

djberg96 commented Nov 3, 2017 • edited by simaishi Loading

bronaghs commented Nov 6, 2017

djberg96 commented Nov 6, 2017

blomquisg commented Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djberg96 Nov 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djberg96 commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bronaghs commented Nov 9, 2017

djberg96 commented Nov 9, 2017

bronaghs commented Nov 9, 2017

bronaghs commented Nov 9, 2017 • edited Loading

miq-bot commented Nov 9, 2017

simaishi commented Nov 13, 2017

djberg96 commented Nov 14, 2017

simaishi commented Nov 14, 2017

simaishi commented Nov 14, 2017

djberg96 commented Nov 3, 2017 •

edited by simaishi

Loading

blomquisg commented Nov 8, 2017 •

edited

Loading

djberg96 Nov 9, 2017 •

edited

Loading

bronaghs commented Nov 9, 2017 •

edited

Loading