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

mon/MDSMonitor.cc:refuse fs new on pools with obj #12825

Merged
merged 1 commit into from Jan 27, 2017

Conversation

Projects
None yet
4 participants
@stiopaa1
Copy link
Contributor

stiopaa1 commented Jan 8, 2017

Fixes: http://tracker.ceph.com/issues/11124
Signed-off-by: Michal Jarzabek stiopa@gmail.com

@tchaikov

This comment has been minimized.

Copy link
Contributor

tchaikov commented Jan 9, 2017

@stiopaa1 the task of test_fsnew is not referenced anywhere, you might want to add it to a new yaml file under qa/suites/fs/basic/tasks maybe? @ukernel @jcsp what do you think?

@tchaikov tchaikov added the cephfs label Jan 9, 2017

@@ -1581,6 +1582,12 @@ int MDSMonitor::management_command(
return -ENOENT;
}

int64_t metadata_num_objects = mon->pgmon()->pg_map.pg_pool_sum[metadata].stats.sum.num_objects;

This comment has been minimized.

Copy link
@jcsp

jcsp Jan 9, 2017

Contributor

It has just occurred to me that we will sometimes want to permit this, in order to enable people to do an "fs new", "fs reset" type procedure -- that is permitted for people who make the mistake of doing an "fs rm" but still have all their objects. Could you add a --force flag here?

from tasks.cephfs.cephfs_test_case import CephFSTestCase
from teuthology.orchestra.run import CommandFailedError

class TestFSNew(CephFSTestCase):

This comment has been minimized.

Copy link
@jcsp

jcsp Jan 9, 2017

Contributor

