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.

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

Jump to bottom

feat(#733)Support nginx bandwidth control #1118

Merged
merged 1 commit into from
Aug 13, 2017
Merged

feat(#733)Support nginx bandwidth control #1118

merged 1 commit into from
Aug 13, 2017

Conversation

zjj2wry
Copy link

@zjj2wry zjj2wry commented Aug 12, 2017 •
edited
Loading

closes #733 @rikatz I has finish this feature. Can you help me review this. thanks 😊

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 12, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 44.721% when pulling 36180cf on zjj2wry:limit-rate into 36cf018 on kubernetes:master.

aledbf
aledbf reviewed Aug 12, 2017
View reviewed changes
core/pkg/ingress/annotations/ratelimit/main.go Outdated
@@ -48,6 +51,10 @@ type RateLimit struct {
RPS Zone `json:"rps"`

RPM Zone `json:"rpm"`

LimitRate int `json:"limitRate"`
Copy link
Member

Choose a reason for hiding this comment

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

limit-rate

aledbf
aledbf reviewed Aug 12, 2017
View reviewed changes
core/pkg/ingress/annotations/ratelimit/main.go Outdated

LimitRate int `json:"limitRate"`

LimitRateAfter int `json:"limitRateAfter"`
Copy link
Member

Choose a reason for hiding this comment

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

limit-rate-after

@aledbf
Copy link
Member

aledbf commented Aug 12, 2017

@zjj2wry this looks great. Thank you for doing this!

@aledbf aledbf self-assigned this Aug 12, 2017
@rikatz
Copy link
Contributor

rikatz commented Aug 13, 2017

@zjj2wry You're doing a great job :) I've just missed some things:

  1. Some docs / examples of this feature, how to use it, what's the impact
  2. I'll test this probably on monday, but taking a quick overview I've seen that you haven't modified the nginx.tmpl file. Am I missing something? :)

Also, @aledbf made some comments and is the better one to help you with other reviews here and in other PRs!

Thank you very much!

@zjj2wry
Copy link
Author

zjj2wry commented Aug 13, 2017

@aledbf thank you for review.

@zjj2wry
Copy link
Author

zjj2wry commented Aug 13, 2017

@rikatz
1、if this PR approved, i will add some doc for this.
2、i add the limit-rate into ratelimit package. because they seems same type feature. and template func in https://github.com/zjj2wry/ingress/blob/890c57f2ca1c85d84e5bad821d9deae28959c069/controllers/nginx/pkg/template/template.go#L381

@coveralls
Copy link

Coverage Status

Coverage increased (+0.008%) to 44.682% when pulling 890c57f on zjj2wry:limit-rate into 0905311 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Aug 13, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2017
@aledbf
Copy link
Member

aledbf commented Aug 13, 2017

@zjj2wry thanks!

@aledbf aledbf merged commit 854da22 into kubernetes:master Aug 13, 2017
@rikatz
Copy link
Contributor

rikatz commented Aug 14, 2017

@zjj2wry Thank you very much!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aledbf aledbf aledbf left review comments

Assignees

@aledbf aledbf

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: NGINX Bandwidth Control
6 participants
@zjj2wry @k8s-reviewable @coveralls @aledbf @rikatz @k8s-ci-robot