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

mgr/dashboard: Watch for pool pg's increase and decrease #28006

Merged
merged 4 commits into from Jul 18, 2019

Conversation

@Devp00l
Copy link
Contributor

commented May 7, 2019

Now when creating or editing a pool the background task will watch
increasing and decreasing pg's. The executing task will also use a
progress to show the progress.

If the change was not executed through the dashboard, there is no task
to show, but the frontend will make sure to inform the user about the
change that is going on in order to stop the user from doing actions on
the changing pool.

Fixes: https://tracker.ceph.com/issues/39482
Signed-off-by: Stephan Müller smueller@suse.com

pg_progress_2019-05-07 16-27

@Devp00l Devp00l added the dashboard label May 7, 2019

@Devp00l Devp00l requested review from LenzGr, tspmelo, alfonsomthd and p-na May 7, 2019

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

jenkins retest this please

@callithea

This comment has been minimized.

Copy link
Member

commented May 8, 2019

jenkins test make check

@callithea

This comment has been minimized.

Copy link
Member

commented May 8, 2019

jenkins test dashboard

@callithea

This comment has been minimized.

Copy link
Member

commented May 8, 2019

Lgtm, tested it on my local system.
But make check and the dashboard tests failed again, will wait for the latest results.

@Devp00l Devp00l force-pushed the Devp00l:wip-39482 branch from d2e0f9c to 6abf621 May 10, 2019

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@callithea I've rebased the branch hope make check passes now.

@tspmelo
Copy link
Contributor

left a comment

looks good, but I have a few recommendations.

Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/pool.py Outdated
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/pool.py Outdated
Show resolved Hide resolved src/pybind/mgr/dashboard/controllers/pool.py Outdated
Show resolved Hide resolved ...gr/dashboard/frontend/src/app/ceph/pool/pool-list/pool-list.component.ts Outdated
@callithea

This comment has been minimized.

Copy link
Member

commented May 16, 2019

jenkins retest this please

@Devp00l Devp00l force-pushed the Devp00l:wip-39482 branch from 6abf621 to d807519 May 17, 2019

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

jenkins retest this please

@Devp00l Devp00l added feature bug fix and removed feature bug fix labels May 17, 2019

@p-na

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

This PR seems to introduce or reveal a problem I wasn't able to reproduce on master. When the number of placement groups of a pool is reduced or increased, the Read ops and Write ops columns show negative values. For write/read operations per seconds, that feels odd, but it may be okay as it's a time series value? It looks like the Read bytes and Write bytes columns may also be affected by this. I haven't seen this problem on master, when increasing or decreasing the placement groups on the CLI, so I'm raising that here.

1


