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

Reduce code redundancy of resyncPeriod calculation #88117

Open
wants to merge 1 commit into
base: master
from

Conversation

@tnqn
Copy link
Contributor

tnqn commented Feb 13, 2020

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
ResyncPeriod is defined multiple times under cmd/ while the same kind of
functions are defined in pkg/controller/controller_utils.go.

type ResyncPeriodFunc func() time.Duration
// Returns 0 for resyncPeriod in case resyncing is not needed.
func NoResyncPeriodFunc() time.Duration {
return 0
}
// StaticResyncPeriodFunc returns the resync period specified
func StaticResyncPeriodFunc(resyncPeriod time.Duration) ResyncPeriodFunc {
return func() time.Duration {
return resyncPeriod
}
}

This patch moves the function to controller_utils.go, renames it to
DynamicResyncPeriodFunc, in the same style as StaticResyncPeriodFunc and
NoResyncPeriodFunc.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


ResyncPeriod is defined multiple times under cmd/ while the same kind of
functions are defined in pkg/controller/controller_utils.go.

This patch moves the function to controller_utils.go, renames it to
DynamicResyncPeriodFunc, in the same style as StaticResyncPeriodFunc and
NoResyncPeriodFunc.
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 13, 2020

Hi @tnqn. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 13, 2020

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tnqn
To complete the pull request process, please assign janetkuo, sttts
You can assign the PR to them by writing /assign @janetkuo @sttts in a comment when ready.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Feb 18, 2020

/ok-to-test
/priority backlog

@fedebongio

This comment has been minimized.

Copy link
Contributor

fedebongio commented Feb 18, 2020

/assign @lavalamp

@@ -208,7 +206,7 @@ func (o *CloudControllerManagerOptions) ApplyTo(c *cloudcontrollerconfig.Config,
c.ClientBuilder = rootClientBuilder
}
c.VersionedClient = rootClientBuilder.ClientOrDie("shared-informers")
c.SharedInformers = informers.NewSharedInformerFactory(c.VersionedClient, resyncPeriod(c)())
c.SharedInformers = informers.NewSharedInformerFactory(c.VersionedClient, controller.DynamicResyncPeriodFunc(c.ComponentConfig.Generic.MinResyncPeriod.Duration)())

This comment has been minimized.

Copy link
@lavalamp

lavalamp Feb 18, 2020

Member

This replacement is super verbose, what if you made this a method, so you could just say something like c.ComponentConfig.Generic.DynamicResyncPeriodFunc?

This comment has been minimized.

Copy link
@lavalamp

lavalamp Feb 18, 2020

Member

Actually it doesn't need to return a function, so just c.ComponentConfig.Generic.DynamicResyncPeriod might be fine.

This comment has been minimized.

Copy link
@tnqn

tnqn Feb 20, 2020

Author Contributor

Sorry, not sure if I understand correctly. c.ComponentConfig.Generic is a config struct in types.go that holds configuration like MinResyncPeriod got from CLI option, I think adding a method to it might break its style?

// GenericControllerManagerConfiguration holds configuration for a generic controller-manager
type GenericControllerManagerConfiguration struct {
// port is the port that the controller-manager's http service runs on.
Port int32
// address is the IP address to serve on (set to 0.0.0.0 for all interfaces).
Address string
// minResyncPeriod is the resync period in reflectors; will be random between
// minResyncPeriod and 2*minResyncPeriod.
MinResyncPeriod metav1.Duration

The reason it's made to return a function is another place is using the returned closure to generate different resyncPeriod for controllers:

// ResyncPeriod generates a duration each time it is invoked; this is so that
// multiple controllers don't get into lock-step and all hammer the apiserver
// with list requests simultaneously.
ResyncPeriod func() time.Duration

The verbose function name was trying to keep same style as the other two resyncPeriod generating functions.

type ResyncPeriodFunc func() time.Duration
// Returns 0 for resyncPeriod in case resyncing is not needed.
func NoResyncPeriodFunc() time.Duration {
return 0
}
// StaticResyncPeriodFunc returns the resync period specified
func StaticResyncPeriodFunc(resyncPeriod time.Duration) ResyncPeriodFunc {
return func() time.Duration {
return resyncPeriod
}
}

Please correct me if I misunderstand you.

Copy link
Contributor Author

tnqn left a comment

Thanks @lavalamp for reviewing.

@@ -208,7 +206,7 @@ func (o *CloudControllerManagerOptions) ApplyTo(c *cloudcontrollerconfig.Config,
c.ClientBuilder = rootClientBuilder
}
c.VersionedClient = rootClientBuilder.ClientOrDie("shared-informers")
c.SharedInformers = informers.NewSharedInformerFactory(c.VersionedClient, resyncPeriod(c)())
c.SharedInformers = informers.NewSharedInformerFactory(c.VersionedClient, controller.DynamicResyncPeriodFunc(c.ComponentConfig.Generic.MinResyncPeriod.Duration)())

This comment has been minimized.

Copy link
@tnqn

tnqn Feb 20, 2020

Author Contributor

Sorry, not sure if I understand correctly. c.ComponentConfig.Generic is a config struct in types.go that holds configuration like MinResyncPeriod got from CLI option, I think adding a method to it might break its style?

// GenericControllerManagerConfiguration holds configuration for a generic controller-manager
type GenericControllerManagerConfiguration struct {
// port is the port that the controller-manager's http service runs on.
Port int32
// address is the IP address to serve on (set to 0.0.0.0 for all interfaces).
Address string
// minResyncPeriod is the resync period in reflectors; will be random between
// minResyncPeriod and 2*minResyncPeriod.
MinResyncPeriod metav1.Duration

The reason it's made to return a function is another place is using the returned closure to generate different resyncPeriod for controllers:

// ResyncPeriod generates a duration each time it is invoked; this is so that
// multiple controllers don't get into lock-step and all hammer the apiserver
// with list requests simultaneously.
ResyncPeriod func() time.Duration

The verbose function name was trying to keep same style as the other two resyncPeriod generating functions.

type ResyncPeriodFunc func() time.Duration
// Returns 0 for resyncPeriod in case resyncing is not needed.
func NoResyncPeriodFunc() time.Duration {
return 0
}
// StaticResyncPeriodFunc returns the resync period specified
func StaticResyncPeriodFunc(resyncPeriod time.Duration) ResyncPeriodFunc {
return func() time.Duration {
return resyncPeriod
}
}

Please correct me if I misunderstand you.

@tnqn

This comment has been minimized.

Copy link
Contributor Author

tnqn commented Feb 20, 2020

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.