[PATCH] Fix server side DRSUAPI_DRS_GET_ANC handling (bug #12398)

[Bob Campbell]

Hi Metze,

Comments inline.

On 15/12/16 00:35, Stefan Metzmacher wrote:
> - can you take the full AbstractLink changes including the __eq__() change?
Done.
> - I'd really like to avoid adding the ancestors within
> getncchanges_collect_objects
>   as that's not how it's done in the documentation. The main problem
>   I have with this is site_res_cmp_anc_order, which is the main reason
>   why we need the r->out.ctr->ctr6.new_highwatermark.reserved_usn += 1;
>   hack, we should only have
> r->out.ctr->ctr6.new_highwatermark.tmp_highest_usn
>   incremented to a number that's part of the replicated changes
>   (without the ancestors) the added ancestors should not impact
>   the new_highwatermark.
>   I think it's also the reason why you see the different sorting
>   when two DNs aren't ancestors/children of each other.
While investigating this, we found a worrying related problem, and added
a test for it. When a DC replicates from another DC with
DRSUAPI_DRS_CRITICAL_ONLY, it seems that it still updates the
highwatermark's highest USN to be the highest USN that was replicated.
This means that if a non-critical object has a lower USN than this, it
may never be replicated. This happens both against Windows and Samba, as
shown in the test. We speculate that Windows may be ignoring the new
highwatermark on the client side (which we obviously don't use in our
tests) if DRSUAPI_DRS_CRITICAL_ONLY is set, as this seems to us to be
the most reasonable way to resolve the issue. Doing this would mean that
critical objects would be unnecessarily replicated in this case, however
it would also mean that we don't miss any non-critical objects. The
alternative as I see it is to add some list of non-replicated
non-critical objects which are below the highwatermark, which defeats
its purpose and would potentially slow replication greatly/add an O(n^2)
segment.

We also understand the issue you've pointed out here. It seems that the
functionality we've put in getncchanges_collect_objects is split into
two functions in Windows: GetReplScope (4.1.10.5.3) and
GetChangesInScope (4.1.10.5.5). In GetReplScope, they always add
ancestors of critical objects to the scope if DRSUAPI_DRS_CRITICAL_ONLY
is set (even if DRSUAPI_DRS_GET_ANC isn't set) - what documentation do
you mean when you say that it's not how it's done in the documentation?
We're going off of MS-DRSR. You are definitely correct in that the
non-critical ancestors shouldn't impact the new_highwatermark, and
indeed we possibly shouldn't change the new_highwatermark at all if
DRSUAPI_DRS_CRITICAL_ONLY is set, as described in the related problem.
> - In case we don't implement the GET_ANC handling for link targets
>   and/or the DRSUAPI_DRS_GET_TGT logic, we should at least
>   implement the changes with that logic in mind. Then we can
>   also stop adding all links at the end of the cycle.
We'll have a better look at this once we've resolved the previous issue.

Thanks for the input,
Bob