Could you move this file to test_misc.py please? That way it will get picked up and run and we avoid the need to create a separate yaml as @tchaikov mentions (it makes our tests take a bit less time if we bundle things like that, it's all a bit arbitrary though!)


dummyfile = '/etc/fstab'

self.fs.put_metadata_object_raw("key", dummyfile)

This comment has been minimized.

Copy link
@jcsp

jcsp Jan 9, 2017

Contributor

You could just use an inline string like "foobar" instead of dummyfile here -- put_metadata_object_raw isn't reading a file or anything like that.

This comment has been minimized.

Copy link
@stiopaa1

stiopaa1 Jan 9, 2017

Author Contributor

I'm not sure I understand. Isn't this calling:
bin/rados -p cephfs_metadata put key "/etc/fstab"
which I thought requires a file as last argument?

@@ -0,0 +1,64 @@
import errno
import time
from unittest import SkipTest

This comment has been minimized.

Copy link
@jcsp

jcsp Jan 9, 2017

Contributor

SkipTest import is unused


class TestFSNew(CephFSTestCase):
def test_fs_new(self):
data_pool_name = "{0}_data".format(self.fs.name)

This comment has been minimized.

Copy link
@jcsp

jcsp Jan 9, 2017

Contributor

You can use self.fs.get_data_pool_name()

timeout = 10
elapsed = 0

while True:

This comment has been minimized.

Copy link
@jcsp

jcsp Jan 9, 2017

Contributor

There is a wait_until_true function for situations like this. Something like:

self.wait_until_equal(lambda: self.fs.get_pool_df(self.fs.metadata_pool_name)['objects'] > 0, timeout=timeout)
@stiopaa1

This comment has been minimized.

Copy link
Contributor Author

stiopaa1 commented Jan 9, 2017

@jcsp
I've made the changes you requested (apart from 1, look at the comment above)

@jcsp

This comment has been minimized.

Copy link
Contributor

jcsp commented Jan 12, 2017

Oops, you are correct about the put_metadata_object_raw usage, I had misread it

@jcsp

jcsp approved these changes Jan 13, 2017

@jcsp

This comment has been minimized.

Copy link
Contributor

jcsp commented Jan 13, 2017

@stiopaa1 thanks for updating, this is ready to merge, please squash

@stiopaa1 stiopaa1 force-pushed the stiopaa1:r11124_refuseToDoFsNew branch from 59cc219 to 7942dc7 Jan 15, 2017

@stiopaa1

This comment has been minimized.

Copy link
Contributor Author

stiopaa1 commented Jan 15, 2017

@jcsp
squashed

@jcsp

This comment has been minimized.

Copy link
Contributor

jcsp commented Jan 16, 2017

Ah, now we can see that this also needs an update to the test script in qa/workunits/cephtool/test.sh:

../qa/workunits/cephtool/test.sh:911: test_mon_mds:  set +e
../qa/workunits/cephtool/test.sh:912: test_mon_mds:  ceph fs new cephfs fs_metadata mds-ec-pool
../qa/workunits/cephtool/test.sh:913: test_mon_mds:  check_response erasure-code 22 22
../qa/workunits/cephtool/test.sh:94: check_response:  expected_string=erasure-code
../qa/workunits/cephtool/test.sh:95: check_response:  retcode=22
../qa/workunits/cephtool/test.sh:96: check_response:  expected_retcode=22
../qa/workunits/cephtool/test.sh:97: check_response:  '[' 22 -a 22 '!=' 22 ']'
../qa/workunits/cephtool/test.sh:102: check_response:  grep --quiet -- erasure-code /tmp/cephtool.4bj/test_invalid.XuU
../qa/workunits/cephtool/test.sh:103: check_response:  echo 'Didn'\''t find erasure-code in output'
Didn't find erasure-code in output
../qa/workunits/cephtool/test.sh:104: check_response:  cat /tmp/cephtool.4bj/test_invalid.XuU
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2017-01-16 11:20:33.164255 7fd739767700 -1 WARNING: all dangerous and experimental features are enabled.
2017-01-16 11:20:33.178010 7fd739767700 -1 WARNING: all dangerous and experimental features are enabled.
Error EINVAL: pool 'fs_metadata' already contains some objects. Use another pool which is empty.
../qa/workunits/cephtool/test.sh:105: check_response:  exit 1
../qa/workunits/cephtool/test.sh:1: check_response:  rm -fr /tmp/cephtool.4bj

You can debug this on a local vstart cluster by invoking it from a built directory something like this:
PATH=$PATH:bin/ SUDO="" ../qa/workunits/cephtool/test.sh --test-mds

@stiopaa1 stiopaa1 force-pushed the stiopaa1:r11124_refuseToDoFsNew branch from 7942dc7 to 21030ff Jan 22, 2017

@stiopaa1

This comment has been minimized.

Copy link
Contributor Author

stiopaa1 commented Jan 23, 2017

@jcsp
I have updated the test script

int64_t metadata_num_objects = mon->pgmon()->pg_map.pg_pool_sum[metadata].stats.sum.num_objects;
if (force != "--force" && metadata_num_objects > 0) {
ss << "pool '" << metadata_name
<< "' already contains some objects. Use another pool which is empty.";

This comment has been minimized.

Copy link
@jecluis

jecluis Jan 23, 2017

Member

a native english speaker please correct me, but this phrase feels funny. I'd personally go with "Use an empty pool instead", or something of the sorts. The which is empty portion makes me all tingly (I should probably see a doctor about that...)

This comment has been minimized.

Copy link
@jcsp

jcsp Jan 23, 2017

Contributor

It's only slightly awkward. "Use an empty pool instead" would be a bit simpler (and maybe also easier to parse for users with limited english?). Up to @stiopaa1 if you want to come back and edit this.

This comment has been minimized.

Copy link
@stiopaa1

stiopaa1 Jan 23, 2017

Author Contributor

@jecluis @jcsp
Yeah, I agree that "Use an empty pool instead" sounds better. Will change it later today.

mon/MDSMonitor.cc:refuse fs new on pools with obj
Fixes: http://tracker.ceph.com/issues/11124
Signed-off-by: Michal Jarzabek <stiopa@gmail.com>

@stiopaa1 stiopaa1 force-pushed the stiopaa1:r11124_refuseToDoFsNew branch from 21030ff to 052c3d3 Jan 23, 2017

@stiopaa1

This comment has been minimized.

Copy link
Contributor Author

stiopaa1 commented Jan 23, 2017

updated to "Use an empty pool instead"

@jcsp jcsp merged commit 48adc77 into ceph:master Jan 27, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.