Hi Willy,
Willy Tarreau <w...@1wt.eu> writes:
> Hi Krisztián,
>
> On Tue, Sep 24, 2019 at 12:18:51PM +0000, Krisztián Kovács (kkovacs) wrote:
>> When doing a soft shutdown, we won't be making new connections anymore so
>> there's no point in keeping the namespace file descriptors open anymore.
>>
>
> This does make sense. However I'm having two very minor style
> comments:
Thanks for your comments. Hopefully both are fixed. I've attached the
updated patch. (Is attaching the proper way to follow up with fixes?)
From 9aba76fd082beb12cfdbb0a7dd699325753e9dd7 Mon Sep 17 00:00:00 2001
From: Krisztian Kovacs <krisztian.kov...@oneidentity.com>
Date: Tue, 24 Sep 2019 14:12:13 +0200
Subject: [PATCH] BUG/MEDIUM: namespace: close open namespaces during soft
shutdown
When doing a soft shutdown, we won't be making new connections anymore so
there's no point in keeping the namespace file descriptors open anymore.
Keeping these open effectively makes it impossible to properly clean up
namespaces which are no longer used in the new configuration until all
previously opened connections are closed in the old worker process.
This change introduces a cleanup function that is called during soft shutdown
that closes all namespace file descriptors by iterating over the namespace
ebtree.
---
src/namespace.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/src/namespace.c b/src/namespace.c
index cfb81ba0f..f23da48f8 100644
--- a/src/namespace.c
+++ b/src/namespace.c
@@ -15,6 +15,7 @@
#include <common/hash.h>
#include <common/errors.h>
#include <proto/log.h>
+#include <proto/signal.h>
#include <types/global.h>
/* Opens the namespace <ns_name> and returns the FD or -1 in case of error
@@ -39,6 +40,24 @@ static int init_default_namespace()
static struct eb_root namespace_tree_root = EB_ROOT;
+static void netns_sig_stop(struct sig_handler *sh)
+{
+ struct ebpt_node *node, *next;
+ struct netns_entry *entry;
+
+ /* close namespace file descriptors and remove registered namespaces from the
+ * tree when stopping */
+ node = ebpt_first(&namespace_tree_root);
+ while (node) {
+ next = ebpt_next(node);
+ ebpt_delete(node);
+ entry = container_of(node, struct netns_entry, node);
+ free(entry->node.key);
+ close(entry->fd);
+ node = next;
+ }
+}
+
int netns_init(void)
{
int err_code = 0;
@@ -55,6 +74,8 @@ int netns_init(void)
}
}
+ signal_register_fct(0, netns_sig_stop, 0);
+
return err_code;
}
--
2.23.0
>
>> /* Opens the namespace <ns_name> and returns the FD or -1 in case of error
>> @@ -39,6 +40,24 @@ static int init_default_namespace()
>>
>> static struct eb_root namespace_tree_root = EB_ROOT;
>>
>> +static void netns_sig_stop(struct sig_handler *sh) {
> ^
> This brace should be on the left at the begginning of next line here:
> {
>> + struct ebpt_node *node, *next;
>> + struct netns_entry *entry;
>
> (...)
>> +
>> + /* close namespace file descriptors and remove registered namespaces
>> from the
>> + * tree when stopping */
>> + node = ebpt_first(&namespace_tree_root);
>> + while (node) {
>> + next = ebpt_next(node);
>> + ebpt_delete(node);
>> + entry = container_of(node, struct netns_entry, node);
>> + ha_warning("Closing namespace %s.\n", (char *)entry->node.key);
>
> Please do not emit warnings here. Warnings are exclusively to warn users
> about things they *can* fix. Here there is nothing they can do about this
> one and it ends up in users sending them to /dev/null and not seeing the
> important ones.
>
>> @@ -55,6 +74,8 @@ int netns_init(void)
>> }
>> }
>>
>> + signal_register_fct(0, netns_sig_stop, 0);
>> +
>> return err_code;
>
> Warning indent issue above, you seem to have used two spaces instead
> of the tab used in other lines around.
>
> And that's all, as you see it was really minor.
>
> Thanks,
> Willy
