-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Make EphemeralRunnerController MaxConcurrentReconciles configurable #3832
Changes from 2 commits
0d88dae
b0f52ba
0bebb66
f8b1adc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
package actionsgithubcom | ||
|
||
import ( | ||
"fmt" | ||
"os" | ||
"strconv" | ||
|
||
"sigs.k8s.io/controller-runtime/pkg/builder" | ||
"sigs.k8s.io/controller-runtime/pkg/controller" | ||
) | ||
|
||
// Options is the optional configuration for the controllers, which can be | ||
// set via command-line flags or environment variables. | ||
type Options struct { | ||
// RunnerMaxConcuncurrentReconciles is the maximum number of concurrent Reconciles which can be run | ||
// by the EphemeralRunnerController. | ||
RunnerMaxConcuncurrentReconciles int | ||
} | ||
|
||
// OptionsWithDefault returns the default options. | ||
// This is here to maintain the options and their default values in one place, | ||
// rather than having to correlate those in multiple places. | ||
func OptionsWithDefault() Options { | ||
return Options{ | ||
RunnerMaxConcuncurrentReconciles: 2, | ||
} | ||
} | ||
|
||
// LoadEnv loads the options from the environment variables. | ||
// This updates the option value only if the environment variable is set. | ||
// If the option is already set (via a command-line flag), the value from the environment variable takes precedence. | ||
func (o *Options) LoadEnv() error { | ||
v, err := o.getEnvInt("RUNNER_MAX_CONCURRENT_RECONCILES") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this env just to make it easy to give it a shot without modifying the chart. Instead, we can modify the manager args template there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mumoshu - I recommend we go with modifying the chart and adding this as an extra element under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if err != nil { | ||
return err | ||
} | ||
|
||
if v != nil { | ||
o.RunnerMaxConcuncurrentReconciles = *v | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func (o *Options) getEnvInt(name string) (*int, error) { | ||
s := os.Getenv(name) | ||
if s == "" { | ||
return nil, nil | ||
} | ||
|
||
v, err := strconv.Atoi(s) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to convert %s=%s to int: %w", name, s, err) | ||
} | ||
|
||
return &v, nil | ||
} | ||
|
||
type Option func(*controller.Options) | ||
|
||
// WithMaxConcurrentReconciles sets the maximum number of concurrent Reconciles which can be run. | ||
// | ||
// This is useful to improve the throughput of the controller, but it may also increase the load on the API server and | ||
// the external service (e.g. GitHub API). The default value is 1, as defined by the controller-runtime. | ||
// | ||
// See https://github.com/actions/actions-runner-controller/issues/3021 for more information | ||
// on real-world use cases and the potential impact of this option. | ||
func WithMaxConcurrentReconciles(n int) Option { | ||
return func(b *controller.Options) { | ||
b.MaxConcurrentReconciles = n | ||
} | ||
} | ||
|
||
// builderWithOptions applies the given options to the provided builder, if any. | ||
// This is a helper function to avoid the need to import the controller-runtime package in every reconciler source file | ||
// and the command package that creates the controller. | ||
// This is also useful for reducing code duplication around setting controller options in | ||
// multiple reconcilers. | ||
func builderWithOptions(b *builder.Builder, opts []Option) *builder.Builder { | ||
if len(opts) == 0 { | ||
return b | ||
} | ||
|
||
var controllerOpts controller.Options | ||
for _, opt := range opts { | ||
opt(&controllerOpts) | ||
} | ||
|
||
return b.WithOptions(controllerOpts) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making 2 the new default so that users can benefit from the update without any config changes.
Formerly it was treated 1 as not specified, and having 2 shouldn't be harmful as you should be able to limit CPU req/lim at the K8s level anyway.