Skip to content

replace nginz disco with native upstream resolver WPB-15302 #4663

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.

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

Conversation

jschaul
Copy link
Member

@jschaul jschaul commented Jul 15, 2025

WPB-15302

  • replace outside upstreams.txt file by an inline block, making use of 'resolve' keyword (available since nginx 1.27.3). Current nix packages pins nginx to 1.28.
nix repl --file nix/default.nix
nix-repl> pkgs.nginx.version
"1.28.0"

(but actually hardcoded nginx to 1.26 for some reason! - So this PR also upgrades nginx from 1.26 to 1.28)

  • remove nginz_disco code, and sidecar deployment
  • drive-by improvement: be more graceful when shutting down: longer graceperiod, and use quit not sigterm (to process in-flight requests before shutting down - hopefully less 502s that way)

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jul 15, 2025
@jschaul jschaul force-pushed the replace-nginz-disco-by-native-upstream-resolution-WPB-15302 branch from 0769ae8 to e38d9c6 Compare July 15, 2025 15:51
@jschaul jschaul marked this pull request as ready for review July 16, 2025 10:39
@jschaul jschaul requested review from a team as code owners July 16, 2025 10:39
Comment on lines +200 to +202
zone {{ $name }} 64k; # needed for dynamic DNS updates
least_conn;
keepalive 32;
Copy link
Contributor

Choose a reason for hiding this comment

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

where do these settings come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are kept from before, see https://github.com/wireapp/wire-server/blob/develop/tools/nginz_disco/nginz_disco.sh#L49-L55 which results in current upstream files looking like

upstream cargohold { 
	 least_conn; 
	 keepalive 32; 
	 server 10.100.213.88:8080 max_fails=3 weight=100; 
}
upstream galeb.integrations { 
	 least_conn; 
	 keepalive 32; 
	 server 10.100.18.166:8080 max_fails=3 weight=100; 
}

The only difference here is the addition of a zone, which according to the docs in https://nginx.org/en/docs/http/ngx_http_upstream_module.html (see 'resolve' keyword) is needed, as

The server group must reside in the shared memory.

@fisx fisx changed the title replace nginz disco by native upstream resolver WPB-15302 replace nginz disco with native upstream resolver WPB-15302 Jul 16, 2025
My attempt to try refactorings on the base branch (instead of messing with it
directly.)

Besides minor cleanups, I'm now using an Attoparsec Parser to identify and drop
upsteam <name> { <code> } blocks (blocks can be nested, thus a simple regex
won't help) and Interpolate Template Haskell templates to generate new upstream
blocks.
We still need to drop the existing upstreams to add new (adjusted) ones.
Stop at the first '}' which is seen.
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

LGTM

@supersven supersven merged commit ea551b9 into develop Jul 29, 2025
8 checks passed
@supersven supersven deleted the replace-nginz-disco-by-native-upstream-resolution-WPB-15302 branch July 29, 2025 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants