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

Time skew fixes during initial replication #1155

Closed
wants to merge 2 commits into from

Conversation

abbra
Copy link
Contributor

@abbra abbra commented Oct 16, 2017

This patchset implements time skew fixes for initial replication as required in the bug https://bugzilla.redhat.com/show_bug.cgi?id=1493150. This approach allows creating or re-initializing replicas in the situation when there are time differences between data centers.

Note that by default 389-ds does not allow for time skew in replication, this is generally a good approach and should be our default state. This patchset makes possible to unlock an impasse with cases where one would get replication state out of control due to various external reasons like a failed attempt at a parallel IPA upgrade.

@rcritten
Copy link
Contributor

Untested yet but I like the approach. I wonder if a block comment would be appropriate in the dsinstance methods to describe the reasoning for ignoring/preventing time skew. I'm not usually a fan of embedding ticket #'s into code but I wonder if this is complex enough to warrant it, or if a text desc is enough.

@abbra
Copy link
Contributor Author

abbra commented Oct 16, 2017

Thanks. I added a text comment in dsinstance.py and also fixed pylint errors (long lines)

@pvoborni
Copy link
Member

A Pagure ticket was cloned for it: https://pagure.io/freeipa/issue/7211

@abbra abbra force-pushed the fixes-time-skew branch 2 times, most recently from d763c30 to f56f67a Compare October 16, 2017 13:25
@rcritten rcritten self-assigned this Oct 16, 2017
@rcritten
Copy link
Contributor

Force sync will fail because it does an add operation so there can be two values on a single-valued attribute.

I believe you can replace the add with replace in replica-ignore-time-skew.ldif

I'm a little unsure about when the value should be set back to off. You are doing so a little later than I would have expected because replication should be completed immediately after step 28 and you aren't resetting it until step 42.

@abbra
Copy link
Contributor Author

abbra commented Oct 16, 2017

Thanks, I refactored it a bit to reuse replica-prevent-time-skew.ldif for both cases. By default, it prevents time skew, while in ipa-replica-manage both enabling and disabling the time skew ignore is done explicitly via an optional argument.

I moved the second call in the initial case right after the replication. This should address your comments.

@rcritten
Copy link
Contributor

My force-sync test was failing with a no such entry because it was looking on the wrong host for the wrong agreement. This is because you use agreement.dn in the wait_for_repl_init. I got it working by adding this above that call:

agreement = repl.get_replication_agreement(thishost)

I also wonder what happens here in the case of winsync. Does it matter at all or should the ignore/prevent timesync be limited to the IPA_REPLICA type? I'm not sure and don't have an AD to test against this week. The 389-ds guys may be able to tell us if this would blow up winsync.

@abbra
Copy link
Contributor Author

abbra commented Oct 17, 2017

@rcritten I updated ipa-replica-manage part to only apply to IPA_REPLICA type and fixed agreement DN use.

@rcritten
Copy link
Contributor

Not to be too pedantic but is replica-ignore-time-skew.ldif needed since the setting is dynamic in replica_prevent_time_skew()? Could it be renamed to replica_manage_time_skew() or something? I'm not firm on this but it's an ldif used only once during replica install.

Otherwise looks good and tests were successful.

Initial replica creation can go with ignoring time skew checks.
We should, however, force time skew checks during normal operation.

Fixes https://pagure.io/freeipa/issue/7211
When performing force synchronization, implicitly ignore initial
time skew (if any) and restore it afterwards.

This also changes semantics of force-sync by waiting until the end of
the initial replication.

Fixes https://pagure.io/freeipa/issue/7211
@abbra abbra added the re-run Trigger a new run of PR-CI label Oct 17, 2017
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Oct 17, 2017
@rcritten rcritten added the ack Pull Request approved, can be merged label Oct 19, 2017
@abbra abbra added the pushed Pull Request has already been pushed label Oct 19, 2017
@abbra
Copy link
Contributor Author

abbra commented Oct 19, 2017

master:

  • 051786c ds: ignore time skew during initial replication step
  • 620f965 ipa-replica-manage: implicitly ignore initial time skew in force-sync

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants