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

[DNM] osd/PG: Remove redundant advmap reaction #12971

Closed

Conversation

badone
Copy link
Contributor

@badone badone commented Jan 18, 2017

In the Incomplete state there is no longer any reason to check for a
min_size reduction in an advancing map as a reduction in min_size has no
bearing on whether we can recover from Incomplete.

Signed-off-by: Brad Hubbard bhubbard@redhat.com

In the Incomplete state there is no longer any reason to check for a
min_size reduction in an advancing map as a reduction in min_size has no
bearing on whether we can recover from Incomplete.

Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
@tchaikov tchaikov self-assigned this Jan 18, 2017
@liewegas liewegas changed the title PG: Remove redundant advmap reaction osd/PG: Remove redundant advmap reaction Jan 18, 2017
@liewegas
Copy link
Member

Here:

  // We go incomplete if below min_size for ec_pools since backfill
  // does not currently maintain rollbackability
  // Otherwise, we will go "peered", but not "active"
  if (num_want_acting < pool.info.min_size &&
      (pool.info.ec_pool() ||
       !cct->_conf->osd_allow_recovery_below_min_size)) {
    want_acting.clear();
    dout(10) << "choose_acting failed, below min size" << dendl;
    return false;
  }

from choose_acting(). If min_size is reduced, then we may be able to go active...

@badone badone changed the title osd/PG: Remove redundant advmap reaction [DNM] osd/PG: Remove redundant advmap reaction Jan 18, 2017
@badone
Copy link
Contributor Author

badone commented Jan 18, 2017

@liewegas looking into this further. I believe this code is still redundant since the default behaviour for the statechart is a forward_event to the next outer state (Peering) and should result in a transition to reset anyway but I need to confirm that and also look at the code you pointed out. This likely means #12936 needs to be revisited and notation added that this all only applies to EC.

shinobu-x pushed a commit to shinobu-x/ceph that referenced this pull request Mar 4, 2017
This was being ignored in new code that tested
the per-pool full flags.  Refactor it so that
all the tests of full flags go through _osdmap_pool_full
and thereby experience the honor_osdmap_full policy.

Fixes: ceph#12971
Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit 4d5d15c)

Conflicts:
	src/osdc/Objecter.cc
	src/osdc/Objecter.h
shinobu-x pushed a commit to shinobu-x/ceph that referenced this pull request Mar 4, 2017
This was being ignored in new code that tested
the per-pool full flags.  Refactor it so that
all the tests of full flags go through _osdmap_pool_full
and thereby experience the honor_osdmap_full policy.

Fixes: ceph#12971
Signed-off-by: John Spray <john.spray@redhat.com>
(cherry picked from commit 4d5d15c)

Conflicts:
	src/osdc/Objecter.cc
	src/osdc/Objecter.h
@badone
Copy link
Contributor Author

badone commented Mar 19, 2017

@liewegas finally got to look at this agin and see what you mean now. I guess we could still remove this code if we moved the check for min_size reduction to the Peering advmap reaction but it probably is more explicit and future proof where it is. Closing this.

@badone badone closed this Mar 19, 2017
@badone badone deleted the wip-incomplete-advmap-min_size-fix branch March 19, 2017 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants