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/MgrStatMonitor: do not crash on luminous dev version upgrades #16287

Merged
merged 1 commit into from Jul 12, 2017

Conversation

Projects
None yet
5 participants
@liewegas
Member

liewegas commented Jul 12, 2017

Signed-off-by: Sage Weil sage@redhat.com

mon/MgrStatMonitor: do not crash on luninous dev version upgrades
Signed-off-by: Sage Weil <sage@redhat.com>
@ukernel

This comment has been minimized.

Member

ukernel commented Jul 12, 2017

LGTM

@joscollin joscollin changed the title from mon/MgrStatMonitor: do not crash on luninous dev version upgrades to mon/MgrStatMonitor: do not crash on luminous dev version upgrades Jul 12, 2017

@joscollin

LGTM. Need to print e msg inside catch ?

@joscollin joscollin added the needs-qa label Jul 12, 2017

@jecluis

This comment has been minimized.

Member

jecluis commented Jul 12, 2017

fwiw, I hit this issue and upon cherry-picking this patch everything went back to working seamlessly.

@liewegas liewegas merged commit 808bd2c into ceph:master Jul 12, 2017

4 checks passed

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
make check (arm64) make check succeeded
Details

@liewegas liewegas deleted the liewegas:wip-try-decode branch Jul 12, 2017

<< " service_map e" << service_map.epoch << dendl;
}
catch (buffer::error& e) {
derr << "failed to decode mgrstat state; luminous dev version?" << dendl;

This comment has been minimized.

@gregsfortytwo

gregsfortytwo Jul 12, 2017

Member

Can we do some kind of tagging so we know that's what happened going forward?

Or are all the upgrade paths certain to be safe with default values of those members? Given the previous discussion on its uses I'm a little worried about losing the service map.

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 12, 2017

@gregsfortytwo

This comment has been minimized.

Member

gregsfortytwo commented Jul 12, 2017

Yeah I'm just worried that the way it's formulated we could (basically silently) lose data long after the upgrades are done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment