Skip to content

Commit 751674d

Browse files
sjbermansjberman
committed
Use state file for updating N+ upstreams
Problem: When splitting the data and control plane, we want to be able to maintain the ability to dynamically update upstreams without a reload if using NGINX Plus. With the current process, we write the upstreams into the nginx conf but don't reload, then call the API to actually do the update. Writing into the conf will trigger a reload from the agent once we split, however. Solution: Don't write the upstream servers into the nginx conf anymore when using NGINX Plus. Instead, utilize the `state` file option that the NGINX Plus API will populate with the upstream servers. This way we can just call the API and don't unintentionally reload by writing servers into the conf.
1 parent 245b516 commit 751674d

File tree

7 files changed

+125
-133
lines changed

7 files changed

+125
-133
lines changed

‎internal/mode/static/handler.go‎

Lines changed: 41 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -182,11 +182,11 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
182182

183183
h.setLatestConfiguration(&cfg)
184184

185-
err = h.updateUpstreamServers(
186-
ctx,
187-
logger,
188-
cfg,
189-
)
185+
if h.cfg.plus {
186+
err = h.updateUpstreamServers(logger, cfg)
187+
} else {
188+
err = h.updateNginxConf(ctx, logger, cfg)
189+
}
190190
case state.ClusterStateChange:
191191
h.version++
192192
cfg := dataplane.BuildConfiguration(ctx, gr, h.cfg.serviceResolver, h.version)
@@ -198,10 +198,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
198198

199199
h.setLatestConfiguration(&cfg)
200200

201-
err = h.updateNginxConf(
202-
ctx,
203-
cfg,
204-
)
201+
err = h.updateNginxConf(ctx, logger, cfg)
205202
}
206203

207204
var nginxReloadRes status.NginxReloadResult
@@ -306,7 +303,11 @@ func (h *eventHandlerImpl) parseAndCaptureEvent(ctx context.Context, logger logr
306303
}
307304

