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 #4509 - VMWare scsi react #4366

Closed

Conversation

matanwerbner
Copy link
Contributor

Adding vmware storage controllers + volumes in react

@timogoebel can you please check it's working with server side? ty

@mention-bot
Copy link

@matanwerbner, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ohadlevy, @gailsteiger and @timogoebel to be potential reviewers.


function hide() {
show = false;
storiesOf('Power Status', module).add('Loading', () => (<PowerStatusInner/>)).add('ON', () => (<PowerStatusInner state="on" title="on" statusText="On"/>)).add('OFF', () => (<PowerStatusInner state="off" title="off" statusText="Off"/>)).add('N/A', () => (<PowerStatusInner state="na" statusText="No power support" title="N/A"/>)).add('Error', () => (<PowerStatusInner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 81 exceeds the maximum line length of 100 max-len

deep_symbolize_keys
volumes = {}
scsi_and_vol[:volumes].each_with_index do |vol, index|
volumes["#{index}"] = vol

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer to_s over string interpolation.

@@ -100,7 +100,7 @@ if (production) {
hot: true
};
// Source maps
config.devtool = 'inline-source-map';
config.devtool = 'eval';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to this pr?

@@ -0,0 +1 @@
$light-grey: #efefef;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is from patternfly pallet? https://www.patternfly.org/styles/color-palette/#_


const CommonForm = (props) => {
return (
<div className={ `form-group ${props.className || ''}` }>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cant we default the props in the const, e.g.:

const CommonForm = ({className = '',  label = ''}) => { ...

};

Select.defaultProps = {
className: ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inconsistent with CommonForm?

@@ -0,0 +1,11 @@
.host-power-status {
&.on {
color: #3f9c35;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to the pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've moved powerStatus files to their own dedicated directory inside hosts, thats the relation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH: based on the size of this PR, please do the restructuring in another PR that we most likely can merge quickly, then you can rebase this pr again.

value={size}
className="text-vmware-size"
onChange={updateDisk.bind(this, 'size')}
label={__('Size (GB)')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we include any basic validation (e.g. size is bigger than 0 and and is integer?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matanwerbner I'm OK with doing it in a separate PR, please open an issue for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateController({[attribute]: value});
};

const _updateDisk = (diskIndex, attribute, e) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the difference for this function? comment maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they each dispatch a different action -
one handles changes for the controller object,
the other for disk object.

storagePod: '',
thinProvision: false,
eagerZero: false,
name: 'Hard disk'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be translated?

@@ -14,3 +14,9 @@ export default createStore(
window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__(),
applyMiddleware(...middleware)
);

export const getNewStore = () => createStore(
reducer,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this do? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this creates a new store, it is very usefull for testing

@@ -1,7 +1,5 @@
import Immutable from 'seamless-immutable';
export const initialState = Immutable({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please elaborate on all the structure changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

state is changed to { hosts: { powerStatus: {}, storage: {} } }

export const MaxDisksPerController = 15;
export const MaxControllers = 4;
export const ControllerTypes = {
VirtualBusLogicController: 'Bus Logic Parallel',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get these values from the rails model?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, can you help put those there? my ruby is very weak

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do.

Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matanwerbner : Thanks for taking over the PR! I did not test provisioning, yet. But I have some comments regarding the react UI.

className="cb-controller-is-selected"
checked={controller.isSelected}
onChange={_updateController.bind(this, 'isSelected')}/>
<label>{__('Create SCSI controller')}</label>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checkbox should be checked by default. A VM needs at least one scsi controller or your VM won't have any storage.
I'm wondering if this checkbox is actually useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is by design supplied (i will attach), it's only purpose is to decide which controllers get deleted when you click "delete controllers".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the design is flawed then. It would be better to have a dedicated delete button per controller (we already have a dedicated button per disk). The checkbox is very confusing. I didn't get, that it's related to the Delete Controller button.

image

btw: This is how this looks in VMWare. The UI is pretty bad I think, but Foreman users might be familiar with this:

image
(Sorry for the German language, but I guess you get the point)

@Rohoover: Do you agree? Any more comments?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timogoebel @matanwerbner

The original design was provided here: #3979 (comment)

It was done with a low level of effort in mind. The checkboxes work similar to the Foreman tables, where items are selected before actions occur to them.

That said, alternative layouts can be done. Attached is another quick mockup with you feedback. I think it we wanted we can also add the chevrons (accordion) feature shown in the VMWare UI as well if desired.

create controller volumev2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rohoover : Thanks. I think with this design it's a lot more clear how to delete a controller. I like that.

I do think the chevrons are a good idea. However I don't think it's a requirement for this PR.
@matanwerbner: If you want to earn some extra points, go ahead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll try to get the chevrons in soon, i have some things on my plate prior to that, so for now i just removed the checkboxes...

value={props.value}
onChange={props.onChange}
>
<option value="">{__('Please select')}</option>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hide this if a user selects something, e.g. make this a real placeholder?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is hidden when a user selects another options, it is regular select behavior, same as other options in the select

<Select
label={__('Data store')}
value={dataStore}
onChange={updateDisk.bind(this, 'dataStore')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data doesn't update when you change the compute resource. We have a couple of VMWare compute resources that all have different datastores.
If I select one compute resource, then another one it still shows the datastores of the first one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition if you create a new volume, the select fields aren't rendered as `select2´. The usage makes a lot of sense, though. We have at least 100 datastores. Select2 helps a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first comment: the json seems to update OK, can you check if maybe it gets lost in the server somehow?
as for select2 - i will add for compute resource

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matanwerbner I think this is an issue as the react component is already initialized, it wont be mounted again.

afaiu steps to reproduce are:

  1. have two different vmware compute resource
  2. create a new host, select the first vmware, then change the compute resource to the second vmware

first vmware form will persist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timogoebel i believe i've solved this issue, can you pull and check again?


return (
<div className="disk-container">
<TextInput
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be read-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disk name should be read only? maybe we should no show it at all then..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the name is generated by VMWare. A user cannot change it but should be able to see it when editing a host (or he would edit the wrong disk by accident).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently they will all show "hard disk" since it is taken from consts. should i implement an ajax request to fetch the new volume name from the server?

});

return (
<div className="disk-container">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disk mode parameter is missing. It was introduced in 1d5f6c2.

expect(
wrapper.render().find('.controller-container').length
).toEqual(1);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing spaces not allowed no-trailing-spaces

@@ -0,0 +1,14 @@
export const defaultConrollerAttributes = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo in filename, it should be vmware.const.js

@@ -7,7 +7,12 @@ export const ACTIONS = {
NOTIFICATIONS_EXPAND_DRAWER_TAB: 'NOTIFICATIONS_EXPAND_DRAWER_TAB',
NOTIFICATIONS_MARK_AS_READ: 'NOTIFICATIONS_MARK_AS_READ',
NOTIFICATIONS_MARKED_AS_READ: 'NOTIFICATIONS_MARKED_AS_READ',
NOTIFICATIONS_SET_REQUEST_STATUS: 'NOTIFICATIONS_SET_REQUEST_STATUS'
NOTIFICATIONS_SET_REQUEST_STATUS: 'NOTIFICATIONS_SET_REQUEST_STATUS',
CONTROLLER_ADDED: 'CONTROLLER_ADDED',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whats the difference to the STORAGE_VMWARE_ADD_CONTROLLER const?

@matanwerbner matanwerbner force-pushed the fix_4509_scsi_react branch 2 times, most recently from 2b91ef6 to ba7f287 Compare March 14, 2017 13:47
@timogoebel
Copy link
Member

@matanwerbner : I'm super busy this week. Sorry for the delay. I do want this feature, appreciate your work and will gladly help you. I'll check this out first thing next week. Please bear with me here.

@@ -925,4 +926,18 @@ def host_attributes_for_templates(host)
def csv_columns
[:name, :operatingsystem, :environment, :model, :hostgroup, :last_report]
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace detected.

@matanwerbner matanwerbner force-pushed the fix_4509_scsi_react branch 4 times, most recently from f410f39 to 46ae0af Compare March 15, 2017 15:42
Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matanwerbner : Sorry for the delay in reviewing this. All in all, it looks good, but I found some issue while testing this.

First of all I added code to pass the controller_types to react:
timogoebel@c047ccc

Just squash that in if you want.

Second of all, the code does not pass the values of datastore or storage_pod in the JSON hash send via post. The fields are just empty strings. That's why vm creation fails right now.

When there are errors creating the VM, the react part of the form is reset.

When you edit a vm, it does not pick up the present values. The form should show the current data from the VM, e.g. if the VM has three volumes it should show three volumes. All fields except the size should be disabled as fog-vsphere (the library we use for vmware management) does not support editing any more parameters of a volume.

In addition, I left some comments inline.

And thanks again for doing this, first react form and a vmware form - that's not trivial 👍

<TextInput
value={name}
onChange={updateDisk.bind(this, 'name')}
label={__('Disk name')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be read-only or not even a text field at all. The name is interesting for users but can't actually be changed at all. Please see #4252.

const selectablePods = renderOptions(storagePods);

return (
<div className="disk-container">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disk mode select box is missing which was recently added. Please see #4314

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added, do you want to send the possible values the same was as controller_types?

/>

<Select
label={__('Storage Pod')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we hide this if there aren't any storage pods?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And if you select a storage pod we have to disable or hide the datastore select.
You can create a VM either on a storage pod or on a datastore. Storage pod has the higher priority. If a user selects a storage pod, vmware host creation will ignore what was selected as the datastore.

<label>{__('Create SCSI controller')}</label>
</div>
<div className="controller-type-container col-md-4">
<select
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a select2 as well when adding a new controller.

@matanwerbner
Copy link
Contributor Author

@timogoebel I've taken care of most of what you've asked, just a couple of questions:
regarding the change in select.js, what is the reason to doing that? dont we want a friendly, localized "please select" text to be shown?
second, what do you mean by debug json?

thanks

@matanwerbner matanwerbner force-pushed the fix_4509_scsi_react branch 2 times, most recently from 217aaf5 to 4b5a794 Compare June 22, 2017 07:52
@timogoebel
Copy link
Member

timogoebel commented Jun 22, 2017

@matanwerbner

regarding the change in select.js, what is the reason to doing that? dont we want a friendly, localized "please select" text to be shown?

Actually, we don't have that anywhere else. We always use the pattern I suggested. But maybe @Rohoover has some advice here.
It's clear button vs. a "Please select".
image

second, what do you mean by debug json?

I mean the blue json output under the react form.

@matanwerbner
Copy link
Contributor Author

gotcha. done.

@timogoebel
Copy link
Member

@matanwerbner: Thanks. I retested this.
Some more comments:

  1. When editing a VM, the size is now shown correctly. I posted the fix earlier:
diff --git a/app/views/compute_resources_vms/form/vmware/_base.html.erb b/app/views/compute_resources_vms/form/vmware/_base.html.erb
index 7695f0e92..46cdeb12b 100644
--- a/app/views/compute_resources_vms/form/vmware/_base.html.erb
+++ b/app/views/compute_resources_vms/form/vmware/_base.html.erb
@@ -52,6 +52,6 @@ end %>
     storagePods: vsphere_storage_pods(compute_resource),
     datastores: vsphere_datastores(compute_resource)
   },
-  volumes: f.object.volumes.map { |volume| volume.attributes.deep_transform_keys { |key| key.to_s.camelize(:lower).to_sym }.reject { |k,v| k == :Size } },
+  volumes: f.object.volumes.map { |volume| volume.attributes.merge(:size_gb => volume.size_gb).deep_transform_keys { |key| key.to_s.camelize(:lower).to_sym }.reject { |k,v| k == :Size } },
   controllers: f.object.scsi_controllers
 }.to_json) %>
  1. Can we please disable the image button alongside the other form fields when editing a host?

  2. The code still shows the please select. :-)

  3. I can select a disk of size 0. This doesn't work. Can we prohibit this and make 1 the minimum value?
    image

The rest works great! Good job.

@matanwerbner matanwerbner force-pushed the fix_4509_scsi_react branch 3 times, most recently from b6c83c7 to 75193d2 Compare June 25, 2017 08:15
@matanwerbner
Copy link
Contributor Author

@timogoebel requested changes have been commited, please check

@timogoebel
Copy link
Member

timogoebel commented Jun 28, 2017

@matanwerbner : Thanks. Nicely done.

@ohadlevy made some minor changes in ohadlevy@541fb96 . Could you squash them in except for the precision / decimal stuff for sizeGb? sizeGb has to be an integer. Float does not work.

Unfortunately I found another bug. When you clone a host the scsi and harddisk settings aren't cloned to the new host. :-(

Here is the fix:

diff --git a/app/models/compute_resources/foreman/model/vmware.rb b/app/models/compute_resources/foreman/model/vmware.rb
index 1b4fd6ee0..38817d624 100644
--- a/app/models/compute_resources/foreman/model/vmware.rb
+++ b/app/models/compute_resources/foreman/model/vmware.rb
@@ -553,6 +553,9 @@ module Foreman::Model
         hsh[index.to_s] = interface_attrs
         hsh
       end
+      vm_attrs[:scsi_controllers] = vm.scsi_controllers.map do |controller|
+        controller.attributes
+      end
       vm_attrs
     end

I also tested compute profiles. The volumes don't seem to get saved there. I'll investigate.

@timogoebel
Copy link
Member

timogoebel commented Jun 28, 2017

I also tested compute profiles.

Unfortunately, I found some more serious issues with this.
Compute profiles don't save and update correctly.
Hosts don't update correctly.

Edit: Fix posted below.

@ohadlevy
Copy link
Member

@matanwerbner I've made a few minor fixes and improvements to the storybook, can you merge ohadlevy@e36314d into this PR as well? thanks!

@timogoebel
Copy link
Member

@matanwerbner : I have completed my fix for the compute profiles: timogoebel@b1c47b9 This also contains a tiny test for the scsi controller normalization and @ohadlevy's review fixes. It does not contain ohadlevy/foreman@e36314d.

To summarize:
You need to add
ohadlevy/foreman@e36314d (needs a rebase as a new key was added to the Component's config hash in the other commit)
and
timogoebel@b1c47b9

@matanwerbner matanwerbner force-pushed the fix_4509_scsi_react branch 3 times, most recently from bbfe56f to 820e916 Compare July 5, 2017 09:00
@matanwerbner
Copy link
Contributor Author

@timogoebel i'm having difficulties and errors applying your patch, can you rebase/pull from develop?
i've included @ohadlevy 's patch

const {
addController,
controllers,
volumes,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: whitespace.

return controllers.map((controller, idx) => {
const controllerVolumes = volumes.filter(
v => v.controllerKey === controller.key
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace in here and below

@@ -14,6 +14,8 @@ import PowerStatusInner
from '../assets/javascripts/react_app/components/hosts/powerStatus/powerStatusInner';
import Toast
from '../assets/javascripts/react_app/components/toastNotifications/toastListitem';
import StorageContainer from '../assets/javascripts/react_app/components/hosts/storage/vmware';
import * as VMWareData from './data/storage/vmware';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'VMWareData' is defined but never used no-unused-vars

@@ -14,6 +14,8 @@ import PowerStatusInner
from '../assets/javascripts/react_app/components/hosts/powerStatus/powerStatusInner';
import Toast
from '../assets/javascripts/react_app/components/toastNotifications/toastListitem';
import StorageContainer from '../assets/javascripts/react_app/components/hosts/storage/vmware';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'StorageContainer' is defined but never used no-unused-vars

@ohadlevy
Copy link
Member

replaced by #4676 - thanks @matanwerbner :-)

@ohadlevy ohadlevy closed this Jul 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants