theforeman / foreman_azure_rm

Adds Azure Resource Manager as a compute resource for The Foreman
GNU General Public License v3.0
9 stars 24 forks source link

Fixes #32270 - Implement custom size for OS disk #108

Closed eLvErDe closed 3 years ago

eLvErDe commented 3 years ago

Working just fine here, unless so specify a size smaller than image's one but that's expected

eLvErDe commented 3 years ago

Hi @chris1984

Thanks, an improvment might be doable but initializing the field with information extract from the image itself, but I'm not sure if it possible and if it worth it. Leaving the form empty will pass nil to underlying SDK and keep previous behavior while putting a positive integer override the OS disk size.

eLvErDe commented 3 years ago

Another improvement: being able to store the overridden size from a compute profiles. My new form option does show up here, which was not expected (is it possible to hive it from here? seems form code is shared) but it's useless as setting the value here won't persist it into Foreman's database. I have absolutely no idea if it could be saved into DB from here but I guess it should be possible because it work for example with write cache option.

Let me know if you're interested in getting this implemented but I'll need a hand.

chris1984 commented 3 years ago

Will test this today for sure, sorry for the delay, I had to leave early yesterday for an appointment.

eLvErDe commented 3 years ago

Don't worry I wouldn't have had any minute to answer today anyway

chris1984 commented 3 years ago

Before:

https://nimb.ws/ZjiYsn

After:

https://nimb.ws/IGRtEM

Looks good, so far. Will ack and merge it tomorrow, just finishing up some testing to make sure it does not break anything in the current workflows.

chris1984 commented 3 years ago

Another improvement: being able to store the overridden size from a compute profiles. My new form option does show up here, which was not expected (is it possible to hive it from here? seems form code is shared) but it's useless as setting the value here won't persist it into Foreman's database. I have absolutely no idea if it could be saved into DB from here but I guess it should be possible because it work for example with write cache option.

Let me know if you're interested in getting this implemented but I'll need a hand.

There is some code overlap between the compute_profile page and the virtual_machine tab in the Create Host page. I can link you an example of how we store it in the database. We can do it in this pr or if you want to open another. The main thing will prob be a database migration in this plugin to account for the new value so we can save it in the database.

eLvErDe commented 3 years ago

Hello,

I would definitely be interested in implementing this. I'll have some time to work on it on Tuesday/Wednesday if you can provide some hints

eLvErDe commented 3 years ago

@chris1984 any hint to implement such feature ?

chris1984 commented 3 years ago

So, looking into how the attributes are stored I am seeing this, so we don't need a database migration :)

change is sent to the compute attributes controller:

https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/compute_attributes_controller.rb

This updates the compute_attributes table in the DB, with the info in a hash as vm_attrs

https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/compute_attributes_controller.rb#L12

This is from my devel box with an Azure compute resource:

katello=# select vm_attrs from compute_attributes;
                                                                                  vm_attrs
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 --- !ruby/hash:ActiveSupport::HashWithIndifferentAccess                                                                                                                   +
 resource_group: RHSatellite                                                                                                                                               +
 vm_size: Standard_B1s                                                                                                                                                     +
 platform: Linux                                                                                                                                                           +
 username: azureuser                                                                                                                                                       +
 password: Redhat1!                                                                                                                                                        +
 ssh_key_data: ''                                                                                                                                                          +
 premium_os_disk: 'false'                                                                                                                                                  +
 os_disk_size_gb: ''                                                                                                                                                       +
 os_disk_caching: ''                                                                                                                                                       +
 script_command: ''                                                                                                                                                        +
 script_uris: ''                                                                                                                                                           +
 interfaces_attributes: !ruby/hash:ActiveSupport::HashWithIndifferentAccess                                                                                                +
   '1615831448291': !ruby/hash:ActiveSupport::HashWithIndifferentAccess                                                                                                    +
     network: "/subscriptions/f5877eb3-9016-4ddd-80b1-3657c9fdbed8/resourceGroups/RHSatellite/providers/Microsoft.Network/virtualNetworks/RHSatellite-vnet/subnets/default"+
     public_ip: Dynamic                                                                                                                                                    +
     private_ip: 'false'                                                                                                                                                   +

