The following pull request was submitted through Github.
It can be accessed and reviewed at: https://github.com/lxc/lxd/pull/6669
This e-mail was sent by the LXC bot, direct replies will not reach the author
unless they happen to be subscribed to this list.
=== Description (from pull-request) ===
Building on issue #5826 to use the new background proceess manager to refactor how dnsmasq and forkdns are processes are started or killed.
From e44db0e99534821cdfb3bddcc67a28efd12ca94d Mon Sep 17 00:00:00 2001
From: Rishabh Thakkar <rishabh.thak...@gmail.com>
Date: Fri, 3 Jan 2020 15:53:09 -0600
Subject: [PATCH] lxd: updated dnsmasq and forkdns to use new subprocess module
Signed-off-by: ribsthakkar <rishabh.thak...@gmail.com>
lxd: Cleanedup code refactor
Signed-off-by: Rishabh Thakkar <rishabh.thak...@gmail.com>
---
lxd/dnsmasq/dnsmasq.go | 79 ++++-------------------------
lxd/networks.go | 103 +++++++++++++++++++++++++-------------
lxd/networks_utils.go | 46 +++--------------
shared/subprocess/proc.go | 18 +++----
4 files changed, 95 insertions(+), 151 deletions(-)
diff --git a/lxd/dnsmasq/dnsmasq.go b/lxd/dnsmasq/dnsmasq.go
index 1c1f07d12f..19bcfb3db8 100644
--- a/lxd/dnsmasq/dnsmasq.go
+++ b/lxd/dnsmasq/dnsmasq.go
@@ -6,16 +6,13 @@ import (
"io/ioutil"
"net"
"os"
- "path/filepath"
- "strconv"
"strings"
"sync"
"github.com/lxc/lxd/lxd/project"
"github.com/lxc/lxd/shared"
+ "github.com/lxc/lxd/shared/subprocess"
"github.com/lxc/lxd/shared/version"
-
- "golang.org/x/sys/unix"
)
// DHCPAllocation represents an IP allocation from dnsmasq.
@@ -71,85 +68,31 @@ func RemoveStaticEntry(network string, projectName string,
instanceName string)
// Kill kills dnsmasq for a particular network (or optionally reloads it).
func Kill(name string, reload bool) error {
- // Check if we have a running dnsmasq at all
- pidPath := shared.VarPath("networks", name, "dnsmasq.pid")
- if !shared.PathExists(pidPath) {
- if reload {
- return fmt.Errorf("dnsmasq isn't running")
- }
-
- return nil
- }
-
- // Grab the PID
- content, err := ioutil.ReadFile(pidPath)
- if err != nil {
- return err
- }
- pid := strings.TrimSpace(string(content))
-
- // Check for empty string
- if pid == "" {
- os.Remove(pidPath)
-
- if reload {
- return fmt.Errorf("dnsmasq isn't running")
- }
-
- return nil
- }
-
- // Check if the process still exists
- if !shared.PathExists(fmt.Sprintf("/proc/%s", pid)) {
- os.Remove(pidPath)
+ pidPath := shared.VarPath("networks", name, "dnsmasq.pidfd")
- if reload {
- return fmt.Errorf("dnsmasq isn't running")
- }
-
- return nil
- }
-
- // Check if it's dnsmasq
- cmdPath, err := os.Readlink(fmt.Sprintf("/proc/%s/exe", pid))
- if err != nil {
- cmdPath = ""
- }
-
- // Deal with deleted paths
- cmdName := filepath.Base(strings.Split(cmdPath, " ")[0])
- if cmdName != "dnsmasq" {
- if reload {
- return fmt.Errorf("dnsmasq isn't running")
- }
-
- os.Remove(pidPath)
- return nil
- }
-
- // Parse the pid
- pidInt, err := strconv.Atoi(pid)
+ //Import saved subprocess details
+ p, err := subprocess.ImportProcess(pidPath)
if err != nil {
- return err
+ return fmt.Errorf("Could not read pid file: %s", err)
}
- // Actually kill the process
if reload {
- err = unix.Kill(pidInt, unix.SIGHUP)
+ err = p.Reload()
if err != nil {
- return err
+ return fmt.Errorf("Could not reload dnsmasq: %s", err)
}
return nil
}
- err = unix.Kill(pidInt, unix.SIGKILL)
+ err = p.Stop()
if err != nil {
- return err
+ if !strings.Contains(err.Error(), "Process is not running") {
+ return fmt.Errorf("Unable to kill dnsmasq: %s", err)
+ }
}
// Cleanup
- os.Remove(pidPath)
return nil
}
diff --git a/lxd/networks.go b/lxd/networks.go
index 8b2458870e..4ce104dfad 100644
--- a/lxd/networks.go
+++ b/lxd/networks.go
@@ -1365,9 +1365,9 @@ func (n *network) Setup(oldConfig map[string]string)
error {
}
}
- // Start building the dnsmasq command line
- dnsmasqCmd := []string{"dnsmasq", "--strict-order", "--bind-interfaces",
- fmt.Sprintf("--pid-file=%s", shared.VarPath("networks", n.name,
"dnsmasq.pid")),
+ // Start building process using subprocess package
+ command := "dnsmasq"
+ dnsmasqArgs := []string{"--strict-order", "--bind-interfaces",
"--except-interface=lo",
"--no-ping", // --no-ping is very important to prevent delays
to lease file updates.
fmt.Sprintf("--interface=%s", n.name)}
@@ -1380,7 +1380,7 @@ func (n *network) Setup(oldConfig map[string]string)
error {
// --dhcp-rapid-commit option is only supported on >2.79
minVer, _ := version.NewDottedVersion("2.79")
if dnsmasqVersion.Compare(minVer) > 0 {
- dnsmasqCmd = append(dnsmasqCmd, "--dhcp-rapid-commit")
+ dnsmasqArgs = append(dnsmasqArgs, "--dhcp-rapid-commit")
}
if !daemon.Debug {
@@ -1388,7 +1388,7 @@ func (n *network) Setup(oldConfig map[string]string)
error {
minVer, _ := version.NewDottedVersion("2.67")
if err == nil && dnsmasqVersion.Compare(minVer) > 0 {
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--quiet-dhcp", "--quiet-dhcp6", "--quiet-ra"}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--quiet-dhcp", "--quiet-dhcp6", "--quiet-ra"}...)
}
}
@@ -1401,14 +1401,14 @@ func (n *network) Setup(oldConfig map[string]string)
error {
}
// Update the dnsmasq config
- dnsmasqCmd = append(dnsmasqCmd,
fmt.Sprintf("--listen-address=%s", ip.String()))
+ dnsmasqArgs = append(dnsmasqArgs,
fmt.Sprintf("--listen-address=%s", ip.String()))
if n.config["ipv4.dhcp"] == "" ||
shared.IsTrue(n.config["ipv4.dhcp"]) {
- if !shared.StringInSlice("--dhcp-no-override",
dnsmasqCmd) {
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-no-override", "--dhcp-authoritative",
fmt.Sprintf("--dhcp-leasefile=%s", shared.VarPath("networks", n.name,
"dnsmasq.leases")), fmt.Sprintf("--dhcp-hostsfile=%s",
shared.VarPath("networks", n.name, "dnsmasq.hosts"))}...)
+ if !shared.StringInSlice("--dhcp-no-override",
dnsmasqArgs) {
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--dhcp-no-override", "--dhcp-authoritative",
fmt.Sprintf("--dhcp-leasefile=%s", shared.VarPath("networks", n.name,
"dnsmasq.leases")), fmt.Sprintf("--dhcp-hostsfile=%s",
shared.VarPath("networks", n.name, "dnsmasq.hosts"))}...)
}
if n.config["ipv4.dhcp.gateway"] != "" {
- dnsmasqCmd = append(dnsmasqCmd,
fmt.Sprintf("--dhcp-option=3,%s", n.config["ipv4.dhcp.gateway"]))
+ dnsmasqArgs = append(dnsmasqArgs,
fmt.Sprintf("--dhcp-option=3,%s", n.config["ipv4.dhcp.gateway"]))
}
expiry := "1h"
@@ -1419,10 +1419,10 @@ func (n *network) Setup(oldConfig map[string]string)
error {
if n.config["ipv4.dhcp.ranges"] != "" {
for _, dhcpRange := range
strings.Split(n.config["ipv4.dhcp.ranges"], ",") {
dhcpRange = strings.TrimSpace(dhcpRange)
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-range", fmt.Sprintf("%s,%s", strings.Replace(dhcpRange, "-",
",", -1), expiry)}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--dhcp-range", fmt.Sprintf("%s,%s", strings.Replace(dhcpRange, "-",
",", -1), expiry)}...)
}
} else {
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-range", fmt.Sprintf("%s,%s,%s", networkGetIP(subnet,
2).String(), networkGetIP(subnet, -2).String(), expiry)}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--dhcp-range", fmt.Sprintf("%s,%s,%s", networkGetIP(subnet,
2).String(), networkGetIP(subnet, -2).String(), expiry)}...)
}
}
@@ -1519,7 +1519,7 @@ func (n *network) Setup(oldConfig map[string]string)
error {
}
// Update the dnsmasq config
- dnsmasqCmd = append(dnsmasqCmd,
[]string{fmt.Sprintf("--listen-address=%s", ip.String()), "--enable-ra"}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{fmt.Sprintf("--listen-address=%s", ip.String()), "--enable-ra"}...)
if n.config["ipv6.dhcp"] == "" ||
shared.IsTrue(n.config["ipv6.dhcp"]) {
if n.config["ipv6.firewall"] == "" ||
shared.IsTrue(n.config["ipv6.firewall"]) {
// Setup basic iptables overrides for DHCP/DNS
@@ -1527,8 +1527,8 @@ func (n *network) Setup(oldConfig map[string]string)
error {
}
// Build DHCP configuration
- if !shared.StringInSlice("--dhcp-no-override",
dnsmasqCmd) {
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-no-override", "--dhcp-authoritative",
fmt.Sprintf("--dhcp-leasefile=%s", shared.VarPath("networks", n.name,
"dnsmasq.leases")), fmt.Sprintf("--dhcp-hostsfile=%s",
shared.VarPath("networks", n.name, "dnsmasq.hosts"))}...)
+ if !shared.StringInSlice("--dhcp-no-override",
dnsmasqArgs) {
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--dhcp-no-override", "--dhcp-authoritative",
fmt.Sprintf("--dhcp-leasefile=%s", shared.VarPath("networks", n.name,
"dnsmasq.leases")), fmt.Sprintf("--dhcp-hostsfile=%s",
shared.VarPath("networks", n.name, "dnsmasq.hosts"))}...)
}
expiry := "1h"
@@ -1541,16 +1541,16 @@ func (n *network) Setup(oldConfig map[string]string)
error {
if n.config["ipv6.dhcp.ranges"] != "" {
for _, dhcpRange := range
strings.Split(n.config["ipv6.dhcp.ranges"], ",") {
dhcpRange =
strings.TrimSpace(dhcpRange)
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-range", fmt.Sprintf("%s,%d,%s", strings.Replace(dhcpRange,
"-", ",", -1), subnetSize, expiry)}...)
+ dnsmasqArgs =
append(dnsmasqArgs, []string{"--dhcp-range", fmt.Sprintf("%s,%d,%s",
strings.Replace(dhcpRange, "-", ",", -1), subnetSize, expiry)}...)
}
} else {
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-range", fmt.Sprintf("%s,%s,%d,%s", networkGetIP(subnet, 2),
networkGetIP(subnet, -1), subnetSize, expiry)}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--dhcp-range", fmt.Sprintf("%s,%s,%d,%s", networkGetIP(subnet, 2),
networkGetIP(subnet, -1), subnetSize, expiry)}...)
}
} else {
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-range", fmt.Sprintf("::,constructor:%s,ra-stateless,ra-names",
n.name)}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--dhcp-range", fmt.Sprintf("::,constructor:%s,ra-stateless,ra-names",
n.name)}...)
}
} else {
- dnsmasqCmd = append(dnsmasqCmd,
[]string{"--dhcp-range", fmt.Sprintf("::,constructor:%s,ra-only", n.name)}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"--dhcp-range", fmt.Sprintf("::,constructor:%s,ra-only", n.name)}...)
}
// Allow forwarding
@@ -1723,7 +1723,7 @@ func (n *network) Setup(oldConfig map[string]string)
error {
expiry = n.config["ipv4.dhcp.expiry"]
}
- dnsmasqCmd = append(dnsmasqCmd, []string{
+ dnsmasqArgs = append(dnsmasqArgs, []string{
fmt.Sprintf("--listen-address=%s", addr[0]),
"--dhcp-no-override", "--dhcp-authoritative",
fmt.Sprintf("--dhcp-leasefile=%s",
shared.VarPath("networks", n.name, "dnsmasq.leases")),
@@ -1917,11 +1917,11 @@ func (n *network) Setup(oldConfig map[string]string)
error {
if n.config["dns.mode"] != "none" {
if dnsClustered {
- dnsmasqCmd = append(dnsmasqCmd, "-s", dnsDomain)
- dnsmasqCmd = append(dnsmasqCmd, "-S",
fmt.Sprintf("/%s/%s#1053", dnsDomain, dnsClusteredAddress))
- dnsmasqCmd = append(dnsmasqCmd,
fmt.Sprintf("--rev-server=%s,%s#1053", overlaySubnet, dnsClusteredAddress))
+ dnsmasqArgs = append(dnsmasqArgs, "-s",
dnsDomain)
+ dnsmasqArgs = append(dnsmasqArgs, "-S",
fmt.Sprintf("/%s/%s#1053", dnsDomain, dnsClusteredAddress))
+ dnsmasqArgs = append(dnsmasqArgs,
fmt.Sprintf("--rev-server=%s,%s#1053", overlaySubnet, dnsClusteredAddress))
} else {
- dnsmasqCmd = append(dnsmasqCmd, []string{"-s",
dnsDomain, "-S", fmt.Sprintf("/%s/", dnsDomain)}...)
+ dnsmasqArgs = append(dnsmasqArgs,
[]string{"-s", dnsDomain, "-S", fmt.Sprintf("/%s/", dnsDomain)}...)
}
}
@@ -1930,11 +1930,11 @@ func (n *network) Setup(oldConfig map[string]string)
error {
if err != nil {
return err
}
- dnsmasqCmd = append(dnsmasqCmd, fmt.Sprintf("--conf-file=%s",
shared.VarPath("networks", n.name, "dnsmasq.raw")))
+ dnsmasqArgs = append(dnsmasqArgs, fmt.Sprintf("--conf-file=%s",
shared.VarPath("networks", n.name, "dnsmasq.raw")))
// Attempt to drop privileges
if n.state.OS.UnprivUser != "" {
- dnsmasqCmd = append(dnsmasqCmd, []string{"-u",
n.state.OS.UnprivUser}...)
+ dnsmasqArgs = append(dnsmasqArgs, []string{"-u",
n.state.OS.UnprivUser}...)
}
// Create DHCP hosts directory
@@ -1951,10 +1951,26 @@ func (n *network) Setup(oldConfig map[string]string)
error {
return fmt.Errorf("dnsmasq is required for LXD managed
bridges")
}
- // Start dnsmasq (occasionally races, try a few times)
- _, err = shared.TryRunCommand(dnsmasqCmd[0], dnsmasqCmd[1:]...)
+ // Create subprocess object dnsmasq (occasionally races, try a
few times)
+ p, err := subprocess.NewProcess(command, dnsmasqArgs, "", "",
"")
if err != nil {
- return fmt.Errorf("Failed to run: %s: %v",
strings.Join(dnsmasqCmd, " "), err)
+ return fmt.Errorf("Failed to create subprocess: %s",
err)
+ }
+
+ err = p.Start()
+ if err != nil {
+ return fmt.Errorf("Failed to run: %s %s: %v", command,
strings.Join(dnsmasqArgs, " "), err)
+ }
+
+ err = p.Save(shared.VarPath("networks", n.name,
"dnsmasq.pidfd"))
+ if err != nil {
+ //Kill Process if started, but could not save the file
+ err2 := p.Kill()
+ if err != nil {
+ return fmt.Errorf("Could not kill subprocess
while handling saving error: %s: %s", err, err2)
+ }
+
+ return fmt.Errorf("Failed to save subprocess details:
%s", err)
}
// Update the static leases
@@ -2226,18 +2242,33 @@ func (n *network) spawnForkDNS(listenAddress string)
error {
dnsDomain = "lxd"
}
- // Spawn the daemon
- _, err := shared.RunCommand(
- n.state.OS.ExecPath,
- "forkdns",
+ // Spawn the daemon using subprocess
+ command := n.state.OS.ExecPath
+ forkdnsargs := []string{"forkdns",
shared.LogPath(fmt.Sprintf("forkdns.%s.log", n.name)),
- shared.VarPath("networks", n.name, "forkdns.pid"),
fmt.Sprintf("%s:1053", listenAddress),
dnsDomain,
- n.name,
- )
+ n.name}
+
+ p, err := subprocess.NewProcess(command, forkdnsargs, "", "", "")
if err != nil {
- return err
+ return fmt.Errorf("Failed to create subprocess: %s", err)
+ }
+
+ err = p.Start()
+ if err != nil {
+ return fmt.Errorf("Failed to run: %s %s: %v", command,
strings.Join(forkdnsargs, " "), err)
+ }
+
+ err = p.Save(shared.VarPath("networks", n.name, "forkdns.pidfd"))
+ if err != nil {
+ //Kill Process if started, but could not save the file
+ err2 := p.Kill()
+ if err != nil {
+ return fmt.Errorf("Could not kill subprocess while
handling saving error: %s: %s", err, err2)
+ }
+
+ return fmt.Errorf("Failed to save subprocess details: %s", err)
}
return nil
diff --git a/lxd/networks_utils.go b/lxd/networks_utils.go
index 0f6194edef..5935423491 100644
--- a/lxd/networks_utils.go
+++ b/lxd/networks_utils.go
@@ -32,6 +32,7 @@ import (
"github.com/lxc/lxd/shared"
"github.com/lxc/lxd/shared/api"
"github.com/lxc/lxd/shared/logger"
+ "github.com/lxc/lxd/shared/subprocess"
)
var forkdnsServersLock sync.Mutex
@@ -574,51 +575,20 @@ func networkFanAddress(underlay *net.IPNet, overlay
*net.IPNet) (string, string,
func networkKillForkDNS(name string) error {
// Check if we have a running forkdns at all
- pidPath := shared.VarPath("networks", name, "forkdns.pid")
- if !shared.PathExists(pidPath) {
- return nil
- }
+ pidPath := shared.VarPath("networks", name, "forkdns.pidfd")
- // Grab the PID
- content, err := ioutil.ReadFile(pidPath)
+ p, err := subprocess.ImportProcess(pidPath)
if err != nil {
- return err
- }
- pid := strings.TrimSpace(string(content))
-
- // Check for empty string
- if pid == "" {
- os.Remove(pidPath)
- return nil
+ return fmt.Errorf("Could not read pid file: %s", err)
}
- // Check if it's forkdns
- cmdArgs, err := ioutil.ReadFile(fmt.Sprintf("/proc/%s/cmdline", pid))
+ err = p.Stop()
if err != nil {
- os.Remove(pidPath)
- return nil
- }
-
- cmdFields := strings.Split(string(bytes.TrimRight(cmdArgs,
string("\x00"))), string(byte(0)))
- if len(cmdFields) < 5 || cmdFields[1] != "forkdns" {
- os.Remove(pidPath)
- return nil
- }
-
- // Parse the pid
- pidInt, err := strconv.Atoi(pid)
- if err != nil {
- return err
- }
-
- // Actually kill the process
- err = unix.Kill(pidInt, unix.SIGKILL)
- if err != nil {
- return err
+ if !strings.Contains(err.Error(), "Process is not running") {
+ return fmt.Errorf("Unable to kill dnsmasq: %s", err)
+ }
}
- // Cleanup
- os.Remove(pidPath)
return nil
}
diff --git a/shared/subprocess/proc.go b/shared/subprocess/proc.go
index 4b69d75b93..ea76a424e5 100644
--- a/shared/subprocess/proc.go
+++ b/shared/subprocess/proc.go
@@ -13,7 +13,7 @@ import (
// Process struct. Has ability to set runtime arguments
type Process struct {
- pid int64
+ Pid int64 `yaml:"pid"`
Name string `yaml:"name"`
Args []string `yaml:"args,flow"`
Stdin string `yaml:"stdin"`
@@ -22,11 +22,11 @@ type Process struct {
}
// Pid returns the pid for the given process object
-func (p *Process) Pid() (int64, error) {
- pr, _ := os.FindProcess(int(p.pid))
+func (p *Process) GetPid() (int64, error) {
+ pr, _ := os.FindProcess(int(p.Pid))
err := pr.Signal(syscall.Signal(0))
if err == nil {
- return p.pid, nil
+ return p.Pid, nil
}
return 0, fmt.Errorf("Process not running, cannot retrieve PID")
@@ -34,7 +34,7 @@ func (p *Process) Pid() (int64, error) {
// Stop will stop the given process object
func (p *Process) Stop() error {
- pr, _ := os.FindProcess(int(p.pid))
+ pr, _ := os.FindProcess(int(p.Pid))
err := pr.Signal(syscall.Signal(0))
if err == nil {
err = pr.Kill()
@@ -87,7 +87,7 @@ func (p *Process) Start() error {
return fmt.Errorf("Unable to start process: %s", err)
}
- p.pid = int64(cmd.Process.Pid)
+ p.Pid = int64(cmd.Process.Pid)
return nil
}
@@ -108,7 +108,7 @@ func (p *Process) Restart() error {
// Reload sends the SIGHUP signal to the given process object
func (p *Process) Reload() error {
- pr, _ := os.FindProcess(int(p.pid))
+ pr, _ := os.FindProcess(int(p.Pid))
err := pr.Signal(syscall.Signal(0))
if err == nil {
err = pr.Signal(syscall.SIGHUP)
@@ -140,7 +140,7 @@ func (p *Process) Save(path string) error {
// Signal will send a signal to the given process object given a signal value
func (p *Process) Signal(signal int64) error {
- pr, _ := os.FindProcess(int(p.pid))
+ pr, _ := os.FindProcess(int(p.Pid))
err := pr.Signal(syscall.Signal(0))
if err == nil {
err = pr.Signal(syscall.Signal(signal))
@@ -157,7 +157,7 @@ func (p *Process) Signal(signal int64) error {
// Wait will wait for the given process object exit code
func (p *Process) Wait() (int64, error) {
- pr, _ := os.FindProcess(int(p.pid))
+ pr, _ := os.FindProcess(int(p.Pid))
err := pr.Signal(syscall.Signal(0))
if err == nil {
procstate, err := pr.Wait()
_______________________________________________
lxc-devel mailing list
lxc-devel@lists.linuxcontainers.org
http://lists.linuxcontainers.org/listinfo/lxc-devel
