-
Notifications
You must be signed in to change notification settings - Fork 64
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
OCS Installation UI flow #211
OCS Installation UI flow #211
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add an entry for OpenShift Container Storage into https://github.com/openshift/openshift-origin-design/readme.md, with an entry for the OCS install workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shirimordechay - Awesome first PR! Well done. I've added some inline comments.
@tlwu2013 - It would be great to hear your thoughts on whether this is in line with the current OLM design or not. I know you are continuously working on updates so it would be great to hear if this flow works with the latest and greatest. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flow wise is looking good! I've left a comment for "Installation Mode" on Create Operator Subscription view. Another minor nit, could you help replace "Operator Hub" to "OperatorHub" in this doc to match up with UI? Also, there seem to be some updated mocks in the Google doc, wondering if we need to update the images here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think it looks good except for some minor things that should be addressed (I've left comments below).
Also, we just need to make sure the node list (how we show devices selection) and device list are synchronized between Install and Expand cluster workflows.
that capacity is missing. | ||
![Installation page](img/Installed_OCS_OCS_Tab_error_mode.png) | ||
In the OCS service details page- show a static error message. | ||
![Installation page](img/Installed_OCS_Overview_error.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning message has typos and also not quite right (as you can't add nodes in a AWS on OCP+RHCOS to get more capacity). You can only add capacity.
I suggest the following wording instead:
OCS-Service-1 has no capacity. Click Add Capacity to add capacity to the OCS service.
If we can have Add Capacity bring up the modal (Add Capacity modal from the Expand Cluster design), that would be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yuvalgalanti Do we have an updated modal design for "Add capacity"? if not, we should wait until the Expand cluster design is fully baked before adding it here.
|
||
|
||
# The OCS installation process | ||
OCS Overview page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shirimordechay Between the OCS_Subscription_page.png and OCS-View.png, it's missing a couple other mockups of what that workflow now looks like in OperatorHub.
Also for OCS-View.png, the "OCS" and "MCG" tabs should also exist before user creates the OCS service based on the latest OCP 4.1 OperatorHub deployment workflow.
Please update accordingly. Thanks.
@yuvalgalanti FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which mockups are missing in this flow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julienlim Open question for you here ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shirimordechay @lizsurette @tlwu2013
Overall it LGTM. Just the operator install/subscription didn't quite seem to match what @tlwu2013 had shared recently (https://projects.invisionapp.com/share/7TRU0EYHJNM#/screens/361317773) with me but does match closer to what's in OCP currently.
As long as @tlwu2013 is ok with it, we should be fine. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great @shirimordechay ! I added a few nit picks, but am going to approve since I think everything else looks great.
Latest changes LGTM! |
I think this PR is in a good spot to merge barring an approval from @tlwu2013 on the OLM side and @beanh66 or @rileyhuston on the console side. FYI @alimobrem @spadgett |
+1 This PR LGTM barring an exceptions from @tlwu2013 on the OLM side. Just mentioned the implementation is in progress at openshift/console#1982. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting a small change request regarding capacity starting at 1 TiB and in 1 TiB increments only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shirimordechay @lizsurette LGTM, I left some minor comments around changes that were made to the Installed Operator detail pages for 4.2. Let me know if you have questions and we can pull @tlwu2013 in as well.
Installation | ||
![OperatorHub operators view](img/OCS-Install-step1.png) | ||
|
||
![OperatorHub operators view](img/OCS-Install-step2.png) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tlwu2013 Don't we need to specify the installation mode here as well? Typically the first item in this form.
- Terminology fixes - Removed PVCs column - Added a pointer to show where the user is clicking throughout the screens. - Some minor changes
remove PVCs column Change vCPU --> CPU
Some changes in the installation flow form in select devices. Also added an example for no capacity in OCS use case.
- Added a radio button in the OCS installation flow for IPI/ UPI selection - Added Select devices different modes examples
Added a subscribed operator page
- Text changes - Capacity was changed to "Selected Capacity" and now presenting the selected of available - Error message changed
- Both tabs will show under OCS - Design changes in installation page - Minor changes
- IPI is removed, UPI for AWS and VMware only. - Removed devices list and selection - Added capacity and storage class fields
Changed to cluster.ocs.openshift.io/openshift-storage=""
- Default of 1 TiB (and no longer 4 TiB) - Unit is not longer changeable
- Moved subscription page to new tab - Changed Overview layout - Re-order README.md
f1363b1
to
743d44a
Compare
### Select Nodes | ||
Admin needs to select 3 nodes to label with “cluster.ocs.openshift.io/openshift-storage=""” (note label is subject to change as this will be automatically configured by the OCS operator) to be used for the OCS cluster. | ||
* Admin may need to filter the list of nodes in order to make the selection (i.e. nodes that contain storage already), e.g. | ||
Select/unselect all nodes, and exclude master nodes (e.g. based on roles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can exclude the masters, but what if all we have are the masters?
* Non-master nodes’ capacity n TiB or greater (e.g. >= 10 TiB) | ||
* Nodes with a certain name prefix or string within the node name | ||
Admin will also specify the capacity for the cluster and the storage class to use. | ||
* Capacity will default to 1 TiB, and user cannot modify value to be less than 1 TiB. Only TiB, PiB, and units above TiB supported. GiB and MiB are not permitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete.
- Install screens order was switched - a new 5 OSD per OCS node limitation was added - some minor changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added a question but overall LGTM
Thanks for all of the hard work here @shirimordechay ! IMO we can merge once @tlwu2013 is okay with the design. FYI @beanh66 |
@@ -15,6 +15,10 @@ Access design documentation for features slotted in the 4.2 release of OpenShift | |||
- [Operators Overview](http://openshift.github.io/openshift-origin-design/web-console/future-openshift/operators-overview/operators-overview) | |||
- [Taints and Tolerations](http://openshift.github.io/openshift-origin-design/web-console/future-openshift/taints-tolerations/taints-tolerations) | |||
|
|||
## OpenShift Container Storage | |||
- [OCS Install workflow](./web-console/Storage/OCS/OCS_Installation_Workflow.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shirimordechay Clicking the ocs install worklfow link is giving 404 error to me. Is there something I am doing wrong here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not merged yet, try this for now: https://github.com/shirimordechay/openshift-origin-design/blob/OCS-Install-workflow/web-console/Storage/OCS/OCS%20_Installation_Workflow.md
Just noting that the implementation can be seen at openshift/console#2359. |
Removed capacity and storage class fields for 4.2 implementation.
Designs of OCS 4.2 install workflow for converged mode.
Includes Operator hub view, OCS Service Installation and after installation designs.
@openshift/team-ux-leads