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

Implement lb-method fixes #94 #168

Merged
merged 1 commit into from
Aug 18, 2017
Merged

Conversation

sajal
Copy link

@sajal sajal commented Aug 16, 2017

Image built from my fork : turbobytes/nginx-ingress:0.9.0

This adds a configmap key lb-method and annotation nginx.org/lb-method as suggested by @pleshakov in #94 .

If this setting is not blank, then the value is pasted into the upstream block in the nginx config. This is configurable per Ingress, and not per service.

@pleshakov
Copy link
Contributor

@sajal
thanks for the PR!

Here is a couple of suggestions:

  1. In the documentation, could you change the default value from N/A to "" and also add the following statement after "Sets the load balancing method.": The "" specifies the round-robin method.
  2. I think it is better to pass the ingCfg to the createUpstream and extract the method from it, rather than from ingEx as in https://github.com/turbobytes/kubernetes-ingress/blob/e4a43d4e700969fd9d71a90fd879350ba2c3f486/nginx-controller/nginx/configurator.go#L449 Thus, we don't need to have LBMethod string as part of IngressEx. It is sufficient to have LBMethod only in Config and Upstream

@sajal
Copy link
Author

sajal commented Aug 16, 2017

@pleshakov Good ideas. I will look into them now.

@sajal
Copy link
Author

sajal commented Aug 16, 2017

@pleshakov Now im passing LBMethod as argument instead of using IngressEx

@pleshakov
Copy link
Contributor

@sajal great! thx

I have two more suggestions:

Could you move this logic https://github.com/turbobytes/kubernetes-ingress/blob/b819d7e0acdb119541b7d91228167af99c6e5b5f/nginx-controller/nginx/configurator.go#L86-L88 to the createConfig method for consistency. This way all annotation parsing happens only in one place.

Could you also squash you commits into a single one?

@sajal
Copy link
Author

sajal commented Aug 17, 2017

Ah so I will overwrite the annotation value id present at ingCfg in createConfig.

@sajal
Copy link
Author

sajal commented Aug 17, 2017

@pleshakov : moved annotation checking into createConfig and squash'd

@pleshakov pleshakov merged commit 8117c06 into nginxinc:master Aug 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants