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

adding kubernetes core rate limiter handlers #3472

Merged
merged 1 commit into from
Oct 27, 2017
Merged

adding kubernetes core rate limiter handlers #3472

merged 1 commit into from
Oct 27, 2017

Conversation

chrislovecnm
Copy link
Contributor

@chrislovecnm chrislovecnm commented Sep 29, 2017 •
edited
Loading

This PR is re-using the handlers from the k8s core project, to create a global rate limiting.

This work starts work on #3471

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 29, 2017
@chrislovecnm
Copy link
Contributor Author

/approve

@chrislovecnm chrislovecnm added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Sep 29, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2017
justinsb
justinsb reviewed Oct 9, 2017
View reviewed changes
cmd/kops/status_discovery.go Outdated
func (s *cloudDiscoveryStatusStore) GetApiIngressStatus(cluster *kops.Cluster) ([]kops.ApiIngressStatus, error) {
glog.V(10).Infof("getting api ingress status")
Copy link
Member

Choose a reason for hiding this comment

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

We already have https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/awsup/request_logger.go, and kubernetes has https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/aws/log_handler.go . Can we use those instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is debugging :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove

upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go Outdated
@@ -75,8 +75,12 @@ func findAutoscalingGroup(cloud awsup.AWSCloud, name string) (*autoscaling.Group
} else {
glog.Warningf("Got ASG with unexpected name %q", aws.StringValue(g.AutoScalingGroupName))
}

glog.V(10).Infof("paged asg: %s", aws.StringValue(g.AutoScalingGroupName))
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? It's V(10), but still a load of information...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

debugging ... I will cleanup

upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go Outdated
}

glog.V(10).Infof("finished finding autoscaling groups")
Copy link
Member

Choose a reason for hiding this comment

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

I'd hope the normal logger would log these

cmd/kops/status_discovery.go Outdated
@@ -33,7 +34,10 @@ type cloudDiscoveryStatusStore struct {

var _ kops.StatusStore = &cloudDiscoveryStatusStore{}

// TODO - is this dead code??
Copy link
Member

Choose a reason for hiding this comment

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

It is not

upup/pkg/fi/cloudup/awsup/aws_cloud.go
@@ -40,6 +42,7 @@ import (
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kubernetes/federation/pkg/dnsprovider"
dnsproviderroute53 "k8s.io/kubernetes/federation/pkg/dnsprovider/providers/aws/route53"
k8s_aws "k8s.io/kubernetes/pkg/cloudprovider/providers/aws"
Copy link
Member

Choose a reason for hiding this comment

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

Let's just copy it in, not add another k/k dependency

Copy link
Member

Choose a reason for hiding this comment

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

Actually on seconds thoughts it is such a beast it's probably better not to copy it until we have to...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM ... yes beast

@justinsb
Copy link
Member

justinsb commented Oct 9, 2017

Can we dial down the logging (even though it's V(10), it should be handled by the log handler)?

Then looks good.

@chrislovecnm
Copy link
Contributor Author

Go vet is telling me that copying the mutex is bad. Any ideas? Can we not copy the pointer to aws cloud implantation?

@justinsb
Copy link
Member

To work around the mutex copy - put the mutex & map into its own type RegionDelayers struct, store a pointer to it. That's more correct and should give cleaner code.

@chrislovecnm chrislovecnm force-pushed the global-rate-limiter branch 3 times, most recently from 59393c6 to 4397e1d Compare October 12, 2017 03:14
@chrislovecnm chrislovecnm removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. WIP: Work In Progress labels Oct 12, 2017
@chrislovecnm
Copy link
Contributor Author

chrislovecnm commented Oct 12, 2017 •
edited
Loading

@justinsb not sure how we want to test this :P We want to roll with it as is?

Backed out logging. PTAL

justinsb
justinsb reviewed Oct 26, 2017
View reviewed changes
upup/pkg/fi/cloudup/awsup/aws_cloud.go Outdated
// TODO Should we improve LoggingRetryer instead of what k8s core is using?
// TODO According to the aws api docs we will still be using the default retryer
// TODO and then we will have the k8s handlers that are acting as global retriers.
// TODO My thought is keep what we have and add the global handlers.
Copy link
Member

Choose a reason for hiding this comment

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

Yes. Can we demote this from a TODO to an explanation of what we're doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing, as it does not really make sense. I need to limit the paging, not worry about that

justinsb
justinsb reviewed Oct 26, 2017
View reviewed changes
upup/pkg/fi/cloudup/awsup/aws_cloud.go Outdated
// We do this to protect the AWS account from becoming overloaded and effectively locked.
// We also log when we hit request limits.
// Note that this delays the current goroutine; this is bad behaviour and will
// likely cause k8s to become slow or unresponsive for cloud operations.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: s/k8s/kops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

caught another from copy pasta programing

@justinsb
Copy link
Member

LGTM - can you demote the TODO to just an explanation of what we're doing.

Please self-apply LGTM though!

@chrislovecnm
Copy link
Contributor Author

/lgtm

per @justinsb

@k8s-ci-robot
Copy link
Contributor

@chrislovecnm: you cannot LGTM your own PR.

In response to this:

/lgtm

per @justinsb

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@chrislovecnm
Copy link
Contributor Author

@k8s-bot please hold my water bottle

@chrislovecnm chrislovecnm added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2017
@chrislovecnm
Copy link
Contributor Author

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit 1f4224b into kubernetes:master Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinsb justinsb justinsb left review comments

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chrislovecnm @justinsb @k8s-ci-robot @k8s-github-robot