On Tue, Sep 12, 2017 at 8:55 PM, Sage Weil <sage@xxxxxxxxxxxx> wrote: > On Tue, 12 Sep 2017, Joao Eduardo Luis wrote: > >> Hi everyone, >> >> >> As you all are certainly aware, the monitors are quite the critical part of >> Ceph: if we don't have a quorum, the cluster will cease responding to >> requests. For this reason, we tend to be extremely conservative when it comes >> to changing or adding things, and, personally, I've grown a bit paranoid when >> it comes to change them significantly. >> >> However, I believe it is time we change quite a bit on the monitors, to avoid >> mistakes that have become easier to make as time passed, and to make the >> monitors a lot more resilient. While most of these issues have always been >> present in our minds, as our contributor base grows we are bound to have >> people changing parts of the code that may be trickier, and whose nuances may >> have annoying side-effects. >> >> This became more obvious over the course of Luminous (although this has been a >> problem for longer); I've noticed that there were quite a few bugs introduced >> in the monitors due to how tricky it is to deal with all the paxos services' >> internal data structures consistency requirements. >> >> To illustrate this a bit, the most common sources of silent bugs (since ever, >> really) tend to be: >> >> - changing the pending map values based on uncommitted state; >> - changing the committed map value; >> - changing pending map values and not waiting for the value to be committed >> before returning to the user; >> - validating error conditions after changing the pending map, and returning >> error having changed pending map values. >> >> On top of this, we sometimes get a few bugs resulting from returning either >> 'true' or 'false' in a 'preprocess_query()' when it should have been the >> opposite. >> >> I've also realized we've had a few other bugs that resulted from >> misunderstanding how the new 'paxos plug' mechanism works. This was a >> mechanism we introduced to allow cross-service proposals in one go, but at the >> time we didn't add any sanity or validation mechanisms to ensure >> readability/writeability on individual services. While this decision >> guaranteed that we would not be destabilizing the monitors just before the >> release, it introduced a new vector for mistakes to happen. >> >> Over the course of Mimic I intend to go over a significant rework of the paxos >> services (discussing on CDM, lists), with the intent to make them easier to >> contribute to. Whatever change we introduce will likely not be a silver >> bullet, and the changes will need to be conservative and incremental. I will >> make an effort to accompany any changes with proper developer documentation -- >> although this should also be done for the current interface :) > > I think one good place to start would be building a framework around the > CLI commands. If you look at FSCommand it provides a structured way to > implement commands that doesn't add to the nasty if/else pile in the > OSDMonitor and elsewhere. Yes, I've been meaning to bring FSCommand over to MgrMonitor.cc too. If folks are happy with that as a convenience function then we can share out the work of converting the monster if/else blocks. >> In the mean time -- although this should be viewed as a general rule --, >> please be mindful of any significant changes you introduce. We would prefer >> smaller, incremental changes, unless otherwise unavoidable. >> >> And, please, always have someone familiar with the monitors reviewing it >> first, before merging. I would suggest me and/or Kefu, at the very least. And >> if, for some reason, we are not responsive, feel free to poke us on IRC or the >> list*. > > +1 Agreed! John > > sage > >> >> >> Cheers! >> >> -Joao >> >> * Sorry Kefu, I just told everyone to poke you for PRs ;) >> -- >> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
