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

Fixes #25584 - Improve help for compute resources #427

Merged
merged 1 commit into from Aug 27, 2019

Conversation

@ofedoren
Copy link
Member

ofedoren commented Jul 16, 2019

The issue actually is about showing unclear errors, but since the error (e.g. mentioned in the issue) comes from the server, I'd say it is a server side issue.

What I'm proposing is to improve help section for CR in host creation help. Particularly to emphasize which attributes are required (e.g. path mentioned in the issue). Unfortunatelly I've got no environment (except libvirt and a dummy created from VMware) to test which attributes are really required. The chages are based on UI and CR models (basically on default_vm_attributes/new_vm/create_vm, etc)

Requires theforeman/hammer-cli#309 to be merged first. Also this PR is built above #424, since there are chages which will lead to merge conflicts.

Copy link
Contributor

shiramax left a comment

Thanks @ofedoren !
It looks like there's two commits here, one of them is "Host creation with multi SCSI controllers" that was already merged, could you remove it and rebase?

@ofedoren ofedoren force-pushed the ofedoren:bug-25584-path-comp-attr branch from feedd19 to a2f1665 Jul 17, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Jul 17, 2019

@shiramax, rebased.

@shiramax

This comment has been minimized.

Copy link
Contributor

shiramax commented Jul 17, 2019

thanks @ofedoren
We will need to confirm that these are the right attributes for each compute resource
I will verify that for oVirt,
@kgaikwad - can you please confirm that for GCE and EC2?
@ares - can you please confirm that for OpenStack?
@ezr-ondrej - can you please confirm that for VMware?

@@ -6,7 +6,7 @@ def name
end

def compute_attributes
%w[flavor_ref image_ref tenant_id security_groups network]
%w[availability_zone boot_from_volume flavor_ref image_ref tenant_id scheduler_hint_filter security_groups network]

This comment has been minimized.

Copy link
@shiramax

shiramax Jul 17, 2019

Contributor

same here

@@ -6,7 +6,7 @@ def name
end

def compute_attributes
%w[machine_type image_id network external_ip]
%w[associate_external_ip image_id machine_type network user_data]

This comment has been minimized.

Copy link
@shiramax

shiramax Jul 17, 2019

Contributor

same here

@@ -6,7 +6,7 @@ def name
end

def compute_attributes
%w[flavor_id image_id availability_zone security_group_ids managed_ip]
%w[availability_zone flavor_id groups image_id security_group_ids managed_ip]

This comment has been minimized.

Copy link
@shiramax

shiramax Jul 17, 2019

Contributor

can we find the description for each attribute?

['memory', _('Amount of memory, integer value in bytes')]
['sockets', _('Integer value, number of sockets')],
['memory', _('Amount of memory, integer value in bytes')],
['image_id'],

This comment has been minimized.

Copy link
@shiramax

shiramax Jul 17, 2019

Contributor

image_id is a general attribute, shouldn't be here.

['sockets', _('Integer value, number of sockets')],
['memory', _('Amount of memory, integer value in bytes')],
['image_id'],
['user_data', _('Comment')]

This comment has been minimized.

Copy link
@shiramax

shiramax Jul 17, 2019

Contributor

I'm not sure that's spouse to be here... how did you find it?

@ares

This comment has been minimized.

Copy link
Member

ares commented Jul 17, 2019

redirecting to @lzap - can you please confirm that for OpenStack? I guess it would need to be at https://github.com/fog/fog-openstack/blob/master/lib/fog/openstack/compute/models/server.rb but I don't see boot_from_volume and scheduler_hint_filter

but if @lzap has some openstack instnace, it's easier to test on existing vm

@@ -6,7 +6,7 @@ def name
end

def compute_attributes
%w[machine_type image_id network external_ip]
%w[associate_external_ip image_id machine_type network user_data]

This comment has been minimized.

Copy link
@kgaikwad

kgaikwad Jul 17, 2019

Member

Thank you @ofedoren for updating attributes here.
Here, user_data attribute is not required as while creating an image, user can have this attribute.

['memory_mb', _('Integer number, amount of memory in MB')],
['cpus', _('CPU count'), { bold: true }],
['corespersocket', _('Number of cores per socket (applicable to hardware versions < 10 only)'), { bold: true }],
['memory_mb', _('Integer number, amount of memory in MB'), { bold: true }],
['firmware', 'automatic/bios/efi'],

This comment has been minimized.

Copy link
@ezr-ondrej

ezr-ondrej Jul 18, 2019

Member

Could we put the firmware after the required attributes? Now it feels bit weird to be here.
Or sort it alphabeticaly, but now it feels bit messy ordered :)

@ezr-ondrej

This comment has been minimized.

Copy link
Member

ezr-ondrej commented Jul 18, 2019

Could we somehow add a default values for the not required attributes, which have defaults? I do not insist doing it in this PR, though.

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Jul 18, 2019

@shiramax, @kgaikwad, @ezr-ondrej, thanks! The required attributes are highlighted in bold in the help (the note at the beggining of the provider specific atrributes section tells it), the others are plain text.

But I agree that putting the required attributes at the top of the list is a good idea. :)

@ezr-ondrej, yes, I guess we can add a default values for the not required attributes, but I assume it is done on the server side by normalize... methods or default_attrs...

@ezr-ondrej

This comment has been minimized.

Copy link
Member

ezr-ondrej commented Jul 18, 2019

@ezr-ondrej, yes, I guess we can add a default values for the not required attributes, but I assume it is done on the server side by normalize... methods or default_attrs...

Yeah it is, but there is no way for the hammer user to know what those values are going to be. That is my point, to add them in the help :) But I agree it would be better to get them somehow from the apipie, not to hardcode them.

Copy link
Contributor

shiramax left a comment

@ofedoren

  1. Image_id is part of the host common attributes
  2. user_data is configured as part as an image, so it shouldn't be here at all
    image
  3. please change the documentation as well in:
    hammer-cli-foreman/doc/host_create.md
@lzap

This comment has been minimized.

Copy link
Member

lzap commented Jul 22, 2019

I am giving the credentials to the RHOS I test with, all employees can use it.

@ofedoren ofedoren force-pushed the ofedoren:bug-25584-path-comp-attr branch from a2f1665 to 7c0e153 Aug 12, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Aug 12, 2019

@shiramax, updated.

@ofedoren ofedoren changed the title Fixes #25584 - Improve help for compute resources Fixes #25584, #27554 - Improve help for compute resources Aug 14, 2019
]
end

