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

osd/OSDMap: replace require_*_osds flags with a single require_osd_release field #15068

Merged
merged 5 commits into from May 30, 2017

Conversation

Projects
None yet
3 participants
@liewegas
Member

liewegas commented May 12, 2017

This avoids consuming a bit for each release and is easier to reason about
(although the transition is a bit awkward).

"set <key>", "osd", "rw", "cli,rest")
COMMAND("osd unset " \
"name=key,type=CephChoices,strings=full|pause|noup|nodown|noout|noin|nobackfill|norebalance|norecover|noscrub|nodeep-scrub|notieragent", \
"unset <key>", "osd", "rw", "cli,rest")
COMMAND("osd require-osd-release "\
"name=release,type=CephChoices,strings=luminous",
"set the minimum allowed OSD release to participate in teh cluster",

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

s/teh/the/

@@ -1151,7 +1151,7 @@ void OSDMonitor::encode_pending(MonitorDBStore::TransactionRef t)
tmp.deepish_copy_from(osdmap);
tmp.apply_incremental(pending_inc);
if (tmp.test_flag(CEPH_OSDMAP_REQUIRE_LUMINOUS)) {
if (tmp.require_osd_release > CEPH_RELEASE_LUMINOUS) {

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

s/>/>=/

cmd_getval(g_ceph_context, cmdmap, "release", release);
if (!osdmap.test_flag(CEPH_OSDMAP_SORTBITWISE)) {
ss << "the sortbitwise flag must be set first";
err = -EPERM;

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

should goto reply; here.

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

@liewegas ping? we should not fall thru here.

@@ -470,6 +470,9 @@ void OSDMap::Incremental::encode(bufferlist& bl, uint64_t features) const
if (target_v >= 4) {
::encode(new_require_min_compat_client, bl);
}
if (target_v >= 5) {
::encode(new_require_osd_release, bl);

This comment has been minimized.

@tchaikov

tchaikov May 15, 2017

Contributor

shall we translate new_require_osd_release to the new_flags bits if target_v < 5?

This comment has been minimized.

@liewegas

liewegas May 15, 2017

Member

I was originally going to do that, but (1) we don't have the OSDMap so we can't initialize new_flags if it is still -1, and (2) the only daemons that care about these flags or require_osd_release are the mon and osd, and once require_luminous is set they will always have them set. I think the only exception would be if someone tries to start an old daemon; the mon will be locked out due to the existing required mon feature, and the osd will be prevented from booting by the mon.

@@ -156,6 +156,19 @@ extern const char *ceph_osd_state_name(int s);
CEPH_OSDMAP_REQUIRE_KRAKEN | \
CEPH_OSDMAP_REQUIRE_LUMINOUS | \
CEPH_OSDMAP_SORTBITWISE)
#define CEPH_OSDMAP_LEGACY_REQUIRE_FLAGS (CEPH_OSDMAP_REQUIRE_JEWEL| \

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

nit, after a space after CEPH_OSDMAP_REQUIRE_JEWEL

/*
* major ceph release numbers
*/
#define CEPH_RELEASE_JEWEL 10

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

nit, could be

static constexpr uint8_t CEPH_RELEASE_JEWEL = 10;

as long as we neither take the address of it nor pass it by reference.

This comment has been minimized.

@liewegas

liewegas May 21, 2017

Member

this is a C header that is shared by the kernel; hence the antiquated style :)

@@ -49,6 +49,22 @@ const char *ceph_osd_state_name(int s)
}
}
const char *ceph_osd_release_name(int r)

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

could s/int/uint8_t/ to be more consistent. or better off typedef/using a new type.

cmd_getval(g_ceph_context, cmdmap, "release", release);
if (!osdmap.test_flag(CEPH_OSDMAP_SORTBITWISE)) {
ss << "the sortbitwise flag must be set first";
err = -EPERM;

This comment has been minimized.

@tchaikov

tchaikov May 20, 2017

Contributor

@liewegas ping? we should not fall thru here.

@liewegas

This comment has been minimized.

Member

liewegas commented May 21, 2017

fixed!

liewegas added some commits May 12, 2017

osd/OSDMap: add require_osd_release numeric field, CEPH_RELEASE_*
Instead of eating up a flag for each release, which is super-awkward and
annoying, just keep a numeric release version min.

Signed-off-by: Sage Weil <sage@redhat.com>
mon: new 'osd require-osd-release' command
Note that we only allow setting this to 'luminous'.  If the 'jewel' or 'kraken'
flags need to be set that can be done through the legacy interface.

Signed-off-by: Sage Weil <sage@redhat.com>
osd: replace require_*_osds flags with require_osd_release field
- OSDMap encode and decode translate between the flags and int
representations.
- OSDMap::Incremental only does decode; we do not expect to ever encode
an incremental osdmap for an old osd that sets any of these flags.
- the 'osd set' command still lets you set the jewel and kraken flags,
but not luminous.
- OSDMap::apply_incremental handles the conversion of legacy require flags
to the new field if the jewel or kraken flags have to be set before
starting the osd upgrade.
- clear out the legacy flags when we make the luminous transition only;
until then we keep using the old flag in the encoded and decoded version
(although the require_osd_release field will be accurate in memory in all
cases).

Signed-off-by: Sage Weil <sage@redhat.com>
osd: replace all require_*_osds flag checks with a require_osd_releas…
…e comparison

Signed-off-by: Sage Weil <sage@redhat.com>
mon/OSDMonitor: fix osd metadata update ordering
Do not do both.

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

@liewegas liewegas merged commit 5988651 into ceph:master May 30, 2017

2 of 3 checks passed

default Build started sha1 is merged.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details

@liewegas liewegas deleted the liewegas:wip-osdmap-require branch May 30, 2017

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