https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
On (18/06/13 13:51), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
I can confirm, tah patches fix ticket 1947 (crash).
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
I have just a question about code convention. I thought, that name of function parameter starting with "_" means output variable, but in function collapse_srv_lookup it is used as a in/out variable.
If some function is called with NULL in place of output parameter, it means that you do not want store output to this parameter. But function collapse_srv_lookup could not be called with NULL
static struct fo_server * -collapse_srv_lookup(struct fo_server *server) +collapse_srv_lookup(struct fo_server **_server) {
- struct fo_server *tmp, *meta;
- server = *_server;
^^^^^^^ This is reason, why function could not be called with NULL.
Do we have any code conventions for in/out variable?
LS
On Wed, Jun 19, 2013 at 10:51:32AM +0200, Lukas Slebodnik wrote:
On (18/06/13 13:51), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
I can confirm, tah patches fix ticket 1947 (crash).
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
I have just a question about code convention. I thought, that name of function parameter starting with "_" means output variable, but in function collapse_srv_lookup it is used as a in/out variable.
If some function is called with NULL in place of output parameter, it means that you do not want store output to this parameter. But function collapse_srv_lookup could not be called with NULL
static struct fo_server * -collapse_srv_lookup(struct fo_server *server) +collapse_srv_lookup(struct fo_server **_server) {
- struct fo_server *tmp, *meta;
- server = *_server;
^^^^^^^ This is reason, why function could not be called with NULL.
Do we have any code conventions for in/out variable?
I don't think we do. Does the logic of collapse_srv_lookup mandate that the input and output are always the same? If not, can we simply add an input parameter that can't be NULL?
On 06/19/2013 01:43 PM, Jakub Hrozek wrote:
On Wed, Jun 19, 2013 at 10:51:32AM +0200, Lukas Slebodnik wrote:
On (18/06/13 13:51), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
I can confirm, tah patches fix ticket 1947 (crash).
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
I have just a question about code convention. I thought, that name of function parameter starting with "_" means output variable, but in function collapse_srv_lookup it is used as a in/out variable.
If some function is called with NULL in place of output parameter, it means that you do not want store output to this parameter. But function collapse_srv_lookup could not be called with NULL
static struct fo_server * -collapse_srv_lookup(struct fo_server *server) +collapse_srv_lookup(struct fo_server **_server) {
- struct fo_server *tmp, *meta;
- server = *_server;
^^^^^^^ This is reason, why function could not be called with NULL.
Do we have any code conventions for in/out variable?
I don't think we do. Does the logic of collapse_srv_lookup mandate that the input and output are always the same? If not, can we simply add an input parameter that can't be NULL?
It's not real in/out parameter. The thing is that collapse_srv_lookup(server) frees the server, so I wanted to make sure that it is set to NULL.
Old call: meta = collapse_srv_lookup(server);
Now: meta = collapse_srv_lookup(&server); server is set to NULL
On (19/06/13 13:59), Pavel Březina wrote:
On 06/19/2013 01:43 PM, Jakub Hrozek wrote:
On Wed, Jun 19, 2013 at 10:51:32AM +0200, Lukas Slebodnik wrote:
On (18/06/13 13:51), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
I can confirm, tah patches fix ticket 1947 (crash).
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
I have just a question about code convention. I thought, that name of function parameter starting with "_" means output variable, but in function collapse_srv_lookup it is used as a in/out variable.
If some function is called with NULL in place of output parameter, it means that you do not want store output to this parameter. But function collapse_srv_lookup could not be called with NULL
static struct fo_server * -collapse_srv_lookup(struct fo_server *server) +collapse_srv_lookup(struct fo_server **_server) {
- struct fo_server *tmp, *meta;
- server = *_server;
^^^^^^^ This is reason, why function could not be called with NULL.
Do we have any code conventions for in/out variable?
I don't think we do. Does the logic of collapse_srv_lookup mandate that the input and output are always the same? If not, can we simply add an input parameter that can't be NULL?
It's not real in/out parameter. The thing is that collapse_srv_lookup(server) frees the server, so I wanted to make sure that it is set to NULL.
Old call: meta = collapse_srv_lookup(server);
Now: meta = collapse_srv_lookup(&server); server is set to NULL
It is in/out parameter, because value of server is(may be) changed after function call. (it is not important, that value will be NULL)
LS
On Wed, Jun 19, 2013 at 02:20:35PM +0200, Lukas Slebodnik wrote:
On (19/06/13 13:59), Pavel Březina wrote:
On 06/19/2013 01:43 PM, Jakub Hrozek wrote:
On Wed, Jun 19, 2013 at 10:51:32AM +0200, Lukas Slebodnik wrote:
On (18/06/13 13:51), Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
I can confirm, tah patches fix ticket 1947 (crash).
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
I have just a question about code convention. I thought, that name of function parameter starting with "_" means output variable, but in function collapse_srv_lookup it is used as a in/out variable.
If some function is called with NULL in place of output parameter, it means that you do not want store output to this parameter. But function collapse_srv_lookup could not be called with NULL
static struct fo_server * -collapse_srv_lookup(struct fo_server *server) +collapse_srv_lookup(struct fo_server **_server) {
- struct fo_server *tmp, *meta;
- server = *_server;
^^^^^^^ This is reason, why function could not be called with NULL.
Do we have any code conventions for in/out variable?
I don't think we do. Does the logic of collapse_srv_lookup mandate that the input and output are always the same? If not, can we simply add an input parameter that can't be NULL?
It's not real in/out parameter. The thing is that collapse_srv_lookup(server) frees the server, so I wanted to make sure that it is set to NULL.
Old call: meta = collapse_srv_lookup(server);
Now: meta = collapse_srv_lookup(&server); server is set to NULL
It is in/out parameter, because value of server is(may be) changed after function call. (it is not important, that value will be NULL)
Sorry, that's details. I think that it's important that the function synopsis makes it clear that the parameter can be modified.
On Tue, Jun 18, 2013 at 01:51:15PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
One question:
case SRV_NEUTRAL: /* Request SRV lookup */
if (server != NULL && server != state->meta) {
/* A server created by expansion of meta server was marked as
* neutral. We have to collapse the servers and issue new
* SRV resolution. */
state->meta = collapse_srv_lookup(&server);
}
Wouldn't a more systematic solution be to collapse the lookup from functions that are setting the status? Or were you afraid of race conditions where the server might still be in use by another request?
On 06/19/2013 09:22 PM, Jakub Hrozek wrote:
On Tue, Jun 18, 2013 at 01:51:15PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
One question:
case SRV_NEUTRAL: /* Request SRV lookup */
if (server != NULL && server != state->meta) {
/* A server created by expansion of meta server was marked as
* neutral. We have to collapse the servers and issue new
* SRV resolution. */
state->meta = collapse_srv_lookup(&server);
}
Wouldn't a more systematic solution be to collapse the lookup from functions that are setting the status? Or were you afraid of race conditions where the server might still be in use by another request?
This is currently done only in fo_reset_services() which iterates over services and servers and sets them to neutral. I think it would be safe to do the collapse there, I'm not entirely sure though.
However, since resolve_srv_send() allows non meta server as input parameter, it should not expect it to have correct (!SRV_NEUTRAL) state. It is safer this way.
On Thu, Jun 20, 2013 at 10:27:13AM +0200, Pavel Březina wrote:
On 06/19/2013 09:22 PM, Jakub Hrozek wrote:
On Tue, Jun 18, 2013 at 01:51:15PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
One question:
case SRV_NEUTRAL: /* Request SRV lookup */
if (server != NULL && server != state->meta) {
/* A server created by expansion of meta server was marked as
* neutral. We have to collapse the servers and issue new
* SRV resolution. */
state->meta = collapse_srv_lookup(&server);
}
Wouldn't a more systematic solution be to collapse the lookup from functions that are setting the status? Or were you afraid of race conditions where the server might still be in use by another request?
This is currently done only in fo_reset_services() which iterates over services and servers and sets them to neutral. I think it would be safe to do the collapse there, I'm not entirely sure though.
However, since resolve_srv_send() allows non meta server as input parameter, it should not expect it to have correct (!SRV_NEUTRAL) state. It is safer this way.
That's true.
Ack.
On Thu, Jun 20, 2013 at 01:46:34PM +0200, Jakub Hrozek wrote:
On Thu, Jun 20, 2013 at 10:27:13AM +0200, Pavel Březina wrote:
On 06/19/2013 09:22 PM, Jakub Hrozek wrote:
On Tue, Jun 18, 2013 at 01:51:15PM +0200, Pavel Březina wrote:
https://fedorahosted.org/sssd/ticket/1947
A fast explanation how _srv_ expansion works. _srv_ is inserted into server list as so called meta server. Let us consider following configuration:
*Setup* ipa_server = _srv_, ipa.pb server list will contain: meta -> ipa.pb
*Expansion* meta -> ipa.pb:389 -> ipa.pb ; remove meta ipa.pb:389 -> ipa.pb ; meta
*Collapse* remove ipa.pb:389 ; insert meta meta -> ipa.pb
The main problem is that expanded SRV servers are marked as NEUTRAL during online check, but they don't collapse back into a meta server.
This will trigger another SRV expansion, leaving the old server in the list and trying to add the servers again. This is present in both master and 1.9 (and probably older versions), although the result is slightly different.
In master, we don't insert a server into server list if it is already present. Because state->meta is orphaned from the previous SRV expansion, state->meta->next is NULL and SSSD crashes later.
In 1.9, we simply insert duplicate servers. Those servers are inserted after orphaned state->meta, state->meta is orphaned again, leaving those servers globally unreachable. However, it seemingly does not affect the fail over. You just run into d25e7c65.
Here are four patches for master, and two patches for 1.9.
One question:
case SRV_NEUTRAL: /* Request SRV lookup */
if (server != NULL && server != state->meta) {
/* A server created by expansion of meta server was marked as
* neutral. We have to collapse the servers and issue new
* SRV resolution. */
state->meta = collapse_srv_lookup(&server);
}
Wouldn't a more systematic solution be to collapse the lookup from functions that are setting the status? Or were you afraid of race conditions where the server might still be in use by another request?
This is currently done only in fo_reset_services() which iterates over services and servers and sets them to neutral. I think it would be safe to do the collapse there, I'm not entirely sure though.
However, since resolve_srv_send() allows non meta server as input parameter, it should not expect it to have correct (!SRV_NEUTRAL) state. It is safer this way.
That's true.
Ack.
I tested both positive and negative resolution and SRV recods collapsing. The crash itself was tested by lukas.
Pushed to master and sssd-1-9 as appropriate.
sssd-devel@lists.fedorahosted.org