Skip to content

Commit 4257781

Browse files
milan-zededamilan-zededa
committed
Compare DHCP config based on its effect
In situations where customers use override.json, we can sometimes see that going from override to zedagent DPC may trigger unecessary restarts of dhcpcd instances, resulting in interfaces giving up on their existing IP addresses and immediately asking for new ones. This can for example break an ongoing download process. These restarts are unecessary in the sense that arguments of restarted dhcpcd instances might have not actually changed. This can be prevented by improving the Equal method for DHCP config, considering two configs as equivalent if their effect is the same (i.e. same dhcpcd args). Signed-off-by: Milan Lenco <milan@zededa.com>
1 parent a0b1e52 commit 4257781

File tree

2 files changed

+222
-55
lines changed

2 files changed

+222
-55
lines changed

‎pkg/pillar/dpcreconciler/genericitems/dhcpcd.go‎

Lines changed: 81 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111
"net"
1212
"os"
1313
"os/exec"
14-
"reflect"
1514
"strconv"
1615
"strings"
1716
"syscall"
@@ -26,6 +25,8 @@ import (
2625
const (
2726
dhcpcdStartTimeout = 3 * time.Second
2827
dhcpcdStopTimeout = 30 * time.Second
28+
29+
zeroIPv4Addr = "0.0.0.0"
2930
)
3031

3132
// Dhcpcd : DHCP client (https://wiki.archlinux.org/title/dhcpcd).
@@ -54,7 +55,20 @@ func (c Dhcpcd) Type() string {
5455
// Equal is a comparison method for two equally-named Dhcpcd instances.
5556
func (c Dhcpcd) Equal(other depgraph.Item) bool {
5657
c2 := other.(Dhcpcd)
57-
return reflect.DeepEqual(c.DhcpConfig, c2.DhcpConfig)
58+
// Consider two DHCP configs as equal if they result in the same set of arguments for dhcpcd.
59+
// This avoids unnecessary restarts of dhcpcd (when e.g. going from override to zedagent DPC).
60+
configurator := &DhcpcdConfigurator{}
61+
op1, args1 := configurator.dhcpcdArgs(c.DhcpConfig)
62+
op2, args2 := configurator.dhcpcdArgs(c2.DhcpConfig)
63+
if op1 != op2 || len(args1) != len(args2) {
64+
return false
65+
}
66+
for i := range args1 {
67+
if args1[i] != args2[i] {
68+
return false
69+
}
70+
}
71+
return true
5872
}
5973

6074
// External returns false.
@@ -94,31 +108,15 @@ func (c *DhcpcdConfigurator) Create(ctx context.Context, item depgraph.Item) err
94108
ifName := client.AdapterIfName
95109
config := client.DhcpConfig
96110

97-
// Prepare input arguments
98-
var op string
99-
var args []string
111+
// Validate input arguments
100112
switch config.Dhcp {
101113
case types.DT_NONE:
114+
// Nothing to do, return.
102115
done(nil)
103116
return
104117

105118
case types.DT_CLIENT:
106-
op = "--request"
107-
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "-b", "-t", "0"}
108-
switch config.Type {
109-
case types.NtIpv4Only:
110-
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "--ipv4only", "-b", "-t", "0"}
111-
case types.NtIpv6Only:
112-
args = []string{"-f", "/dhcpcd.conf", "--ipv6only", "-b", "-t", "0"}
113-
case types.NT_NOOP:
114-
case types.NT_IPV4:
115-
case types.NT_IPV6:
116-
case types.NtDualStack:
117-
default:
118-
}
119-
if config.Gateway != nil && config.Gateway.String() == "0.0.0.0" {
120-
args = append(args, "--nogateway")
121-
}
119+
// Nothing to validate.
122120

123121
case types.DT_STATIC:
124122
if config.AddrSubnet == "" {
@@ -138,40 +136,6 @@ func (c *DhcpcdConfigurator) Create(ctx context.Context, item depgraph.Item) err
138136
done(err)
139137
return
140138
}
141-
op = "--static"
142-
args = []string{fmt.Sprintf("ip_address=%s", config.AddrSubnet)}
143-
extras := []string{"-f", "/dhcpcd.conf", "-b", "-t", "0"}
144-
if config.Gateway == nil || config.Gateway.String() == "0.0.0.0" {
145-
extras = append(extras, "--nogateway")
146-
} else if config.Gateway.String() != "" {
147-
args = append(args, "--static",
148-
fmt.Sprintf("routers=%s", config.Gateway.String()))
149-
}
150-
var dnsServers []string
151-
for _, dns := range config.DnsServers {
152-
dnsServers = append(dnsServers, dns.String())
153-
}
154-
if len(dnsServers) > 0 {
155-
// dhcpcd uses a very odd space-separation for multiple DNS servers.
156-
// For manual invocation one must be very careful to not forget
157-
// to quote the argument so that the spaces don't make the shell
158-
// break up the list into multiple args.
159-
// Here we do not need quotes because we are passing the DNS server
160-
// list as a single entry of the 'args' slice for exec.Command().
161-
args = append(args, "--static",
162-
fmt.Sprintf("domain_name_servers=%s",
163-
strings.Join(dnsServers, " ")))
164-
}
165-
if config.DomainName != "" {
166-
args = append(args, "--static",
167-
fmt.Sprintf("domain_name=%s", config.DomainName))
168-
}
169-
if config.NtpServer != nil && !config.NtpServer.IsUnspecified() {
170-
args = append(args, "--static",
171-
fmt.Sprintf("ntp_servers=%s",
172-
config.NtpServer.String()))
173-
}
174-
args = append(args, extras...)
175139

176140
default:
177141
err := fmt.Errorf("unsupported DHCP type: %v", config.Dhcp)
@@ -180,6 +144,9 @@ func (c *DhcpcdConfigurator) Create(ctx context.Context, item depgraph.Item) err
180144
return
181145
}
182146

