| Message ID | 1473770020-19533-3-git-send-email-dedeckeh@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | John Crispin |
| Headers | show |
On 2016-09-13 14:33, Hans Dedecker wrote: > Dropping hotplug event in case of interface reload results into hotplug scripts > not being being run for the interface and thus external actors not being informed > about the actual state of the interface. > > This is clearly visible if the interface auto parameter is set to disabled for > multiple interfaces resulting into no hotplug down event for all interfaces. > > Therefore don't flush the interface hotplug queue in case an interface reload > event is observed. I have some doubt about this. What use is the reload event to hotplug scripts? As far as I can tell there is no guarantee that the interface is in a well defined state when this event is emitted. For reloads that imply an up/down cycle, the regular events should suffice. For reloads that affect the state without making the interface go through up/down, we could maybe have a separate event. Am I missing something? - Felix
On Wed, Sep 14, 2016 at 1:15 PM, Felix Fietkau <nbd@nbd.name> wrote: > On 2016-09-13 14:33, Hans Dedecker wrote: >> Dropping hotplug event in case of interface reload results into hotplug scripts >> not being being run for the interface and thus external actors not being informed >> about the actual state of the interface. >> >> This is clearly visible if the interface auto parameter is set to disabled for >> multiple interfaces resulting into no hotplug down event for all interfaces. >> >> Therefore don't flush the interface hotplug queue in case an interface reload >> event is observed. > I have some doubt about this. What use is the reload event to hotplug > scripts? As far as I can tell there is no guarantee that the interface > is in a well defined state when this event is emitted. For reloads that > imply an up/down cycle, the regular events should suffice. For reloads > that affect the state without making the interface go through up/down, > we could maybe have a separate event. Am I missing something? Problem is related to the interface reload event flushing the pending event in the interface hotplug queue. Assume there are three interfaces configured A, B and C; for each of them we set the auto parameter to 0 followed by a network reload. This will bring the interfaces in reload config state; each interface will be brought down resulting into an IFEV_DOWN event. As a result for interface A a hotplug event with action ifdown will be launched; for interfaces B and C the hotplug event will be queued as hotplug event for interface A is running. Once interfaces B and C are marked as down in netifd the function interface_do_reload will launch the IFEV_RELOAD event resulting into drop of the queued hotplug event of both interface B and C in the function interface_event_cb (hotplug event for interface A is still running at that moment) Thus hotplug scripts have seen an ifdown event for interface A but not for interface B and C; meaning state driven hotplug actions for interface B and C are not performed. Therefore I removed the flushing of the hotplug queue based on the reload event which fixes the above described problem; did also tests in other scenarios where a down/up is triggered by reload config and did not observe any issues. Hans > > - Felix >
On 2016-09-14 14:44, Hans Dedecker wrote: > On Wed, Sep 14, 2016 at 1:15 PM, Felix Fietkau <nbd@nbd.name> wrote: >> On 2016-09-13 14:33, Hans Dedecker wrote: >>> Dropping hotplug event in case of interface reload results into hotplug scripts >>> not being being run for the interface and thus external actors not being informed >>> about the actual state of the interface. >>> >>> This is clearly visible if the interface auto parameter is set to disabled for >>> multiple interfaces resulting into no hotplug down event for all interfaces. >>> >>> Therefore don't flush the interface hotplug queue in case an interface reload >>> event is observed. >> I have some doubt about this. What use is the reload event to hotplug >> scripts? As far as I can tell there is no guarantee that the interface >> is in a well defined state when this event is emitted. For reloads that >> imply an up/down cycle, the regular events should suffice. For reloads >> that affect the state without making the interface go through up/down, >> we could maybe have a separate event. Am I missing something? > Problem is related to the interface reload event flushing the pending > event in the interface hotplug queue. > Assume there are three interfaces configured A, B and C; for each of > them we set the auto parameter to 0 followed by a network reload. > This will bring the interfaces in reload config state; each interface > will be brought down resulting into an IFEV_DOWN event. > As a result for interface A a hotplug event with action ifdown will be > launched; for interfaces B and C the hotplug event will be queued as > hotplug event for interface A is running. > Once interfaces B and C are marked as down in netifd the function > interface_do_reload will launch the IFEV_RELOAD event resulting into > drop of the queued hotplug event of both interface B and C in the > function interface_event_cb (hotplug event for interface A is still > running at that moment) > Thus hotplug scripts have seen an ifdown event for interface A but not > for interface B and C; meaning state driven hotplug actions for > interface B and C are not performed. > Therefore I removed the flushing of the hotplug queue based on the > reload event which fixes the above described problem; did also tests > in other scenarios where a down/up is triggered by reload config and > did not observe any issues. It seems that I misunderstood the code. Thanks for the explanation, it makes sense to me now. - Felix
diff --git a/interface-event.c b/interface-event.c index 93da22b..4976c2c 100644 --- a/interface-event.c +++ b/interface-event.c @@ -196,9 +196,10 @@ static void interface_event_cb(struct interface_user *dep, struct interface *ifa interface_queue_event(iface, ev); break; case IFEV_FREE: - case IFEV_RELOAD: interface_dequeue_event(iface); break; + default: + break; } }
Dropping hotplug event in case of interface reload results into hotplug scripts not being being run for the interface and thus external actors not being informed about the actual state of the interface. This is clearly visible if the interface auto parameter is set to disabled for multiple interfaces resulting into no hotplug down event for all interfaces. Therefore don't flush the interface hotplug queue in case an interface reload event is observed. Signed-off-by: Hans Dedecker <dedeckeh@gmail.com> --- interface-event.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)