Merge ~alfonsosanchezbeato/snappy-hwe-snaps/+git/wifi-ap:fix-boot-race into ~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap:master

Proposed by Alfonso Sanchez-Beato on 2018-03-26
Status: Merged
Approved by: Konrad Zapałowicz on 2018-03-27
Approved revision: 51d1492515e8577e791531b6c3d7f0d077973f6d
Merged at revision: 119e76002456989af0902401e356ccae2e9025a6
Proposed branch: ~alfonsosanchezbeato/snappy-hwe-snaps/+git/wifi-ap:fix-boot-race
Merge into: ~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap:master
Diff against target: 191 lines (+82/-18)
5 files modified
cmd/service/service.go (+74/-16)
spread.yaml (+0/-1)
tests/lib/prepare-each.sh (+6/-1)
tests/main/conf-from-gadget/task.yaml (+1/-0)
tests/main/conf-wizard-disabled-from-gadget/task.yaml (+1/-0)
Reviewer Review Type Date Requested Status
System Enablement Bot continuous-integration 2018-03-26 Approve on 2018-03-27
Konrad Zapałowicz (community) code Approve on 2018-03-27
Review via email: mp+342117@code.launchpad.net

Description of the change

Re-start hostapd when NM is (re)started

This is needed so we set the wifi interface as "unmanaged" in
NetworkManager. We detect NM (re)starts by watching at the service
DBus name changes. Fixes LP: #1758342.

To post a comment you must log in.

PASSED: Successfully build documentation, rev: 0e0fb0fc9167e522b53b0d454376134385bbc548

Generated documentation is available at https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-docs/1058/

PASSED: Successfully build documentation, rev: fc0837ce8e770320218d02c9dcfa12251dbcfbef

Generated documentation is available at https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-docs/1059/

Konrad Zapałowicz (kzapalowicz) wrote :

LGTM so acked, two 'mater of taste' comments inline

review: Approve (code)

PASSED: Successfully build documentation, rev: 51d1492515e8577e791531b6c3d7f0d077973f6d