308305
// updateNginxConf updates nginx conf files and reloads nginx.
309-
func (h *eventHandlerImpl) updateNginxConf(ctx context.Context, conf dataplane.Configuration) error {
306+
func (h *eventHandlerImpl) updateNginxConf(
307+
ctx context.Context,
308+
logger logr.Logger,
309+
conf dataplane.Configuration,
310+
) error {
310311
files := h.cfg.generator.Generate(conf)
311312
if err := h.cfg.nginxFileMgr.ReplaceFiles(files); err != nil {
312313
return fmt.Errorf("failed to replace NGINX configuration files: %w", err)
@@ -316,75 +317,52 @@ func (h *eventHandlerImpl) updateNginxConf(ctx context.Context, conf dataplane.C
316317
return fmt.Errorf("failed to reload NGINX: %w", err)
317318
}
318319

319-
return nil
320-
}
321-
322-
// updateUpstreamServers is called only when endpoints have changed. It updates nginx conf files and then:
323-
// - if using NGINX Plus, determines which servers have changed and uses the N+ API to update them;
324-
// - otherwise if not using NGINX Plus, or an error was returned from the API, reloads nginx.
325-
func (h *eventHandlerImpl) updateUpstreamServers(
326-
ctx context.Context,
327-
logger logr.Logger,
328-
conf dataplane.Configuration,
329-
) error {
330-
isPlus := h.cfg.nginxRuntimeMgr.IsPlus()
331-
332-
files := h.cfg.generator.Generate(conf)
333-
if err := h.cfg.nginxFileMgr.ReplaceFiles(files); err != nil {
334-
return fmt.Errorf("failed to replace NGINX configuration files: %w", err)
320+
// If using NGINX Plus, update upstream servers using the API.
321+
if err := h.updateUpstreamServers(logger, conf); err != nil {
322+
return fmt.Errorf("failed to update upstream servers: %w", err)
335323
}
336324

337-
reload := func() error {
338-
if err := h.cfg.nginxRuntimeMgr.Reload(ctx, conf.Version); err != nil {
339-
return fmt.Errorf("failed to reload NGINX: %w", err)
340-
}
325+
return nil
326+
}
341327

328+
// updateUpstreamServers determines which servers have changed and uses the NGINX Plus API to update them.
329+
// Only applicable when using NGINX Plus.
330+
func (h *eventHandlerImpl) updateUpstreamServers(logger logr.Logger, conf dataplane.Configuration) error {
331+
if !h.cfg.plus {
342332
return nil
343333
}
344334

345-
if isPlus {
346-
type upstream struct {
347-
name string
348-
servers []ngxclient.UpstreamServer
349-
}
350-
var upstreams []upstream
351-
352-
prevUpstreams, err := h.cfg.nginxRuntimeMgr.GetUpstreams()
353-
if err != nil {
354-
logger.Error(err, "failed to get upstreams from API, reloading configuration instead")
355-
return reload()
356-
}
335+
type upstream struct {
336+
name string
337+
servers []ngxclient.UpstreamServer
338+
}
339+
var upstreams []upstream
357340

358-
for _, u := range conf.Upstreams {
359-
confUpstream := upstream{
360-
name: u.Name,
361-
servers: ngxConfig.ConvertEndpoints(u.Endpoints),
362-
}
341+
prevUpstreams, err := h.cfg.nginxRuntimeMgr.GetUpstreams()
342+
if err != nil {
343+
return fmt.Errorf("failed to get upstreams from API: %w", err)
344+
}
363345

364-
if u, ok := prevUpstreams[confUpstream.name]; ok {
365-
if !serversEqual(confUpstream.servers, u.Peers) {
366-
upstreams = append(upstreams, confUpstream)
367-
}
368-
}
346+
for _, u := range conf.Upstreams {
347+
confUpstream := upstream{
348+
name: u.Name,
349+
servers: ngxConfig.ConvertEndpoints(u.Endpoints),
369350
}
370351

371-
var reloadPlus bool
372-
for _, upstream := range upstreams {
373-
if err := h.cfg.nginxRuntimeMgr.UpdateHTTPServers(upstream.name, upstream.servers); err != nil {
374-
logger.Error(
375-
err, "couldn't update upstream via the API, reloading configuration instead",
376-
"upstreamName", upstream.name,
377-
)
378-
reloadPlus = true
352+
if u, ok := prevUpstreams[confUpstream.name]; ok {
353+
if !serversEqual(confUpstream.servers, u.Peers) {
354+
upstreams = append(upstreams, confUpstream)
379355
}
380356
}
357+
}
381358

382-
if !reloadPlus {
383-
return nil
359+
for _, upstream := range upstreams {
360+
if err := h.cfg.nginxRuntimeMgr.UpdateHTTPServers(upstream.name, upstream.servers); err != nil {
361+
logger.Error(err, "couldn't update upstream via the API", "upstreamName", upstream.name)
384362
}
385363
}
386364

387-
return reload()
365+
return nil
388366
}
389367

390368
func serversEqual(newServers []ngxclient.UpstreamServer, oldServers []ngxclient.Peer) bool {

‎internal/mode/static/handler_test.go‎

Lines changed: 11 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -428,15 +428,15 @@ var _ = Describe("eventHandler", func() {
428428

429429
When("running NGINX Plus", func() {
430430
It("should call the NGINX Plus API", func() {
431-
fakeNginxRuntimeMgr.IsPlusReturns(true)
431+
handler.cfg.plus = true
432432

433433
handler.HandleEventBatch(context.Background(), ctlrZap.New(), batch)
434434

435435
dcfg := dataplane.GetDefaultConfiguration(&graph.Graph{}, 1)
436436
Expect(helpers.Diff(handler.GetLatestConfiguration(), &dcfg)).To(BeEmpty())
437437

438-
Expect(fakeGenerator.GenerateCallCount()).To(Equal(1))
439-
Expect(fakeNginxFileMgr.ReplaceFilesCallCount()).To(Equal(1))
438+
Expect(fakeGenerator.GenerateCallCount()).To(Equal(0))
439+
Expect(fakeNginxFileMgr.ReplaceFilesCallCount()).To(Equal(0))
440440
Expect(fakeNginxRuntimeMgr.GetUpstreamsCallCount()).To(Equal(1))
441441
})
442442
})
@@ -465,19 +465,6 @@ var _ = Describe("eventHandler", func() {
465465
},
466466
}
467467

468-
type callCounts struct {
469-
generate int
470-
update int
471-
reload int
472-
}
473-
474-
assertCallCounts := func(cc callCounts) {
475-
Expect(fakeGenerator.GenerateCallCount()).To(Equal(cc.generate))
476-
Expect(fakeNginxFileMgr.ReplaceFilesCallCount()).To(Equal(cc.generate))
477-
Expect(fakeNginxRuntimeMgr.UpdateHTTPServersCallCount()).To(Equal(cc.update))
478-
Expect(fakeNginxRuntimeMgr.ReloadCallCount()).To(Equal(cc.reload))
479-
}
480-
481468
BeforeEach(func() {
482469
upstreams := ngxclient.Upstreams{
483470
"one": ngxclient.Upstream{
@@ -491,42 +478,25 @@ var _ = Describe("eventHandler", func() {
491478

492479
When("running NGINX Plus", func() {
493480
BeforeEach(func() {
494-
fakeNginxRuntimeMgr.IsPlusReturns(true)
481+
handler.cfg.plus = true
495482
})
496483

497484
It("should update servers using the NGINX Plus API", func() {
498-
Expect(handler.updateUpstreamServers(context.Background(), ctlrZap.New(), conf)).To(Succeed())
499-
500-
assertCallCounts(callCounts{generate: 1, update: 1, reload: 0})
485+
Expect(handler.updateUpstreamServers(ctlrZap.New(), conf)).To(Succeed())
486+
Expect(fakeNginxRuntimeMgr.UpdateHTTPServersCallCount()).To(Equal(1))
501487
})
502488

503-
It("should reload when GET API returns an error", func() {
489+
It("should return error when GET API returns an error", func() {
504490
fakeNginxRuntimeMgr.GetUpstreamsReturns(nil, errors.New("error"))
505-
Expect(handler.updateUpstreamServers(context.Background(), ctlrZap.New(), conf)).To(Succeed())
506-
507-
assertCallCounts(callCounts{generate: 1, update: 0, reload: 1})
508-
})
509-
510-
It("should reload when POST API returns an error", func() {
511-
fakeNginxRuntimeMgr.UpdateHTTPServersReturns(errors.New("error"))
512-
Expect(handler.updateUpstreamServers(context.Background(), ctlrZap.New(), conf)).To(Succeed())
513-
514-
assertCallCounts(callCounts{generate: 1, update: 1, reload: 1})
491+
Expect(handler.updateUpstreamServers(ctlrZap.New(), conf)).ToNot(Succeed())
515492
})
516493
})
517494

518495
When("not running NGINX Plus", func() {
519-
It("should update servers by reloading", func() {
520-
Expect(handler.updateUpstreamServers(context.Background(), ctlrZap.New(), conf)).To(Succeed())
521-
522-
assertCallCounts(callCounts{generate: 1, update: 0, reload: 1})
523-
})
524-
525-
It("should return an error when reloading fails", func() {
526-
fakeNginxRuntimeMgr.ReloadReturns(errors.New("error"))
527-
Expect(handler.updateUpstreamServers(context.Background(), ctlrZap.New(), conf)).ToNot(Succeed())
496+
It("should not do anything", func() {
497+
Expect(handler.updateUpstreamServers(ctlrZap.New(), conf)).To(Succeed())
528498

529-
assertCallCounts(callCounts{generate: 1, update: 0, reload: 1})
499+
Expect(fakeNginxRuntimeMgr.UpdateHTTPServersCallCount()).To(Equal(0))
530500
})
531501
})
532502
})

‎internal/mode/static/nginx/config/http/config.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,10 @@ const (
8282

8383
// Upstream holds all configuration for an HTTP upstream.
8484
type Upstream struct {
85-
Name string
86-
ZoneSize string // format: 512k, 1m
87-
Servers []UpstreamServer
85+
Name string
86+
ZoneSize string // format: 512k, 1m
87+
StateFile string
88+
Servers []UpstreamServer
8889
}
8990

9091
// UpstreamServer holds all configuration for an HTTP upstream server.

‎internal/mode/static/nginx/config/stream/config.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,10 @@ type Server struct {
1515

1616
// Upstream holds all configuration for a stream upstream.
1717
type Upstream struct {
18-
Name string
19-
ZoneSize string // format: 512k, 1m
20-
Servers []UpstreamServer
18+
Name string
19+
ZoneSize string // format: 512k, 1m
20+
StateFile string // FIXME(sberman): temporary until stream upstreams template is split in UpstreamSettingsPolicy work.
21+
Servers []UpstreamServer
2122
}
2223

2324
// UpstreamServer holds all configuration for a stream upstream server.

‎internal/mode/static/nginx/config/upstreams.go‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ const (
2727
ossZoneSizeStream = "512k"
2828
// plusZoneSize is the upstream zone size for nginx plus.
2929
plusZoneSizeStream = "1m"
30+
// stateDir is the directory for storing state files.
31+
stateDir = "/var/lib/nginx/state"
3032
)
3133

3234
func (g GeneratorImpl) executeUpstreams(conf dataplane.Configuration) []executeResult {
@@ -101,15 +103,18 @@ func (g GeneratorImpl) createUpstreams(upstreams []dataplane.Upstream) []http.Up
101103
}
102104

103105
func (g GeneratorImpl) createUpstream(up dataplane.Upstream) http.Upstream {
106+
var stateFile string
104107
zoneSize := ossZoneSize
105108
if g.plus {
106109
zoneSize = plusZoneSize
110+
stateFile = fmt.Sprintf("%s/%s.conf", stateDir, up.Name)
107111
}
108112

109113
if len(up.Endpoints) == 0 {
110114
return http.Upstream{
111-
Name: up.Name,
112-
ZoneSize: zoneSize,
115+
Name: up.Name,
116+
ZoneSize: zoneSize,
117+
StateFile: stateFile,
113118
Servers: []http.UpstreamServer{
114119
{
115120
Address: nginx503Server,
@@ -130,9 +135,10 @@ func (g GeneratorImpl) createUpstream(up dataplane.Upstream) http.Upstream {
130135
}
131136

132137
return http.Upstream{
133-
Name: up.Name,
134-
ZoneSize: zoneSize,
135-
Servers: upstreamServers,
138+
Name: up.Name,
139+
ZoneSize: zoneSize,
140+
StateFile: stateFile,
141+
Servers: upstreamServers,
136142
}
137143
}
138144

‎internal/mode/static/nginx/config/upstreams_template.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,13 @@ upstream {{ $u.Name }} {
1212
{{ if $u.ZoneSize -}}
1313
zone {{ $u.Name }} {{ $u.ZoneSize }};
1414
{{ end -}}
15-
{{ range $server := $u.Servers }}
15+
16+
{{- if $u.StateFile }}
17+
state {{ $u.StateFile }};
18+
{{- else }}
19+
{{ range $server := $u.Servers }}
1620
server {{ $server.Address }};
21+
{{- end }}
1722
{{- end }}
1823
}
1924
{{ end -}}

0 commit comments

Comments
 (0)