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

[Bob Campbell]

I've placed our patches on top of 6 of Metze's, including the one
indicated, and placed those onto master. I now see the issue with
putting the ancestor collection in getncchanges_collect_objects(). The
tests pass when all the patches are on. The tree is at
http://git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/17-01-anc-patches.

Metze: Should "expand tests" and your test commit be merged? Another way
to do it would possibly be to move the tests into getnc_exop in a
separate patch from adding my new test cases.

Thanks,
Bob

On 12/01/17 22:05, Stefan Metzmacher wrote:
> Am 12.01.2017 um 09:55 schrieb Andrew Bartlett:
>> On Thu, 2017-01-12 at 09:44 +0100, Stefan Metzmacher wrote:
>>> Am 12.01.2017 um 09:23 schrieb Andrew Bartlett:
>>>> On Thu, 2016-12-15 at 12:04 +1300, Bob Campbell wrote:
>>>>> 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.
>>>> Metze,
>>>>
>>>> We have had a customer issue come up that looks to be related to
>>>> this
>>>> issue (incorrect GET_ANC handling).  Can you help us understand
>>>> what is
>>>> needed to make some progress on getting the work you and Bob have
>>>> done
>>>> into master?
>>>>
>>>> I agree that it is critical (for performance) that we not add the
>>>> ancestors within getncchanges_collect_objects(), which needs to be
>>>> absolutely as simple as possible, as it deals with the whole domain
>>>> during a full replication.
>>> I'd just take this patch
>>> https://git.samba.org/?p=metze/samba/wip.git;a=commitdiff;h=8d156d088
>>> 5e31ad6b80c1979efdfa7cfee075215
>>> which works for ours customers just fine and do further improvements
>>> on top of it.
>> Which SerNet samba.plus RPMs include this patch? (my customers are also
>> your customers ;-)
>>
>> I'll work with Bob tomorrow to understand better where his work and
>> yours intersect, so we can move towards merging something with tests.  
>>
>> I'll see if Bob can re-work his patches to be on top of your patch. 
>> Aside from where we add the ancestors, is there anything else major
>> outstanding I should be aware of?
> Starting with 4.5.3, it's not yet in 4.4 or something older (at least yet).
>
> metze
>