def interface_attributes
[
['type', _('Possible values: %s') % 'bridge, network'],
['bridge', _('Name of interface according to type')],
['model', _('Possible values: %s') % 'virtio, rtl8139, ne2k_pci, pcnet, e1000']
['model', _('Possible values: %s') % 'virtio, rtl8139, ne2k_pci, pcnet, e1000'],

This comment has been minimized.

Copy link
@shiramax

shiramax Aug 14, 2019

Contributor

In libvirt we have these three options:
image
i'm not sure about the bridge, do you know why it's not in the UI is well?

This comment has been minimized.

Copy link
@ofedoren

ofedoren Aug 14, 2019

Author Member

@shiramax, it is, when you're creating a host via libvirt and go Interfaces -> New/Edit -> Select Type.

Also, see https://github.com/theforeman/foreman/blob/develop/app/models/compute_resources/foreman/model/libvirt.rb#L224-L228. I guess the API does support it.

@ares

This comment has been minimized.

Copy link
Member

ares commented Aug 16, 2019

hardware_version Hardware version ID from VMware
add_cdrom Must be a 1 or 0, Add a CD-ROM drive to the virtual machine
cpuHotAddEnabled Must be a 1 or 0, lets you add memory resources while the machine is on
memoryHotAddEnabled Must be a 1 or 0, lets you add CPU resources while the machine is on
start Must be a 1 or 0, whether to start the machine or not

This comment has been minimized.

Copy link
@shiramax

shiramax Aug 19, 2019

Contributor

why did you remove start from here? it should still be here in when creating a host.

@@ -195,26 +194,29 @@ Available keys for `--compute-attributes`:
```
cpus # number of CPUs
memory # string, amount of memory, value in bytes
start # Must be a 1 or 0, whether to start the machine or not

This comment has been minimized.

Copy link
@shiramax

shiramax Aug 19, 2019

Contributor

why did you remove start from here? it should still be here in when creating a host.

@ofedoren ofedoren force-pushed the ofedoren:bug-25584-path-comp-attr branch from 7c0e153 to ccd5eea Aug 21, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Aug 21, 2019

@shiramax, thanks! Updated.

Also, just reminding that theforeman/hammer-cli#309 is required to be merged first. Althought tests are passing, there can be a runtime error.

@ofedoren ofedoren force-pushed the ofedoren:bug-25584-path-comp-attr branch from ccd5eea to aff7531 Aug 22, 2019
@ofedoren ofedoren changed the title Fixes #25584, #27554 - Improve help for compute resources Fixes #25584 - Improve help for compute resources Aug 22, 2019
@shiramax

This comment has been minimized.

Copy link
Contributor

shiramax commented Aug 27, 2019

@ofedoren can you please rebase?

@ofedoren ofedoren force-pushed the ofedoren:bug-25584-path-comp-attr branch from aff7531 to 6426eeb Aug 27, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Aug 27, 2019

@shiramax, rebased.

compute_model # one of [virtio, rtl8139, ne2k_pci, pcnet, e1000]
type # one of [bridge, network]
network / bridge # name of interface according to type
model # one of [virtio, rtl8139, ne2k_pci, pcnet, e1000]

This comment has been minimized.

Copy link
@shiramax

shiramax Aug 27, 2019

Contributor

after the rebase, all the --interface attributes, it should stay with "compute_" prefix.

compute_network # select one of available networks for a cluster
name # eg. eth0
network # select one of available networks for a cluster
interface # interface type

This comment has been minimized.

Copy link
@shiramax

shiramax Aug 27, 2019

Contributor

after the rebase, all the --interface attributes, it should stay with "compute_" prefix.

@ofedoren ofedoren force-pushed the ofedoren:bug-25584-path-comp-attr branch from 6426eeb to 0ad0bd1 Aug 27, 2019
@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Aug 27, 2019

@shiramax, updated. Thanks, I forgot to change those as well :(

@shiramax shiramax merged commit 2f3b422 into theforeman:master Aug 27, 2019
2 checks passed
2 checks passed
Redmine issues Valid issues
Details
hammer Build finished. 5337 tests run, 0 skipped, 0 failed.
Details
@kgaikwad

This comment has been minimized.

Copy link
Member

kgaikwad commented Oct 21, 2019

Apologize for late post.
In case of GCE/Ec2, user can configure image inside compute_attributes at compute_profile as well as host level. But removing image_id from compute_attributes in gce.rb broken the flow.

Am I missing anything here? Is there any foreman PR which handles this?

@ofedoren

This comment has been minimized.

Copy link
Member Author

ofedoren commented Nov 4, 2019

@kgaikwad, apologize for late reply.. Thanks for tracking that though :)

In hammer host create command I think we handle it though --image-id (see

).

But in hammer compute-profile values create/update command I don't see any mentions how to set an image...

@shiramax, @kgaikwad what do you think will be better resolution

  • Add --image-id option as we do have in hammer host create command or
  • Return image_id into compute_attributes help section?
@kgaikwad

This comment has been minimized.

Copy link
Member

kgaikwad commented Nov 14, 2019

@ofedoren,

There was no separate option in the compute profile to set an image.
Previously it was done by using image_id value inside --compute-attributes option.
Example -
hammer compute-profile values update --compute-attributes "image_id=3780108136525169178,machine_type=f1-micro,associate_external_ip=true,network=default" --compute-profile-id 4 --compute-resource-id 6 --volume "size_gb=20"

And with removal of image_id from help section, it will be difficult for new user to know about how to set image_id for the compute_profile.

As --image-id option present in host commands and to maintain a consistency as well as to avoid confusion between host & compute_profile commands usage, it would be good to have similar option for compute_profile.

@shiramax, what do you think?

@shiramax

This comment has been minimized.

Copy link
Contributor

shiramax commented Nov 14, 2019

@kgaikwad image should not be inside compute profile at all.
Compte profile should describe the hardware configuration of the host - for example : cores, sockets, memory, interfaces, and volumes.
the OS is not relevant for compute profile.
also, when you create a host from an image, you need to specify Architecture and Operating system , only then you will be able to choose an Image, Architecture and Operating system should not be inside compute profile.

@kgaikwad

This comment has been minimized.

Copy link
Member

kgaikwad commented Nov 14, 2019

@shiramax,
As far as I know we have an option on web UI to set an image at compute profile level.
If user wants to set image at host level, he can by selecting OS and Architecture.

Could you please confirm if there is any recent change into compute profile form?

Edit - User sets OS & architecture at image creation level and compute profile form has only image field on UI.

@kgaikwad

This comment has been minimized.

Copy link
Member

kgaikwad commented Nov 14, 2019

@shiramax,
Now, I got it. That's why may be if user don't select OS and architecture values first before selecting the compute profile, image field from OS tab on host form gets disabled. To enable that field user needs to deselect compute profile.

@shiramax

This comment has been minimized.

Copy link
Contributor

shiramax commented Nov 17, 2019

@kgaikwad exactly,
AFIK images should not be inside the compute profile.

@kgaikwad

This comment has been minimized.

Copy link
Member

kgaikwad commented Nov 19, 2019

@shiramax,

As mentioned web UI is inconsistent with what we are saying. My concerns are as follows:

  1. On web UI compute profile page shows an image field for selection and on hammer side it is not showing in help and don't have any option to configure image value.
  2. There is no place except the compute profile page to configure an image value for hosts. That means every time user needs to select an image value on Host form.

We will need to either consider redesigning those web UI pages or provide an option which will allow user to configure an image.

@shiramax

This comment has been minimized.

Copy link
Contributor

shiramax commented Nov 19, 2019

@kgaikwad
Adding @ShimShtein and @apuntamb since we spoke about this issue last week. I agree that the current design is not ideal, and we should re-think about it. maybe you can open a topic in https://community.theforeman.org ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.