Async iprop fixes #347

nicowilliams · 2017-10-19T01:45:39Z

This is still WIP in that tests/kdc/check-iprop needs to test more things. Also, I need to test graceful rolling upgrade semantics, though that I won't script.

TODO:

on non-reinit truncation, make sure we always keep at least one non-uber record
add version number wraparound protection -- probably just reinit when the latest record's version gets too close to wrapping
test more cases in check-iprop
test two slaves in check-iprop and check that send_complete()s interleave -- this is going to be very difficult to check for considering how small the HDB is in check-iprop, so maybe this will be better done by hand
finish the non-blocking I/O work in ipropd-master to make sure that no slave can cause the master to block without servicing the others.

In preparation for fixes to iprop issues we now log a record immediately after writing the uber record on initialization. This will help the system understand when a slave needs a complete dump. It also gives us a proper timestamp for the time of [re-]initialization.

This commit extends the protocol in various ways, with what should be graceful rolling upgrade semantics (you should be able to upgrade the master and the slaves in any order and not all at once). The master and slave now identify state not by a single uint32_t version number, but also by the timestamp at which an iprop log entry was recorded at that version. This makes reinitialization of iprop safe. This means: - protocol messages were extended: - I_HAVE - TELL_YOU_EVERYTHING (which appears in the dump) - NOW_YOU_HAVE (which also appears in the dump) - master datastructures were extended - the dump format was extended in that the TELL_YOU_EVERYTHING / NOW_YOU_HAVE brackets were extended - a previous commit ensures that the log always has at least two entries: the uber record and a first record, which means that we always have at least one record whose version and timestamp can be used to help establish identify synchronization state

nicowilliams · 2017-10-19T01:47:04Z

@vdukhovni This isn't ready for code review, but if you want to take a look at least at the commit commentary and the PR commentary, that would be great. The changes to lib/kadm5/log.c are pretty straightforward. The meat of this is all in the master and slave code.

nicowilliams · 2017-10-19T16:07:02Z

It would help (and would have helped) a lot to create a data structure for representing versions as {vno, timestamp} rather than carrying around both items, and then defining a sort equality predicate for the two items.

nicowilliams · 2017-10-19T16:09:14Z

For version number wraparound protection... provided we skip version zero (which is always the uber record), maybe the only special thing to do is to make send_diffs() able to notice that version, say, 2^32 - 10 from a slave is "smaller" than version 3 on the master.

EDIT: I think we want a couple of predicates:

version_in_range(first_version, last_version, given_version)
version_eq(first_version, last_version, vno1, tstamp1, vno2, tstamp2)

and then send_diffs() would use those two instead of the expressions it uses now. Those predicates could be macros with multiple evaluation hazards.

EDIT: We'd have to assume or ensure that zero is never crossed more than once in the iprop log...

nicowilliams · 2018-12-25T22:36:21Z

The thought occurs that iprop-{slave, master} are perfect candidates for writing in a language that makes async easy/trivial. Haskell, node, whatever.

jaltman · 2018-12-26T00:43:09Z

Now that Linux has a native AF_RXRPC socket classI wonder if it's worth implementing a ubik replicated database.

nicowilliams · 2018-12-26T01:36:05Z

The Ubik thing would be fine. I'll still plow on (slowly) with a PostgreSQL backend though.

nicowilliams · 2018-12-26T01:36:42Z

What I'd really like though is to not have an iprop protocol anymore, and ubik and PG both would give us that.

lhoward · 2018-12-26T00:28:06Z

lib/kadm5/iprop-log.c

+{
+    time_t tt = t;
+
+    if (sizeof(tt) > sizeof(t))


There’s now a preprocessor definition for the size of time_t although that’s not going to make any practical difference.

Although I’d prefer to use krb5_timestamp instead of time_t so we don’t have three types for times...

lhoward · 2018-12-26T00:31:48Z

lib/kadm5/ipropd_master.c

+    if (sp != NULL)
+        ret = krb5_ret_uint32(sp, &op);
+    if (ret == 0 && op != TELL_YOU_EVERYTHING)
+        ret = EINVAL; /* XXX */


Is there a krb5 error code we can use?

lhoward · 2018-12-26T00:35:09Z

lib/kadm5/log.c

+    uint32_t now = time(NULL);
+
+    assert(now > 0 && now < UINT32_MAX);
+    if (now > 0 && now < UINT32_MAX)


Hmm, shouldn’t we make now time_t and return an error if we are past 2038 rather than asserting? Why do I have the feeling we might still be working on this code in another 20 years...

time_t is not guaranteed to be larger than 32-bit. krb5_timestamp defined as time_t. Perhaps we should define krb5_timestamp as int64_t to ensure that we always have support for post 2038 times.

AuriStorFS takes that approach.

I suspect that will break other things (I just wrote some code that relies on that assumption the other day!). But yes...

@jaltman unsigned 32-bit will get you past 2038. You won't care about 68 years later.

Anyways, this stuff goes on the wire. We can't change it without a flag day or a new protocol. I won't build a new iprop. I want to not be in that business. I want to move to a PG-based backend and do all replication in PostgreSQL.

In any case, this code will go past 2038. The rest of Heimdal, I dunno, I don't recall and I'm busy with the gss_add_cred() rewrite :) I'll look when I'm done with that.

The internal type and the wire protocol are orthogonal. We can consistently represent timestamps internally using signed 64-bit values while at the same time sending signed 32-bit values on the wire.

lhoward · 2018-12-26T02:57:03Z

The code LGTM but I don't know enough about iprop to know why it's failing the tests, @jaltman do you want me to investigate this further or is it better to let @nicowilliams do it?

jaltman · 2018-12-26T06:00:47Z

@nicowilliams will you have time to determine why the check-iprop test is failing with this pull request?

nicowilliams · 2018-12-26T06:06:02Z

@jaltman maybe later this week.

lhoward · 2018-12-29T07:38:25Z

rebased against master, check-iprop failing with

2018-12-29T18:36:08 wrote new dumpfile (version 34)
2018-12-29T18:36:08 send_diffs: failed to find previous entry: Incremental propagation log got corrupted
2018-12-29T18:36:08 message from master
2018-12-29T18:36:08 master sent us a full dump
2018-12-29T18:36:08 master sent us a full dump version 34
2018-12-29T18:36:08 Waiting 30 seconds to restart
2018-12-29T18:36:08 master sent us a full dump version 34, time 1546068967
2018-12-29T18:36:08 master says it's at 34, 1546068967
2018-12-29T18:36:08 receive complete database (34, 1546068967)
2018-12-29T18:36:08 error message: failed to open ../../tests/kdc/mkey.file: No such file or directory: 2
2018-12-29T18:36:08 krb5_read_priv_message: End of file
2018-12-29T18:36:08 db->close: End of file
2018-12-29T18:36:08 Waiting 2 minutes to restart

nicowilliams · 2019-02-21T19:01:47Z

In order to get this into 8.0 I should split this up so that only the async I/O changes in ipropd-master are in this PR. The rest is mostly about iprop log version number wraparound protection and year 2038 problem remediation.

nicowilliams · 2019-11-26T20:30:10Z

This has been superseded by new, recent work that finally made ipropd-master async and able to interleave large full/incremental props to its clients so that no one slave can cause others to miss incrementals that then cause them to do full props.

nicowilliams added 4 commits October 18, 2017 20:20

Fix remaining time_t/uin32_t confusion in log.c

c10f495

Fix time_t/uint32_t confusion in iprop-log

d1ad244

nicowilliams requested a review from vdukhovni October 19, 2017 01:45

Add ipropd-* --protocol-version option for testing

4245fc8

WIP: Use struct for iprop vno & tstamp

3808ac6

jaltman added this to the Heimdal 8 milestone Dec 14, 2018

jaltman assigned lhoward and nicowilliams Dec 25, 2018

lhoward reviewed Dec 26, 2018

View reviewed changes

nicowilliams closed this Nov 26, 2019

nicowilliams deleted the async-iprop branch November 26, 2019 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Async iprop fixes #347

Async iprop fixes #347

nicowilliams commented Oct 19, 2017

nicowilliams commented Oct 19, 2017

nicowilliams commented Oct 19, 2017

nicowilliams commented Oct 19, 2017 •

edited

Loading

nicowilliams commented Dec 25, 2018

jaltman commented Dec 26, 2018 via email

nicowilliams commented Dec 26, 2018

nicowilliams commented Dec 26, 2018

lhoward Dec 26, 2018

lhoward Dec 26, 2018

lhoward Dec 26, 2018

lhoward Dec 26, 2018

jaltman Dec 28, 2018

lhoward Dec 28, 2018

nicowilliams Dec 28, 2018

nicowilliams Dec 28, 2018

jaltman Jun 8, 2019

lhoward commented Dec 26, 2018

jaltman commented Dec 26, 2018

nicowilliams commented Dec 26, 2018

lhoward commented Dec 29, 2018

nicowilliams commented Feb 21, 2019

nicowilliams commented Nov 26, 2019

Async iprop fixes #347

Async iprop fixes #347

Conversation

nicowilliams commented Oct 19, 2017

nicowilliams commented Oct 19, 2017

nicowilliams commented Oct 19, 2017

nicowilliams commented Oct 19, 2017 • edited Loading

nicowilliams commented Dec 25, 2018

jaltman commented Dec 26, 2018 via email

nicowilliams commented Dec 26, 2018

nicowilliams commented Dec 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhoward commented Dec 26, 2018

jaltman commented Dec 26, 2018

nicowilliams commented Dec 26, 2018

lhoward commented Dec 29, 2018

nicowilliams commented Feb 21, 2019

nicowilliams commented Nov 26, 2019

nicowilliams commented Oct 19, 2017 •

edited

Loading