beforeEach(() => {
expected = {
cdIsBinary: true,

This comment has been minimized.

Copy link
@p-na

p-na May 20, 2019

Contributor

Should cdIsBinary be written in snake_case?

This comment has been minimized.

Copy link
@Devp00l

Devp00l May 20, 2019

Author Contributor

It's only used in frontend AFAIK

This comment has been minimized.

Copy link
@votdev

votdev Jun 19, 2019

Contributor

I'm asking myself why we mix snake and camel case in the JS code. I know the backend/REST API returns dict's with snake case keys, but then we should use snake case only in the JS code, even if this is not common, but mixing both types looks strange.

let expected;

beforeEach(() => {
expected = {

This comment has been minimized.

Copy link
@p-na

p-na May 20, 2019

Contributor

Suggestion: What if you created a function that contained the default defined here as expected and extends and returns it by the argument passed to that function? That way you wouldn't need the expected attribute in that block. It would also remove the need to set the default in the beforeEach block, removing the need for the beforeEach block. Also, _.merge would only be called once, namely inside getPoolData (the new function) instead of multiple times in the test it blocks. The expected = undefined; line (further down) would also disappear.

I think that might look a little bit clearer afterwards.

Instead of having to call

      expected = _.merge(expected, {
        cdExecuting: 'Updating',
        pg_num: 32,
        pg_num_target: 16
      });

it'd be

      expected = getPoolData({
        cdExecuting: 'Updating',
        pg_num: 32,
        pg_num_target: 16
      });
@mock.patch('dashboard.tools.TaskManager.current_task')
def test_wait_for_pgs_with_waiting(self, taskMock, _get):
task = MockTask()
taskMock.side_effect = lambda: task

This comment has been minimized.

Copy link
@p-na

p-na May 20, 2019

Contributor

This could be replaced with taskMock.return_value = task, no need to pass a function and use a lambda expression here.

@p-na
Copy link
Contributor

left a comment

Please look into the negative read/write ops mentioned in the comment above.

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@p-na Seems to be a time series problem similar to #28153 . Never saw the graph decrease before but it explains the negative values pretty good. I would like to fix that with #28153 if that's ok with you.

@p-na

This comment has been minimized.

Copy link
Contributor

commented May 20, 2019

@p-na Seems to be a time series problem similar to #28153 . Never saw the graph decrease before but it explains the negative values pretty good. I would like to fix that with #28153 if that's ok with you.

No objections!

@p-na p-na dismissed their stale review May 20, 2019

Issue is supposed to be fixed in #28153

@Devp00l Devp00l force-pushed the Devp00l:wip-39482 branch from d807519 to 5b246e0 May 21, 2019

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Addressed all comments

@alfonsomthd
Copy link
Contributor

left a comment

Tested locally: LGTM

@tspmelo
Copy link
Contributor

left a comment

lgtm

@LenzGr

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

jenkins test make check

@Devp00l Devp00l force-pushed the Devp00l:wip-39482 branch from 5b246e0 to 0de88cd Jun 17, 2019

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

jenkins test make check

1 similar comment
@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

jenkins test make check

diff = max_diff - abs(target - current)
percentage = int(round(diff / float(max_diff) * 100))
TaskManager.current_task().set_progress(percentage)
time.sleep(4)

This comment has been minimized.

Copy link
@votdev

votdev Jun 19, 2019

Contributor

Why are you using 4 seconds here? Is there a special reason?

This comment has been minimized.

Copy link
@Devp00l

Devp00l Jun 19, 2019

Author Contributor

The polling time of the frontend is 5 seconds so we only need to recalculate every 4 seconds as discussed with @tspmelo in his comment

This comment has been minimized.

Copy link
@votdev

votdev Jun 19, 2019

Contributor

Can you add this as a comment, otherwise this info gets lost and is only visible in the PR.

This comment has been minimized.

Copy link
@Devp00l

Devp00l Jun 19, 2019

Author Contributor

done :)

@@ -140,6 +143,23 @@ def reset_arg(arg, value):
reset_arg(arg, '0')
reset_arg('compression_algorithm', 'unset')

def _wait_for_pgs(self, pool):
current_pool = self._get(pool)

This comment has been minimized.

Copy link
@votdev

votdev Jun 19, 2019

Contributor

I'm missing any documentation here. Is pool a string or int?

This comment has been minimized.

Copy link
@Devp00l

Devp00l Jun 19, 2019

Author Contributor

pool is a string and I will add the missing documentation - Thanks for the hint :)

self._pg_wait_loop(current_pool, initial_pgs)

def _pg_wait_loop(self, pool, initial_pgs):
if 'pg_num_target' in pool:

This comment has been minimized.

Copy link
@votdev

votdev Jun 19, 2019

Contributor

I'm missing any documentation here. What types are the method parameters and what are they for? This info would help reviewing and understanding the code.

This comment has been minimized.

Copy link
@Devp00l

Devp00l Jun 19, 2019

Author Contributor

pool is the pool object and initial_pgs is a number. I will add the missing documentation - Thanks for the hint :)


beforeEach(() => {
expected = {
cdIsBinary: true,

This comment has been minimized.

Copy link
@votdev

votdev Jun 19, 2019

Contributor

I'm asking myself why we mix snake and camel case in the JS code. I know the backend/REST API returns dict's with snake case keys, but then we should use snake case only in the JS code, even if this is not common, but mixing both types looks strange.

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

The polling time of the frontend is 5 seconds so we only need to recalculate every 4 seconds as discussed with @tspmelo in his comment.

The recalculation is happening inside pool.py in the method called _pg_wait_loop.

mgr/dashboard: Watch for pool pg's increase and decrease
Now when creating or editing a pool the background task will watch
increasing and decreasing pg's. The executing task will also use a
progress to show the progress.

If the change was not executed through the dashboard, there is no task
to show, but the frontend will make sure to inform the user about the
change that is going on in order to stop the user from doing actions on
the changing pool.

Fixes: https://tracker.ceph.com/issues/39482
Signed-off-by: Stephan Müller <smueller@suse.com>

@Devp00l Devp00l force-pushed the Devp00l:wip-39482 branch from 0de88cd to fc08d2e Jul 3, 2019

@Devp00l

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

@votdev addressed your comments

@tspmelo

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2019

jenkins retest this please

@ricardoasmarques

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

Screenshot from 2019-07-15 16-00-59

nit: @Devp00l Any change we can change the message from "Updating 50%..." to "Updating... 50%"?

@rjfd

rjfd approved these changes Jul 16, 2019

Copy link
Contributor

left a comment

lgtm, this is a really nice improvement.

@rjfd

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

@Devp00l can you address @ricardoasmarques comment soon, so we can merge this today?

@ricardoasmarques

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

jenkins retest this please

@callithea

This comment has been minimized.

Copy link
Member

commented Jul 16, 2019

jenkins test make check

mgr/dashboard: pool: make _get and _wait_for_pgs class methods
Signed-off-by: Ricardo Dias <rdias@suse.com>
@ricardoasmarques
Copy link
Member

left a comment

lgtm

@tspmelo

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

jenkins test make check

rjfd added some commits Jul 16, 2019

mgr/dashboard: test_pool: fix pool unit tests
Signed-off-by: Ricardo Dias <rdias@suse.com>
mgr/dashboard: frontend: move ellipsis to before progress in task exe…
…cution description

Signed-off-by: Ricardo Dias <rdias@suse.com>

@rjfd rjfd force-pushed the Devp00l:wip-39482 branch from 26f8540 to 0d73515 Jul 17, 2019

@callithea

This comment has been minimized.

Copy link
Member

commented Jul 17, 2019

http://pulpito.ceph.com/laura-2019-07-17_14:30:31-rados:mgr-wip-lpaduano-testing-28006-distro-basic-smithi/
Tests failed due to errors like this mds.a (mds.0) 1 : cluster [WRN] evicting unresponsive client smithi146:x (11197), after 301.83 seconds" in cluster log

@callithea

This comment has been minimized.

@rjfd

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

We can ignore the failures in QA tests. The dashboard tests have succeeded and the jobs are being marked as failed due to warning caused by recent changes to the volumes module.

Therefore this PR is ready to be merged.

@ricardoasmarques ricardoasmarques merged commit a95e7bf into ceph:master Jul 18, 2019

5 of 6 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
ceph dashboard tests ceph dashboard tests succeeded
Details
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.