Skip to content
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

Add max connections to TransportServer #1480

Merged

Conversation

soneillf5
Copy link
Contributor

Proposed changes

This change allows users to specify the max number of connections to the proxied server. https://nginx.org/en/docs/stream/ngx_stream_upstream_module.html#max_conns

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Mar 23, 2021
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Hi @soneillf5
Please see my feedback

pkg/apis/configuration/v1alpha1/types.go Outdated Show resolved Hide resolved
internal/configs/version2/nginx.transportserver.tmpl Outdated Show resolved Hide resolved
internal/configs/version2/stream.go Show resolved Hide resolved
@soneillf5 soneillf5 force-pushed the feature-max-connections-transportserver-upstream-server branch from 3ed67a3 to 8c08880 Compare May 4, 2021 11:37
@soneillf5 soneillf5 requested review from vepatel and removed request for ciarams87 and lucacome May 4, 2021 11:37
* Build the docker image with an increased version number ```docker build -t tcp-server:v2```
* Push the docker image to the public repo
* Build the docker image with an increased version number ```docker build -t tcp-server:2.1```
* Push the docker image to the public repo ```docker push seanoneillf5/tcp-server:2.1```
Copy link
Contributor

Choose a reason for hiding this comment

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

should we not specify the exact repo name and replace it with <public_repo_name> as user will be creating their own?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can create a team/company repo?
cc @ciarams87

@soneillf5 soneillf5 requested a review from vepatel May 4, 2021 14:36
tests/settings.py Outdated Show resolved Hide resolved
Copy link
Contributor

@pleshakov pleshakov left a comment

Choose a reason for hiding this comment

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

Hi @soneillf5

The PR looks good to me, expect for the name of the field.

Could we use maxConns as the name of the field? There are two reasons:

(1) With new resources - Policies and TransportServer - we started using camelCase for field names. However, we still want to be consistent with the VS/VSR resources. For max conns, VS/VSR upstreams have max-conns field. This is similar to how NGINX uses the same directive and parameters in HTTP and TCP configuration.
(2) For fields related to parameters and tunables (for example, load balancing parameters, rate-limiting parameters, etc.), we reuse NGINX nomenclature. For example, the existing maxFails, failTimeout in TransportServer.

* Build the docker image with an increased version number ```docker build -t tcp-server:v2```
* Push the docker image to the public repo
* Build the docker image with an increased version number ```docker build -t tcp-server:2.1```
* Push the docker image to the public repo ```docker push seanoneillf5/tcp-server:2.1```
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can create a team/company repo?
cc @ciarams87

@soneillf5 soneillf5 requested a review from pleshakov May 6, 2021 08:46
@soneillf5
Copy link
Contributor Author

@pleshakov I've updated it to 'maxConns'. I think it's better not to, but since you've blocked the PR, I've made the change.

@soneillf5 soneillf5 force-pushed the feature-max-connections-transportserver-upstream-server branch from 6577a86 to f54e24c Compare May 6, 2021 08:51
@soneillf5 soneillf5 force-pushed the feature-max-connections-transportserver-upstream-server branch from f54e24c to 36fabaf Compare May 7, 2021 09:04
@soneillf5 soneillf5 merged commit 61f0032 into master May 7, 2021
@lucacome lucacome deleted the feature-max-connections-transportserver-upstream-server branch May 7, 2021 18:34
@pleshakov pleshakov changed the title Add max connections config to TransportServer Add max connections to TransportServer Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Pull requests/issues for documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants