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.

Sign up for GitHub

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

Jump to bottom

mon: fixes related to mondbstore->get() changes #6564

Merged
merged 7 commits into from Dec 17, 2015
Merged

mon: fixes related to mondbstore->get() changes #6564

merged 7 commits into from Dec 17, 2015

Conversation

branch-predictor
Copy link
Contributor

Mon, in general, was assuming that db store doesn't return errors when key
is not found, and when 66b7b92 was
introduced, an assert was being triggered in AuthMonitor which turned out
to be wrong. This changeset fixes that and the following issues stemming
from that change:

  • assert() being triggered prematurely in sync_obtain_latest_monmap
  • no user-readable error message is outputted when load_metadata() fails
  • unnecessary key existence checks in ConfigKeyService::store_get and
    Monitor::check_fsid
  • attempts to decode keyring from empty bufferlist when store->get()
    doesn't find the key
  • Monitor::read_features_off_disk uses extra bufferlist, when the other one
    is already empty

This changeset also reverts d548b5f which
is a revert of 66b7b92.

Signed-off-by: Piotr Dałek piotr.dalek@ts.fujitsu.com

@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9263/

@jecluis jecluis self-assigned this Nov 13, 2015
@loic-bot
Copy link

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9288/

liewegas
liewegas reviewed Nov 24, 2015
View reviewed changes
src/mon/MonitorDBStore.h
int r = db->get(prefix, key, &outbl);
if (!r) {
bl.append(outbl);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be further simplified to just pass &bl to db->get()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if we can assert that db->get won't totally overwrite bl (i. e. clear() + append()) and/or every case of MontitorDBStore->get() doesn't care about existing contents (if any) of passed bl. I wasn't sure about that and decided to leave this like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had different idea. Instead of that, it would be possible to add something like value_as_ptr() to KeyValueDB iterators, which return bufferptr instead of entire bufferlist. That way, db->get would be appending returned bufferptr to specified bufferlist, removing the need for two interim bufferlists (one in KeyValueDBIter->value() and another one in MonitorDBStore). Not sure if this should go with this PR though...

@loic-bot
Copy link

loic-bot commented Dec 4, 2015

FAILURE: http://jenkins.ceph.dachary.org/job/ceph/9841/

@ghost
Copy link

ghost commented Dec 4, 2015

@branch-predictor the bot failure is due to a temporary network failure, could you please rebase and repush to get a fresh run ? Sorry for the inconvenience.

@loic-bot
Copy link

loic-bot commented Dec 4, 2015

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9843/

@branch-predictor
Copy link
Contributor Author

Went through some testing: http://149.202.170.61:8081/ubuntu-2015-12-05_13:32:24-rados:monthrash-bp-fix-mon-kv-store---basic-openstack/

Piotr Dałek added 7 commits December 7, 2015 14:26
mon: remove unnecessary key existence check
eec4611 
Since store->get() returns -ENOENT when key doesn't exists, there's no longer
needed to explicitly check for its existence.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
mon/AuthMonitor: don't crash on ENOENT
1e6cda0 
When MonDBStore returns ENOENT when trying to get mkfs keyring, monitor
crashes with assert. On the other hand, if it returned 0 and empty
bufferlist, all was fine - so make it decode the bufferlist only when
MonDBStore returned 0, don't fail on ENOENT (and don't try to decode
empty bufferlist in that case), and fail hard otherwise.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
mon/MonitorDBStore: use single-key LevelDB::get() method
f5a7a3b 
Make MonitorDBStore use single-key ::get() method for some performance
increase in heavy K/V workloads.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
mon/Monitor.cc: Reuse empty bufferlist
3da96b9 
When Monitor::read_features_off_disk doesn't find features on disk, it
tries to write them back to disk. featuresbl is known to be empty, so just
reuse it instead of constructing second one.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
mon/ceph_mon.cc: fail on any error, not just empty magicbl
08ae31c 
Assume failure when either MonDBStore returns error or returns empty
bufferlist.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
mon/Monitor.cc: attempt to decode and extract keyring only if it is f…
bb4936a 
…ound.

Monitor::preinit wrongly assumes that keyring always exists. This worked
so far because the logic that followed made this error invisible.
Change the logic so no attempt is taken to decode empty bufferlist, and
in turn - to extract_save_mon_key from empty keyring.

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
mon/Monitor.cc: improve error handling
1d84219 
sync_obtain_latest_monmap will fail with assertion failure if store->get()
returns error, but there's an extra assert for ENOENT error which causes mon
to crash without putting any extra info into derr. Remove this extra assert.

When call to load_metadata will fail in get_mon_metadata, no human-readable
error is emitted. Add cpp_strerror to output.

check_fsid() has checked for cluster_uuid twice, once in store->exists, then
in store->get(), which is unnecessary because store->get returns correct
error codes.

Added two comments regrding code that doesn't handle (at all) errors from
store->get().

Signed-off-by: Piotr Dałek <piotr.dalek@ts.fujitsu.com>
@loic-bot
Copy link

loic-bot commented Dec 7, 2015

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/9922/

@branch-predictor
Copy link
Contributor Author

@jecluis @liewegas ping

liewegas added a commit that referenced this pull request Dec 17, 2015
@liewegas
Merge pull request #6564 from branch-predictor/bp-fix-mon-kv-store
70240f7 
mon: fixes related to mondbstore->get() changes

Reviewed-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit 70240f7 into ceph:master Dec 17, 2015
@branch-predictor branch-predictor deleted the bp-fix-mon-kv-store branch January 24, 2018 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jecluis jecluis

Labels
bug-fix core needs-qa wip-sage-testing
Projects
None yet
Milestone
No milestone
4 participants
@branch-predictor @loic-bot @liewegas @jecluis