[SSSD] [PATCH] Use meta server if there isn't another server after meta.

Pavel Březina pbrezina at redhat.com
Tue Jun 18 11:52:33 UTC 2013


On 06/10/2013 02:22 PM, Lukas Slebodnik wrote:
> On (10/06/13 10:54), Pavel Březina wrote:
>> On 06/10/2013 10:32 AM, Pavel Březina wrote:
>>> On 06/03/2013 10:28 PM, Jakub Hrozek wrote:
>>>> On Mon, Jun 03, 2013 at 03:54:34PM +0200, Lukas Slebodnik wrote:
>>>>> On (03/06/13 12:50), Lukas Slebodnik wrote:
>>>>>> ehlo
>>>>>>
>>>>>> I solved ticket #1929 and then I immediately hit another one #1947
>>>>>> (BZ967004)
>>>>>> It seems that reporter from second ticket was lucky and did not hit
>>>>>> the first
>>>>>> issue.
>>>>>>
>>>>>> Use meta server if there isn't another server after meta.
>>>>>> The same behaviour is in "done" section, if return code is not OK
>>>>>>
>>>>>> https://fedorahosted.org/sssd/ticket/1947
>>>>>>
>>>>>> Patch is attached.
>>>>>>
>>>>>> LS
>>>>>
>>>>> >From f885ea412076843b3e6bf97be51a2e7ae8a0240a Mon Sep 17 00:00:00 2001
>>>>>> From: Lukas Slebodnik <lslebodn at redhat.com>
>>>>>> Date: Fri, 31 May 2013 14:18:17 +0200
>>>>>> Subject: [PATCH] Use meta server if there isn't another server after
>>>>>> meta.
>>>>>>
>>>>>> The same behaviour is in "done" section, if return code is not OK
>>>>>>
>>>>>> https://fedorahosted.org/sssd/ticket/1947
>>>>>> ---
>>>>>> src/providers/fail_over.c | 2 +-
>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
>>>>>> index
>>>>>> 12b6c37828b7da0e68579bbb94668c21574974f1..6fa8b19c6e14d778d5f438b1578f3a22876b1625
>>>>>> 100644
>>>>>> --- a/src/providers/fail_over.c
>>>>>> +++ b/src/providers/fail_over.c
>>>>>> @@ -1285,7 +1285,7 @@ resolve_srv_done(struct tevent_req *subreq)
>>>>>>              }
>>>>>>          }
>>>>>>
>>>>>> -        state->out = state->meta->next;
>>>>>> +        state->out = (state->meta->next) ? state->meta->next :
>>>>>> state->meta;
>>>>>              ^^^^^^^^
>>>>> This code is little bit different in sssd-1-9 branch
>>>>> Somethink like this
>>>>>       if (srv_list) {
>>>>>          <snip>
>>>>>          state->out = srv_list
>>>>>
>>>>>          tevent_req_done(req);
>>>>>          return;
>>>>>      } else {
>>>>>          ret = EIO;
>>>>>          goto fail;
>>>>>      }
>>>>>
>>>>> Should We return EIO? Or my solution is good enough.
>>>>>
>>>>> LS
>>>>
>>>> Sorry, I don't think this is the right fix.
>>>>
>>>> The simplified synopsis of the resolve_srv request is:
>>>>   resolve_srv_send(server_in);
>>>>   resolve_srv_recv(server_out);
>>>>
>>>> server_in is a SRV meta-server that we want to resolve, server_out is
>>>> the first server in the list we have resolved.
>>>>
>>>> Currently, as you noted, server_out is returned even when SRV resolution
>>>> fails, but I don't think that's needed, if the SRV resolution fails, the
>>>> meta-server is marked as bad inside the request. Also, the failover code
>>>> uses the returned server unconditionally.
>>
>> If the resolution fails, server_out is the original meta server with
>> error flag. Failover will then try to resolve next server.
>>
>>>> So one part of the fix would be to make sure the resolve_srv_* request
>>>> always acts on the meta server and only return a fo_server on success.
>>>> The server that we return should be the "_last_server" parameter of
>>>> fo_add_server_list().
>>
>> No. meta->next should be returned on success. last_server is only for
>> ordering purpose. meta->next is the next server to which sssd should
>> try to connect.
>>
>>>> The second part of the bug comes from the fact, that, according to the
>>>> logs, the same server the query resolved was already present in the
>>>> list. Here is the function fo_add_server_list():
>>
>> Patch for this is attached.
>>
>>>>
>>>> 740     for (i = 0; i < num_servers; i++) {
>>>> 741         server = create_fo_server(service, servers[i].host,
>>>> servers[i].port,
>>>> 742                                   user_data, primary);
>>>> 743         if (server == NULL) {
>>>> 744             talloc_free(srv_list);
>>>> 745             return ENOMEM;
>>>> 746         }
>>>> 747
>>>> 748         server->srv_data = srv_data;
>>>> 749
>>>> 750         ret = fo_add_server_to_list(&srv_list, service->server_list,
>>>> 751                                     server, service->name);
>>>> 752         if (ret != EOK) {
>>>>
>>>> Because the server is already in the list, fo_add_server_to_list()
>>>> returns
>>>> EEXIST here.
>>>>
>>>> 753             talloc_free(server);
>>>>
>>>> The pointer to "server" is freed.
>>>>
>>>> 754         }
>>>>
>>>> The loop breaks because num_servers == 1
>>>>
>>>> 755     }
>>>> 756
>>>> 757     if (srv_list != NULL) {
>>>> 758         DLIST_ADD_LIST_AFTER(service->server_list, after_server,
>>>> 759                              srv_list, struct fo_server *);
>>>> 760     }
>>>> 761
>>>> 762     if (_last_server != NULL) {
>>>> 763         *_last_server = server;
>>>>
>>>> Now server is assigned to _last_server, but points to freed memory..
>>>>
>>>> 764     }
>>>>
>>>> I think we should instead assign srv_list to _last_server. I'm not quite
>>>> sure why we used the tail of the list, maybe Pavel will remember? But I
>>>> think we should return the head of what we resolved to the caller.
>>>
>>> Hi,
>>> the tail is used to keep the order of the servers.
>>>
>>> metasrv -> srv1_1 (head) -> srv1_2 (last server) -> srv2_1 -> srv2_2
>>>
>>> If you return head (ie srv_list) you get:
>>> metasrv -> srv1_1 (last_server) -> srv2_1 -> srv2_2 -> srv1_2
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>
> NACK
> sssd_be crashed with your patch.
>
> state->meta->next is still NULL in function resolve_srv_done.
>
> LS

This thread got superseded by [PATCHES] fix SRV expansion.





More information about the sssd-devel mailing list