Skip to content

Add option for node-cache to reload in-process CoreDNS via a signal. #689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 22, 2025
+20 −0
Merged

Add option for node-cache to reload in-process CoreDNS via a signal. #689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions cmd/node-cache/app/cache_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ limitations under the License.
package app

import (
"fmt"
"net"
"os"
"strings"
Expand Down Expand Up @@ -52,6 +53,7 @@ type ConfigParams struct {
HealthPort string // port for the healthcheck
SetupIptables bool
SkipTeardown bool // Indicates whether the iptables rules and interface should be torn down
ReloadWithSignal bool // Indicates config reload should be triggered with SIGUSR1, rather than expecting CoreDNS's reload plugin
Copy link
Member

@bowei bowei May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The classic way Unix daemons were implemented is to use HUP as the reload signal. Do we need to use SIGUSR1

Copy link
Contributor Author

@Michcioperz Michcioperz Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, CoreDNS is not a classic Unix daemon and explicitly ignores SIGHUP https://github.com/coredns/caddy/blob/8de985351a985c280155aad02f96df67817a74b4/sigtrap_posix.go#L101

Copy link
Member

@johnbelamaric johnbelamaric Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, interesting, because our docs say SIGHUP should work in a few different places, like https://coredns.io/plugins/reload/

}

type iptablesRule struct {
Expand All @@ -69,6 +71,7 @@ type CacheApp struct {
kubednsConfig *options.KubeDNSConfig
exitChan chan struct{} // Channel to terminate background goroutines
clusterDNSIP net.IP
selfProcess *os.Process
Copy link
Member

@bowei bowei May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we need to cache this

Copy link
Contributor Author

@Michcioperz Michcioperz Aug 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was writing this defensively. Intuitively os.FindProcess(os.Getpid()) shouldn't fail, but it is an error-returning function and I'd rather not find in 3 months that there's this funny edge case in go's stdlib where it fails.

Following from that, if we assume that it can return an error, startup is a cleaner moment to handle that error, than the moment of reload – because then our options are either to ignore the error and stay unreloaded (bad), or to panic and take down a traffic serving daemon (bad).

}

func isLockedErr(err error) bool {
Expand Down Expand Up @@ -277,6 +280,13 @@ func (c *CacheApp) RunApp() {
// NewCacheApp returns a new instance of CacheApp by applying the specified config params.
func NewCacheApp(params *ConfigParams) (*CacheApp, error) {
c := &CacheApp{params: params, kubednsConfig: options.NewKubeDNSConfig()}
if params.ReloadWithSignal {
var err error
c.selfProcess, err = os.FindProcess(os.Getpid())
if err != nil {
return nil, fmt.Errorf("failed to get process handle on self: %w", err)
}
}
c.clusterDNSIP = net.ParseIP(os.ExpandEnv(toSvcEnv(params.UpstreamSvcName)))
if c.clusterDNSIP == nil {
clog.Warningf("Unable to lookup IP address of Upstream service %s, env %s `%s`", params.UpstreamSvcName, toSvcEnv(params.UpstreamSvcName), os.ExpandEnv(toSvcEnv(params.UpstreamSvcName)))
Expand Down
9 changes: 9 additions & 0 deletions cmd/node-cache/app/configmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"os"
"path"
"strings"
"syscall"
"text/template"
"time"

Expand Down Expand Up @@ -132,6 +133,14 @@ func (c *CacheApp) updateCorefile(dnsConfig *config.Config) {
}
clog.Infof("Updated Corefile with %d custom stubdomains and upstream servers %s", len(dnsConfig.StubDomains), upstreamServers)
clog.Infof("Using config file:\n%s", newConfig.String())

// Trigger reload of in-process CoreDNS
if c.selfProcess != nil {
Copy link
Member

@bowei bowei May 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the flag vs keeping this state?

Alternative:

if c.params.ReloadWithSignal {
c.selfProcess, err = os.FindProcess(os.Getpid())
...
}

Copy link
Contributor Author

@Michcioperz Michcioperz Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I addressed this in the other comment https://github.com/kubernetes/dns/pull/689/files/9c1282aeac745cd31fca73edf2f35e65ba1bea8c#r2283317738

clog.Infof("Sending reload signal to PID %v", c.selfProcess.Pid)
if err := c.selfProcess.Signal(syscall.SIGUSR1); err != nil {
clog.Errorf("Failed to trigger CoreDNS reload: %v", err)
}
}
}

// syncInfo contains all parameters needed to watch a configmap directory for updates
Expand Down
1 change: 1 addition & 0 deletions cmd/node-cache/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ func parseAndValidateFlags() (*app.ConfigParams, error) {
flag.StringVar(&params.UpstreamSvcName, "upstreamsvc", "kube-dns", "Service name whose cluster IP is upstream for node-cache")
flag.StringVar(&params.HealthPort, "health-port", "8080", "port used by health plugin")
flag.BoolVar(&params.SkipTeardown, "skipteardown", false, "indicates whether iptables rules should be torn down on exit")
flag.BoolVar(&params.ReloadWithSignal, "reloadwithsignal", false, "use SIGUSR1 on self to reload CoreDNS")
flag.Parse()

for _, ipstr := range strings.Split(params.LocalIPStr, ",") {
Expand Down