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: Add Pool-create to the backend #20865

Merged
merged 7 commits into from Mar 26, 2018

Conversation

Projects
None yet
3 participants
@sebastian-philipp
Copy link
Member

sebastian-philipp commented Mar 13, 2018

Requires #20863

Local testing is affected by http://tracker.ceph.com/issues/23345

Also:

  • Add API test
  • Fixed @RESTController.args_from_json
  • Refactor send_command into single method …
  • Minor tweak to DashboardTestCase._request
@LenzGr
Copy link
Contributor

LenzGr left a comment

Thanks! I wonder if it would make sense to separate the developer page functionality into a separate PR, though.

@sebastian-philipp sebastian-philipp force-pushed the sebastian-philipp:dashboard_v2_pool-create branch 3 times, most recently from 24e5b91 to 59f02c7 Mar 13, 2018

@sebastian-philipp sebastian-philipp changed the title [WIP] mgr/dashboard_v2: Add create() to Pool controller mgr/dashboard_v2: Add create() to Pool controller Mar 13, 2018

@sebastian-philipp sebastian-philipp requested a review from Devp00l Mar 13, 2018

@sebastian-philipp sebastian-philipp force-pushed the sebastian-philipp:dashboard_v2_pool-create branch from 59f02c7 to 8f220c8 Mar 15, 2018

@sebastian-philipp sebastian-philipp removed the DNM label Mar 15, 2018

@sebastian-philipp sebastian-philipp force-pushed the sebastian-philipp:dashboard_v2_pool-create branch from 8f220c8 to 6a1ee39 Mar 15, 2018

@sebastian-philipp sebastian-philipp added needs-qa and removed needs-qa labels Mar 15, 2018

@rjfd
Copy link
Contributor

rjfd left a comment

please split the pool create commit in two, so that the args_from_json fix is done in a separate commit.

def create(self, pool, pg_num, pool_type, erasure_code_profile=None, cache_mode=None,
tier_of=None, read_tier=None, flags=None, compression_required_ratio=None,
application_metadata=None, crush_rule=None, rule_name=None, **kwargs):
ecp = erasure_code_profile if erasure_code_profile else None

This comment has been minimized.

@rjfd

rjfd Mar 16, 2018

Contributor

Why do you need this line, if the default value of erasure_code_profile is already None?

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 16, 2018

Author Member

This also catches empty strings

CephService.send_command('mon', 'osd pool set', pool=pool, var='crush_rule',
val=rule_name)
for key, value in kwargs.items():
if type == 'replicated' and key not in \

This comment has been minimized.

@rjfd

rjfd Mar 16, 2018

Contributor

there is no type variable declared in this method. I believe this must be evaluating to the builtin type function.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 21, 2018

Author Member

fixed.

if type == 'replicated' and key not in \
['name', 'erasure_code_profile_id'] and value is not None:
CephService.send_command('mon', 'osd pool set', pool=pool, var=key, val=value)
elif self.type == 'erasure' and key not in ['name', 'size', 'min_size'] \

This comment has been minimized.

@rjfd

rjfd Mar 16, 2018

Contributor

I don't see anywhere in the Pool class the type instance attribute being assigned. I guess what you want to use here is pool_type.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 21, 2018

Author Member

fixed.

for key, value in kwargs.items():
if type == 'replicated' and key not in \
['name', 'erasure_code_profile_id'] and value is not None:
CephService.send_command('mon', 'osd pool set', pool=pool, var=key, val=value)

This comment has been minimized.

@rjfd

rjfd Mar 16, 2018

Contributor

Isn't this code a bit of dangerous? basically you are calling osd pool set for unknown (key, value) pairs that come from kwargs without any validation.
Since, the frontend is the consumer of this API we are in controll of all things that come inside the request JSON, therefore we should validate them in the backend.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 16, 2018

Author Member

We have validation in C++ and I'm trusting the C++ validation. And if something is not properly validated, I'd favor fixing the C++ validation.

This comment has been minimized.

@rjfd

rjfd Mar 16, 2018

Contributor

OK, I guess this is part of a more generic problem of handling the errors from Ceph library calls, which we still haven't tackled properly.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 21, 2018

Author Member

Indeed. let's tackle this after this PR.

var='compression_required_ratio',
val=str(compression_required_ratio))
if application_metadata:
for app in set(json.loads(application_metadata)):

This comment has been minimized.

@rjfd

rjfd Mar 16, 2018

Contributor

Why are we parsing the application_metadata variable? shouldn't this variable already be an array of strings?

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Mar 21, 2018

Author Member

fixed.

@LenzGr LenzGr changed the title mgr/dashboard_v2: Add create() to Pool controller mgr/dashboard: Add create() to Pool controller Mar 16, 2018

@sebastian-philipp sebastian-philipp force-pushed the sebastian-philipp:dashboard_v2_pool-create branch from 6a1ee39 to 243fdd1 Mar 16, 2018

@sebastian-philipp sebastian-philipp changed the title mgr/dashboard: Add create() to Pool controller [WIP] mgr/dashboard: Add create() to Pool controller Mar 20, 2018

@sebastian-philipp sebastian-philipp force-pushed the sebastian-philipp:dashboard_v2_pool-create branch 2 times, most recently from 4913bd0 to fc9d785 Mar 20, 2018

@sebastian-philipp sebastian-philipp changed the title [WIP] mgr/dashboard: Add create() to Pool controller [WIP] mgr/dashboard: Add Pool-create to the backend Mar 21, 2018

@sebastian-philipp sebastian-philipp force-pushed the sebastian-philipp:dashboard_v2_pool-create branch from fc9d785 to a5910d0 Mar 21, 2018

@sebastian-philipp

This comment has been minimized.

Copy link
Member Author

sebastian-philipp commented Mar 21, 2018

please split the pool create commit in two, so that the args_from_json fix is done in a separate commit.

done.

@sebastian-philipp sebastian-philipp changed the title [WIP] mgr/dashboard: Add Pool-create to the backend mgr/dashboard: Add Pool-create to the backend Mar 21, 2018

@sebastian-philipp sebastian-philipp removed the DNM label Mar 21, 2018

@rjfd

This comment has been minimized.

Copy link
Contributor

rjfd commented Mar 22, 2018

@sebastian-philipp I found some problems while testing in my local environment.

First is because of http://tracker.ceph.com/issues/23345 , and I applied this patch to overcome the issue:

diff --git a/src/pybind/mgr/dashboard/run-backend-api-tests.sh b/src/pybind/mgr/dashboard/run-backend-api-tests.sh
index e7b78e9126..6bc4297d93 100755
--- a/src/pybind/mgr/dashboard/run-backend-api-tests.sh
+++ b/src/pybind/mgr/dashboard/run-backend-api-tests.sh
@@ -91,6 +91,7 @@ EOF
 
     MGR=2 RGW=1 ../src/vstart.sh -n -d
     sleep 10
+    ./bin/ceph config set mon osd_pool_default_erasure_code_profile "plugin=jerasure technique=reed_sol_van k=2 m=1 crush-failure-domain=osd"
     cd $CURR_DIR
 }

The second issue is something that was already broken (not introduced in this PR) but only now is exposed when running test_pool.py. I fixed the issue with the following patch:

diff --git a/qa/tasks/mgr/dashboard/test_auth.py b/qa/tasks/mgr/dashboard/test_auth.py
index b176dd041b..1c71687a4d 100644
--- a/qa/tasks/mgr/dashboard/test_auth.py
+++ b/qa/tasks/mgr/dashboard/test_auth.py
@@ -10,7 +10,6 @@ from .helper import DashboardTestCase
 class AuthTest(DashboardTestCase):
     def setUp(self):
         self.reset_session()
-        self._ceph_cmd(['dashboard', 'set-session-expire', '2'])
         self._ceph_cmd(['dashboard', 'set-login-credentials', 'admin', 'admin'])
 
     def test_a_set_login_credentials(self):
@@ -60,6 +59,7 @@ class AuthTest(DashboardTestCase):
         self.assertStatus(401)
 
     def test_session_expire(self):
+        self._ceph_cmd(['dashboard', 'set-session-expire', '2'])
         self._post("/api/auth", {'username': 'admin', 'password': 'admin'})
         self.assertStatus(201)
         self._get("/api/host")
@@ -67,6 +67,7 @@ class AuthTest(DashboardTestCase):
         time.sleep(3)
         self._get("/api/host")
         self.assertStatus(401)
+        self._ceph_cmd(['dashboard', 'set-session-expire', '1200'])
 
     def test_unauthorized(self):
         self._get("/api/host")

Can you add each patch as a single commit to this PR?

@sebastian-philipp

This comment has been minimized.

Copy link
Member Author

sebastian-philipp commented Mar 22, 2018

@rjfd thanks for the patches! Regarding the run-backend-api-tests.sh issue: @jecluis Do you mind, if we add this workaround for now?

@rjfd

This comment has been minimized.

Copy link
Contributor

rjfd commented Mar 22, 2018

@sebastian-philipp forget about the first patch on run-backend-api-tests.sh script since that problem is now fixed in master due to the merge of this PR #20986

@sebastian-philipp

This comment has been minimized.

Copy link
Member Author

sebastian-philipp commented Mar 23, 2018

@rjfd regarding the second patch: I've created a PR for that: #21021

@rjfd

rjfd approved these changes Mar 23, 2018

Copy link
Contributor

rjfd left a comment

lgtm

@rjfd

This comment has been minimized.

Copy link
Contributor

rjfd commented Mar 23, 2018

jenkins retest this please

@rjfd

This comment has been minimized.

Copy link
Contributor

rjfd commented Mar 23, 2018

@sebastian-philipp "make check" failed, maybe you should rebase the PR on top of master and try "make check" again.

sebastian-philipp added some commits Mar 13, 2018

mgr/dashboard: Refactor `send_command` into CephService
* `CephService.send_command` is much easier to use.
* Refactored `CephFSClients.get` and `Dashboard.load_bufer`

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
mgr/dashboard: Minor tweak to `DashboardTestCase._request`
Don't fall through silently.

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
mgr/dashboard: Add `pool/_info` api endpoint to fill the create-pool …
…dialog

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
mgr/dashboard: Change `crush_rule` serialization of Pools from id to …
…name

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
mgr/dashboard: Changed `@RESTController.args_from_json`
It now allows any keyword arguments. This is needed for ECP
and Pool creation.

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
mgr/dashboard: Add create() to Pool controller
Also:

* Added tests.
* Renamed `DashboardTest` to `PoolTest`.
* Added `delete()`.

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
mgr/dashboard: Add Teuthology test for `/api/pool/_info`
Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>

@sebastian-philipp sebastian-philipp force-pushed the sebastian-philipp:dashboard_v2_pool-create branch from 3e2365c to cbee371 Mar 23, 2018

@sebastian-philipp

This comment has been minimized.

Copy link
Member Author

sebastian-philipp commented Mar 23, 2018

@rjfd rebased, but now I'm running into #21004 (comment)

@rjfd rjfd merged commit d5763de into ceph:master Mar 26, 2018

4 of 5 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
make check make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment