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
mgr/dashboard: Host Maintenance Feature #39226
Conversation
Need to add the unit tests and integration tests for frontend as well as backend. |
This is worthy a demo at tomorrow's daily, @nizamial09? |
Yup. Can do that. |
@nizamial09 why is the maintenance button separate? In the context of host management I'd expect that we need to support maintenance, and in the future drain etc - each feature can't be a separate button can it? |
12128af
to
d11d7cd
Compare
jenkins test make check |
d11d7cd
to
9e4753e
Compare
@pcuzner I've put the button inside the dropdown box. PTAL, thanks. |
9e4753e
to
015c11a
Compare
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.
Awesome work, @nizamial09 ! I'm really sorry for my late review here.
Regarding the action buttons, given enter and exit are complimentary actions I find a total waste of space and visual clarity to display (even grayed-out) an action that the user won't ever be able to perform...
I remember we discussed this with @votdev, in order to perform this in a later refactoring, but given we are now doing this and 'later refactors' some times never show up, why not starting right now to adopt this practice of only showing the actions that can be performed (unless there's a good reason to keep them grayed-out, like providing a tooltip)?
Is it possible to suppress the red warning pop-up?
Alternatively, this approach blue->{red,green} notification approach could work (we already had this behaviour for some operations, like Pool creation):
- The enter-maintenance launches a background job/notification (in blue)
- If successful, a green notification appears.
If the modal doesn't allow the user to do anything at all (just closing the modal):
... I'd rather go with the blue->red notification approach.
Regarding the error when the host cannot enter in maintenance mode, perhaps a 400 is too broad and mostly to indicate syntax errors in the request. What about a '409 Conflict' (Indicates that the request could not be processed because of conflict in the current state of the resource,
)
src/pybind/mgr/dashboard/frontend/cypress/integration/cluster/hosts.po.ts
Outdated
Show resolved
Hide resolved
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.
Tested it manually & LGTM. Great work @nizamial09 !
cb781f3
to
3c4348c
Compare
jenkins test dashboard |
jenkins test make check |
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.
3c4348c
to
3dc230d
Compare
jenkins test make check |
jenkins test dashboard |
We have merged: Which introduces the possibility to create hosts that arrives to the cluster in "maintenance mode". I do not know if this enhancement for adding hosts can be included or not in this pr. But in any case is good to outline that we have this feature available.(very useful) The UI seems easy to use and understand. For me is ok. |
Thanks @jmolmo and yeah we are planning to include this new feature on a seperate PR as an improvement. Once this is merged I'll get started on that ASAP. |
jenkins test dashboard |
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/hosts/hosts.component.ts
Show resolved
Hide resolved
src/pybind/mgr/dashboard/frontend/src/app/ceph/cluster/hosts/hosts.component.ts
Outdated
Show resolved
Hide resolved
3dc230d
to
1291785
Compare
jenkins test dashboard |
This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved |
1291785
to
db6840f
Compare
In Cluster -> Hosts, I've added additional button to put the selected host on maintenance or exit out of the maintenance mode. Also for some hosts the ok-to-stop tests may trigger some warnings which requires a --force command to pass along with the maintenance enter command to enter a host into maintenance. In UI this is achieved using a confirmation Modal. In addition to this if the check error is It is NOT safe to stop the host then the host wont be able to put into maintenance mode. Fixes: https://tracker.ceph.com/issues/49101 Signed-off-by: Nizamudeen A <nia@redhat.com>
db6840f
to
0780fc1
Compare
jenkins test make check |
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.
Awesome job @nizamial09 ! Let's try to get this backported asap to Pacific so that we can demo it in the next review 🎉
In Cluster -> Hosts, I've added additional button to put the selected host on maintenance or exit out of the maintenance mode. Also for some hosts the ok-to-stop tests may trigger some warnings which requires a --force command to pass along with the maintenance enter command to enter a host into maintenance. In UI this is achieved using a confirmation Modal. In addition to this if the check error is It is NOT safe to stop the host then the host wont be able to put into maintenance mode.
ENTER MAINTENANCE MODE
EXIT MAINTENANCE MODE
FORCE ENTER MAINTENANCE MODE
NOT SAFE TO ENTER MAINTENANCE MODE
ONLY ONE HOST
Removed the other two hosts but it was showing in the list
Fixes: https://tracker.ceph.com/issues/49101
Signed-off-by: Nizamudeen A nia@redhat.com
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox