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.

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

BZ #20538 - add compute profile commands #398

Merged
merged 6 commits into from Feb 26, 2019

Conversation

Projects
None yet
4 participants
@shiramax
Copy link
Contributor

commented Nov 22, 2018

No description provided.

@shiramax

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

@mbacovsky I created a new PR for compute profile as you requested.
Thanks :)

@mbacovsky
Copy link
Member

left a comment

@shiramax first of all thanks for this new attempt. I think we can go with the nested attribute commands as it is set now. I left a few comments with local impact inline. There are two things that we should solve before this can go public. First is consistency of how we call things on CLI and in the code. Second is how we extend the help in the commands. I'll describe both in more detail:

Consistency:
It seems it is not very clear how some objects are called and it causes pain. It is partly caused by unfortunate naming of the things in the core model and too strict following it in the API and CLI. I noticed mix of 'attributes', 'values' and 'properties' in profiles which makes the command options and method names a bit unpredictable. E.g.

$ hammer compute-profile attribute create --compute-profile-id 3 --compute-resource-id 1 --compute-attributes cpus=4
$ hammer compute-profile attribute add-volume --compute-profile-id 3 --compute-resource-id 1 --set-value capacity=8G

I think we should define some name scheme and stick to it. Here is my take on this:

  • compute profile the name of the compute profile, handled by compute-profile command
  • compute attributes compute profile content for specific compute attribute. It consists of three type of attributes - machine/root/main(?) attributes, interface attributes, volume attributes
  • properties - each kind of attributes has its properties (specific per compute resource)
  • attribute values - hash of property=value for the specific attribute type

With this naming I would expect the previous commands to become:

$ hammer compute-profile attribute create --compute-profile-id 3 --compute-resource-id 1 --values cpus=4
$ hammer compute-profile attribute add-volume --compute-profile-id 3 --compute-resource-id 1 --values capacity=8G

Not a big change but the interface is cleaner and more predictable. It should also help naming things in the code. Does it make sense?

Help extensions:
I noticed some circular deps between the modules and also it is not clear what is which module responsibility.

I think we need to clean up the design of this feature and redesign the original HostHelpExtension.
What is currently in ./compute_resources/* should be lightweight independent definition of things specific for respective compute_resource. I think we should end up with HammerCLIForeman.compute_resources:

{
  'ovirt' => HammerCLIForeman::ComputeResources::OVirt,
  'libvirt' => ...
}

Where each of the objects would be subclass of the Base class

class Base
   attr_reader :name
   def machine_properties; []; end
   def interface_properties; []; end
   def volume_properties; []; end
end

In the subclass the the properties are defined

def volume_properties
  [
                   ['size_gb',           _('Volume size in GB, integer value')],
                   ['storage_domain',    _('Storage Domain ID, Select one of available storage domains')],
                   ['preallocate',       _('Boolean, (expressed as 0 or 1) preallocate=1,  thin provisioning=0')],
                   ['wipe_after_delete', _('Boolean, (expressed as 0 or 1)')],
                   ['interface',         _('Name of Disk Interface')],
                   ['bootable',          _('Boolean (expressed as 0 or 1), only one volume can be bootable')]
  ]
end

Then we could extend the help in the command like:

      extend_help do |h|
        h.section _('Provider specific options') do |h|
          ::HammerCLIForeman.compute_resources.each do |name, provider|
            h.section name do |h|
               h.section '--volume' do |h|
                  h.list(provider.volume_properties)
               end
            end
          end
        end

Which may be subject of further refactoring if it makes sense. This way the compute profile does not have to know anything about neither the command nor the help and we can untangle the circular deps.
What do you think about such design?

Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated
Show resolved Hide resolved lib/hammer_cli_foreman/attribute.rb Outdated

@shiramax shiramax force-pushed the shiramax:new_compute_profile branch from cf11793 to 896b7fe Dec 17, 2018

@shiramax shiramax force-pushed the shiramax:new_compute_profile branch from 896b7fe to 40301c1 Dec 18, 2018

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

@shiramax thanks for this update. I like how you reworked the compute resources. It looks much cleaner now. there are some minor comments inline.

I played a bit with the --interface and --volume params and I think we should make it the same way as in host create/update commands. So to keep --interface and --volume and keep them multivalued. I think it does not make sense to keep the --volume-id and --interface-id because it changes how the --interface parameter work and is confusing.

It seems we are almost there, could you please rebase?

@shiramax shiramax force-pushed the shiramax:new_compute_profile branch from 40301c1 to 4cc48cd Dec 20, 2018

@shiramax

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

@mbacovsky thanks, please review again

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Jan 6, 2019

@shiramax the value create and update options looks good now. I'll do some more testing.
It seems you've missed my comments about registering of the compute resources from the resource definition. Is the comment and reasoning clear?
Could you please add the updates in separate commits? It is much easier for me to review the changes. Thanks.

@shiramax

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

@mbacovsky, hey thanks for the review.
Sorry for missing your comment about registering the key, I thought that I can fix the problem with recreating @compute_resources hash each time in a different way.
But I will try to do the register part, the problem is that i'm not sure where the function register_compute_resource should be, should it be in hammer-cli-foreman/lib/hammer_cli_foreman/compute_resources.rb, if i think it will cause problems in the required files.
thanks.

@shiramax shiramax force-pushed the shiramax:new_compute_profile branch from 4cc48cd to 53059ec Jan 17, 2019

@pondrejk

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Hi @shiramax, overall it looks good. I'm hitting the same issue as Martin with "values update" wiping other settings, I can add that it does not happen with "values update-interface" or "values update-volume". BTW when I attempt to update nonexistent interface hammer creates it, not sure if bug or feature :)

Otherwise I made some comments inline, there are some problems with help generation, probably due to requires. And we should check that key=values are correctly named for all compute resources, I was able to check just libvirt and rhev.

Hope this helps

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

[test hammer]

@shiramax shiramax force-pushed the shiramax:new_compute_profile branch 3 times, most recently from e1db275 to b2dc021 Feb 13, 2019

@shiramax

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2019

@mbacovsky please review again :)

@shiramax shiramax force-pushed the shiramax:new_compute_profile branch from b2dc021 to 4fcc38e Feb 21, 2019

@shiramax shiramax force-pushed the shiramax:new_compute_profile branch from b2eaff1 to f2b104b Feb 25, 2019

@@ -37,6 +37,6 @@ def volume_attributes;
]
end
end
HammerCLIForeman.register_compute_resource('ovirt', Ovirt.new)
HammerCLIForeman.register_compute_resource('oVirt', Ovirt.new)

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Feb 25, 2019

Member

As it is not sometimes very clear how the compute resources are named we should use the names in lowercase as it was originally.

@@ -1,11 +1,17 @@
module HammerCLIForeman
@compute_resources = {}
@interfaces_attrs_name_list ={}

This comment has been minimized.

Copy link
@mbacovsky

mbacovsky Feb 25, 2019

Member

What benefit it has to store this duplicate information in here? I'd prefer to not add to top lavel namespace more things then it is necessary.

HammerCLIForeman.compute_resources['ovirt'].interfaces_attrs_name

feels readable enough.

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@shiramax thanks a lot for the last update. It finally works as expected for my libvirt 🍾! It is good to go, well almost. There is one thing I'd prefer not to merge now - syntax error in common_update_help.rb which is preventing rendering host help. See comment inline.

I'm pretty happy with the rest. There are some other minor things that reserve refactoring that I would agree to merge as technical debt to unblock your kubevirt work as we discussed but also would prefer to have it fixed rather sooner then later. All are quite small things so it may be actually less work to fix them right away then creating a new PR so let me know your preference. All are marked inline but the list is:

  • attribute_test.rb was not renamed
  • interfaces_attrs_name_list helper method in HammerCLIForeman does not provide much value and we are duplicating data there
  • I'd prefer to identify the compute resources with lowercase names to avoid issues with inconsistent naming (Ovirt vs. oVirt)
@shiramax

This comment has been minimized.

Copy link
Contributor Author

commented Feb 26, 2019

@mbacovsky thanks!
I've changed the syntax error and renamed the test file.
I think we can merge this PR, and I will open new PRs for your other comments.

thanks!

@mbacovsky

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

@shiramax thanks for the update! I'm going to merge this. The two missing things will be resolved in separate PR. Thanks for your hard work and getting it to this point. 🍰

@mbacovsky mbacovsky merged commit 6233ac6 into theforeman:master Feb 26, 2019

1 check passed

hammer Build finished. 5232 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.