Let me know if that is enough to start with, if not I can look more exactly it is where we we send that off to Foreman from within the plugin.

Sorry for the delay, yesterday when I was looking through some stuff Azure AD had an outage so my devel box would not connect, go figure the time i start to look into this Microsoft has an outage.

eLvErDe commented 3 years ago

Hello,

Confirmed, the value is actually saved but, not displayed when creating the VM from the profile. Any idea ?

eLvErDe commented 3 years ago

Okay so, this issue is in azure_rm_compute.rb file, I need to create a getter returning default value, most of other options get their default from the image definition itself like:

    def os_disk_caching
      @azure_vm.storage_profile.os_disk.caching
    end

(btw I'm not sure this is correct because it's very likely to overwrite what has been defined in the compute profile imho)

For defaut size I just return nil:

    def os_disk_size_gb
    end

How can I access the actual selected compute profile from here ? Any idea ?

Adam.

chris1984 commented 3 years ago

@eLvErDe let me try a few things to see how to access the compute profile, I will test your updated commit too.

eLvErDe commented 3 years ago

It works now, with the commit I just added :)

chris1984 commented 3 years ago

Awesome, ok will test it and we can get this merged :)

chris1984 commented 3 years ago

ACK, works great! Picture of the Virtual_Machine tab keeping the value that I set in the compute_profile

https://nimb.ws/wsECht

Here is the output from the database compute_attributes table showing the value is in the hash.

katello=# select * from compute_attributes;
 id | compute_profile_id | compute_resource_id |         name         |
                                 vm_attrs
   |         created_at         |         updated_at
----+--------------------+---------------------+----------------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------+----------------------------+----------------------------
  1 |                  2 |                   1 | Standard_B1s VM Size | --- !ruby/hash:ActiveSupport::HashWithIndifferentAccess
  +| 2021-03-16 18:14:06.108363 | 2021-03-16 18:14:06.108363
    |                    |                     |                      | resource_group: RHSatellite

  +|                            |
    |                    |                     |                      | vm_size: Standard_B1s

  +|                            |
    |                    |                     |                      | platform: Linux

  +|                            |
    |                    |                     |                      | username: azureuser

  +|                            |
    |                    |                     |                      | password: Redhat1!

  +|                            |
    |                    |                     |                      | ssh_key_data: ''

  +|                            |
    |                    |                     |                      | premium_os_disk: 'false'

  +|                            |
    |                    |                     |                      | os_disk_size_gb: '20'                                                                                                                                                     +|                            |
    |                    |                     |                      | os_disk_caching: ''                                                                                                                                                       +|                            |
    |                    |                     |                      | script_command: ''                                                                                                                                                        +|                            |
    |                    |                     |                      | script_uris: ''                                                                                                                                                           +|                            |
    |                    |                     |                      | interfaces_attributes: !ruby/hash:ActiveSupport::HashWithIndifferentAccess                                                                                                +|                            |
    |                    |                     |                      |   '1615918427146': !ruby/hash:ActiveSupport::HashWithIndifferentAccess                                                                                                    +|                            |
    |                    |                     |                      |     network: "/subscriptions/f5877eb3-9016-4ddd-80b1-3657c9fdbed8/resourceGroups/RHSatellite/providers/Microsoft.Network/virtualNetworks/RHSatellite-vnet/subnets/default"+|                            |
    |                    |                     |                      |     public_ip: Dynamic                                                                                                                                                    +|                            |
    |                    |                     |                      |     private_ip: 'false'                                                                                                                                                   +|                            |
    |                    |                     |                      |                                                                                                                                                                            |                            |