Generated documentation is available at https://jenkins.canonical.com/system-enablement/job/snappy-hwe-snaps-snap-docs/1060/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cmd/service/service.go b/cmd/service/service.go
2index c580f4a..ce92b7d 100644
3--- a/cmd/service/service.go
4+++ b/cmd/service/service.go
5@@ -26,6 +26,7 @@ import (
6 "path/filepath"
7 "time"
8
9+ "github.com/godbus/dbus"
10 "gopkg.in/tomb.v2"
11 )
12
13@@ -48,6 +49,8 @@ type service struct {
14 listener net.Listener
15 router *mux.Router
16 ap BackgroundProcess
17+ dbusConn *dbus.Conn
18+ quit chan int
19 }
20
21 func (c *serviceCommand) ServeHTTP(w http.ResponseWriter, r *http.Request) {
22@@ -100,26 +103,44 @@ func (s *service) setupAccesPoint() error {
23 }
24
25 func (s *service) Shutdown() {
26- log.Println("Shutting down ...")
27- s.listener.Close()
28- s.tomb.Kill(nil)
29- s.tomb.Wait()
30+ s.quit <- 0
31+}
32+
33+func (s *service) createNetworkManagerMonitorCh() (chan *dbus.Signal, error) {
34+ var err error
35+ s.dbusConn, err = dbus.SystemBus()
36+ if err != nil {
37+ return nil, fmt.Errorf("Failed to connect to system bus: %v", err)
38+ }
39+
40+ err = s.dbusConn.BusObject().Call("org.freedesktop.DBus.AddMatch", 0,
41+ "type='signal',path='/org/freedesktop/DBus',"+
42+ "interface='org.freedesktop.DBus',"+
43+ "sender='org.freedesktop.DBus',member='NameOwnerChanged',"+
44+ "arg0='org.freedesktop.NetworkManager'").Store()
45+ if err != nil {
46+ return nil, fmt.Errorf("Error when calling AddMatch: %v", err)
47+ }
48+
49+ c := make(chan *dbus.Signal, 10)
50+ s.dbusConn.Signal(c)
51+
52+ return c, nil
53 }
54
55 func (s *service) Run() error {
56+ s.quit = make(chan int)
57 s.addRoutes()
58- if err := s.setupAccesPoint(); err != nil {
59- return err
60- }
61
62 var err error
63- if validTokens, err = loadValidTokens(filepath.Join(os.Getenv("SNAP"), "conf", "default-config")); err != nil {
64+ if validTokens, err = loadValidTokens(filepath.Join(os.Getenv("SNAP"),
65+ "conf", "default-config")); err != nil {
66 log.Println("Failed to read default configuration:", err)
67 }
68
69 // Create the socket directory and remove any stale socket
70 path := filepath.Join(os.Getenv("SNAP_DATA"), socketPathSuffix)
71- if _, err := os.Stat(filepath.Dir(path)); os.IsNotExist(err) {
72+ if _, err = os.Stat(filepath.Dir(path)); os.IsNotExist(err) {
73 os.Mkdir(filepath.Dir(path), 0755)
74 }
75 os.Remove(path)
76@@ -131,20 +152,57 @@ func (s *service) Run() error {
77 }
78
79 s.tomb.Go(func() error {
80- err := s.server.Serve(s.listener)
81+ err = s.server.Serve(s.listener)
82 if err != nil {
83- return fmt.Errorf("Failed to server HTTP: %s", err)
84+ return fmt.Errorf("Failed to start service: %s", err)
85 }
86- return nil
87+ return err
88 })
89
90- s.tomb.Wait()
91+ nmChangeCh, err := s.createNetworkManagerMonitorCh()
92+ if err != nil {
93+ return err
94+ }
95+ if err = s.setupAccesPoint(); err != nil {
96+ return err
97+ }
98
99- if s.ap.Running() {
100- s.ap.Stop()
101+ var errLp error
102+monitorOut:
103+ for {
104+ select {
105+ case sign := <-nmChangeCh:
106+ if len(sign.Body) != 3 {
107+ errLp = fmt.Errorf("Bad number of args in " +
108+ "NameOwnerChanged signal")
109+ break monitorOut
110+ }
111+ newOwner, ok := sign.Body[2].(string)
112+ if ok == false {
113+ errLp = fmt.Errorf("Bad argument type in " +
114+ "NameOwnerChanged signal")
115+ break monitorOut
116+ }
117+ if newOwner != "" {
118+ log.Println("NetworkManager stopped -> started")
119+ s.ap.Stop()
120+ if errLp = s.setupAccesPoint(); errLp != nil {
121+ break monitorOut
122+ }
123+ }
124+ case <-s.quit:
125+ log.Println("Shutting down...")
126+ break monitorOut
127+ }
128 }
129
130- return nil
131+ s.dbusConn.Close()
132+ s.listener.Close()
133+ s.tomb.Kill(nil)
134+ s.ap.Stop()
135+ s.tomb.Wait()
136+
137+ return errLp
138 }
139
140 type tcpKeepAliveListener struct {
141diff --git a/spread.yaml b/spread.yaml
142index 3a743c0..3cbdb3b 100644
143--- a/spread.yaml
144+++ b/spread.yaml
145@@ -43,7 +43,6 @@ prepare-each: |
146 # Cleanup logs so we can just dump what has happened in the debug-each
147 # step below after a test case ran.
148 journalctl --rotate
149- sleep .1
150 journalctl --vacuum-time=1ms
151 dmesg -c > /dev/null
152
153diff --git a/tests/lib/prepare-each.sh b/tests/lib/prepare-each.sh
154index 7c9426a..c321930 100644
155--- a/tests/lib/prepare-each.sh
156+++ b/tests/lib/prepare-each.sh
157@@ -1,4 +1,4 @@
158-#!/bin/sh
159+#!/bin/sh -x
160 . $TESTSLIB/utilities.sh
161
162 # Powercycle both interface to get them back into a sane state before
163@@ -14,3 +14,8 @@ snap remove wireless-tools
164 install_snap_under_test
165 # Give wifi-ap a bit time to settle down to avoid clashed
166 sleep 5
167+# hostapd fails due to a seccomp error in qemu. We need to re-start wifi-ap to
168+# have it back. This is probably a qemu/kvm bug, so we woraround the issue for
169+# the moment.
170+snap restart wifi-ap
171+sleep 10
172diff --git a/tests/main/conf-from-gadget/task.yaml b/tests/main/conf-from-gadget/task.yaml
173index e26d7bf..234515c 100644
174--- a/tests/main/conf-from-gadget/task.yaml
175+++ b/tests/main/conf-from-gadget/task.yaml
176@@ -1,4 +1,5 @@
177 summary: Verify that the default SSID can be read from a gadget snap
178+manual: true
179 description: |
180 We check that we get the SSID name from the defaults in the gadget
181 snap. To do this, we re-package the gadget snap in the store after
182diff --git a/tests/main/conf-wizard-disabled-from-gadget/task.yaml b/tests/main/conf-wizard-disabled-from-gadget/task.yaml
183index 84cbbd3..5bc2974 100644
184--- a/tests/main/conf-wizard-disabled-from-gadget/task.yaml
185+++ b/tests/main/conf-wizard-disabled-from-gadget/task.yaml
186@@ -1,4 +1,5 @@
187 summary: Verify the wizard can be disabled by default from a gadget snap
188+manual: true
189
190 prepare: |
191 . $TESTSLIB/snap-names.sh

Subscribers

People subscribed via source and target branches