147+
// Prepare input arguments for dhcpcd.
148+
op, args := c.dhcpcdArgs(config)
149+
183150
// Start DHCP client.
184151
if c.dhcpcdExists(client.AdapterIfName) {
185152
err := fmt.Errorf("dhcpcd for interface %s is already running", ifName)
@@ -297,6 +264,65 @@ func (c *DhcpcdConfigurator) NeedsRecreate(oldItem, newItem depgraph.Item) (recr
297264
return true
298265
}
299266

267+
func (c *DhcpcdConfigurator) dhcpcdArgs(config types.DhcpConfig) (op string, args []string) {
268+
switch config.Dhcp {
269+
case types.DT_CLIENT:
270+
op = "--request"
271+
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "-b", "-t", "0"}
272+
switch config.Type {
273+
case types.NtIpv4Only:
274+
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "--ipv4only", "-b", "-t", "0"}
275+
case types.NtIpv6Only:
276+
args = []string{"-f", "/dhcpcd.conf", "--ipv6only", "-b", "-t", "0"}
277+
case types.NT_NOOP:
278+
case types.NT_IPV4:
279+
case types.NT_IPV6:
280+
case types.NtDualStack:
281+
default:
282+
}
283+
if config.Gateway != nil && config.Gateway.String() == zeroIPv4Addr {
284+
args = append(args, "--nogateway")
285+
}
286+
287+
case types.DT_STATIC:
288+
op = "--static"
289+
args = []string{fmt.Sprintf("ip_address=%s", config.AddrSubnet)}
290+
extras := []string{"-f", "/dhcpcd.conf", "-b", "-t", "0"}
291+
if config.Gateway == nil || config.Gateway.String() == zeroIPv4Addr {
292+
extras = append(extras, "--nogateway")
293+
} else if config.Gateway.String() != "" {
294+
args = append(args, "--static",
295+
fmt.Sprintf("routers=%s", config.Gateway.String()))
296+
}
297+
var dnsServers []string
298+
for _, dns := range config.DnsServers {
299+
dnsServers = append(dnsServers, dns.String())
300+
}
301+
if len(dnsServers) > 0 {
302+
// dhcpcd uses a very odd space-separation for multiple DNS servers.
303+
// For manual invocation one must be very careful to not forget
304+
// to quote the argument so that the spaces don't make the shell
305+
// break up the list into multiple args.
306+
// Here we do not need quotes because we are passing the DNS server
307+
// list as a single entry of the 'args' slice for exec.Command().
308+
args = append(args, "--static",
309+
fmt.Sprintf("domain_name_servers=%s",
310+
strings.Join(dnsServers, " ")))
311+
}
312+
if config.DomainName != "" {
313+
args = append(args, "--static",
314+
fmt.Sprintf("domain_name=%s", config.DomainName))
315+
}
316+
if config.NtpServer != nil && !config.NtpServer.IsUnspecified() {
317+
args = append(args, "--static",
318+
fmt.Sprintf("ntp_servers=%s",
319+
config.NtpServer.String()))
320+
}
321+
args = append(args, extras...)
322+
}
323+
return
324+
}
325+
300326
func (c *DhcpcdConfigurator) dhcpcdCmd(op string, extras []string,
301327
ifName string, background bool) error {
302328
name := "/sbin/dhcpcd"

‎pkg/pillar/dpcreconciler/genericitems/dhcpcd_test.go‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
package genericitems_test
2+
3+
import (
4+
"net"
5+
"testing"
6+
7+
configitems "github.com/lf-edge/eve/pkg/pillar/dpcreconciler/genericitems"
8+
"github.com/lf-edge/eve/pkg/pillar/types"
9+
)
10+
11+
func TestDhcpcdEqual(t *testing.T) {
12+
type test struct {
13+
name string
14+
item1 configitems.Dhcpcd
15+
item2 configitems.Dhcpcd
16+
expEqual bool
17+
}
18+
var tests = []test{
19+
{
20+
name: "DHCP client for IPv4 only",
21+
item1: configitems.Dhcpcd{
22+
DhcpConfig: types.DhcpConfig{
23+
Dhcp: types.DT_CLIENT,
24+
AddrSubnet: "192.168.1.44/24", // irrelevant
25+
Gateway: net.ParseIP("192.168.1.1"), // irrelevant
26+
DomainName: "mydomain", // irrelevant
27+
NtpServer: net.ParseIP("192.168.1.1"), // irrelevant
28+
Type: types.NtIpv4Only, // must match
29+
},
30+
},
31+
item2: configitems.Dhcpcd{
32+
DhcpConfig: types.DhcpConfig{
33+
Dhcp: types.DT_CLIENT,
34+
Type: types.NtIpv4Only, // must match
35+
},
36+
},
37+
expEqual: true,
38+
},
39+
{
40+
name: "DHCP client with effectively equivalent IP types",
41+
item1: configitems.Dhcpcd{
42+
DhcpConfig: types.DhcpConfig{
43+
Dhcp: types.DT_CLIENT,
44+
Type: types.NT_NOOP, // sometimes we get this in override.json
45+
},
46+
},
47+
item2: configitems.Dhcpcd{
48+
DhcpConfig: types.DhcpConfig{
49+
Dhcp: types.DT_CLIENT,
50+
Type: types.NT_IPV4, // effectively equivalent to NT_NOOP
51+
},
52+
},
53+
expEqual: true,
54+
},
55+
{
56+
name: "DHCP client with effectively different IP types",
57+
item1: configitems.Dhcpcd{
58+
DhcpConfig: types.DhcpConfig{
59+
Dhcp: types.DT_CLIENT,
60+
Type: types.NT_IPV4,
61+
},
62+
},
63+
item2: configitems.Dhcpcd{
64+
DhcpConfig: types.DhcpConfig{
65+
Dhcp: types.DT_CLIENT,
66+
Type: types.NtIpv4Only, // differs in --ipv4only arg
67+
},
68+
},
69+
expEqual: false,
70+
},
71+
{
72+
name: "DHCP client with disabled default route",
73+
item1: configitems.Dhcpcd{
74+
DhcpConfig: types.DhcpConfig{
75+
Dhcp: types.DT_CLIENT,
76+
Gateway: net.ParseIP("0.0.0.0"), // default route is disabled
77+
78+
},
79+
},
80+
item2: configitems.Dhcpcd{
81+
DhcpConfig: types.DhcpConfig{
82+
Dhcp: types.DT_CLIENT,
83+
// default route is enabled
84+
},
85+
},
86+
expEqual: false,
87+
},
88+
{
89+
name: "equivalent static IP config",
90+
item1: configitems.Dhcpcd{
91+
DhcpConfig: types.DhcpConfig{
92+
Dhcp: types.DT_STATIC,
93+
AddrSubnet: "192.168.1.44/24",
94+
DomainName: "mydomain",
95+
NtpServer: net.ParseIP("192.168.1.1"),
96+
DnsServers: []net.IP{net.ParseIP("8.8.8.8")},
97+
Type: types.NtIpv4Only, // irrelevant
98+
},
99+
},
100+
item2: configitems.Dhcpcd{
101+
DhcpConfig: types.DhcpConfig{
102+
Dhcp: types.DT_STATIC,
103+
AddrSubnet: "192.168.1.44/24",
104+
DomainName: "mydomain",
105+
NtpServer: net.ParseIP("192.168.1.1"),
106+
DnsServers: []net.IP{net.ParseIP("8.8.8.8")},
107+
Type: types.NT_IPV4, // irrelevant
108+
},
109+
},
110+
expEqual: true,
111+
},
112+
{
113+
name: "different statically configured DNS servers",
114+
item1: configitems.Dhcpcd{
115+
DhcpConfig: types.DhcpConfig{
116+
Dhcp: types.DT_STATIC,
117+
AddrSubnet: "192.168.1.44/24",
118+
DomainName: "mydomain",
119+
NtpServer: net.ParseIP("192.168.1.1"),
120+
DnsServers: []net.IP{net.ParseIP("8.8.8.8")}, // does not match
121+
},
122+
},
123+
item2: configitems.Dhcpcd{
124+
DhcpConfig: types.DhcpConfig{
125+
Dhcp: types.DT_STATIC,
126+
AddrSubnet: "192.168.1.44/24",
127+
DomainName: "mydomain",
128+
NtpServer: net.ParseIP("192.168.1.1"),
129+
DnsServers: []net.IP{net.ParseIP("1.1.1.1")}, // does not match
130+
},
131+
},
132+
expEqual: false,
133+
},
134+
}
135+
for _, test := range tests {
136+
if test.item1.Equal(test.item2) != test.expEqual {
137+
t.Errorf("TEST CASE \"%s\" FAILED - Equal() returned: %t, expected: %t",
138+
test.name, test.item1.Equal(test.item2), test.expEqual)
139+
}
140+
}
141+
}

0 commit comments

Comments
 (0)