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

Allow to override options for specific controllers #2030

Conversation

max-melentyev
Copy link
Contributor

@max-melentyev max-melentyev commented Apr 4, 2024

Description of your changes

We're facing long poll queues when using crossplane to manage thousands of EC2 instances,
and facing AWS API throttling while observing thousands of route53 records (#2029).
As an intermediate step we want to reduce poll interval for these resources and keep it short for other resources.

In further PRs I plan to make these resources to be monitored using EventBridge events
and these controller to have even longer poll intervals.
Resources without EventBridge events will still have short poll intervals.

Options are parsed from env variables with PROVIDER_AWS_ prefix, for example: PROVIDER_AWS_ec2.instance.pollInterval=10m.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

  • with new tests
  • manually in dev setup with fmt.Printf :)

Later some resources may be monitored using EventBridge events
and these controller will have longer poll intervals.
Resources without EventBridge events will have short poll intervals.

Options are parsed from env variables with `PROVIDER_AWS_` prefix,
for example: `PROVIDER_AWS_ec2.instance.pollInterval=10m`.

Signed-off-by: Max Melentyev <[email protected]>
@max-melentyev
Copy link
Contributor Author

I'm happy to add more docs on this feature, but I didn't find any appropriate place for it other than source code comments. Please let me know if there's a better option.


// Collects all env variables with the prefix OPTION_ENV_PREFIX and returns them as a map
// with the prefix removed.
func optionsOverridesFromEnv() map[string]string {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't kingpin support natively loading flags from environment variables?

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 see it has binding of particular flag to an env variable. This will add ~320 new flags and it'll still require some code to collect these flags into overrides map.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not a maintainer of this provider anymore, so I'll defer to @MisterMX and @chlunde.

That said, I'm a bit concerned about:

  • Adding a feature to only this provider to tune poll interval per resource type
  • Introducing a new path to configure the provider (environment variables)

I think if this is a problem for this provider it's probably a problem for other providers. It would be ideal to agree on the problem, and a standard way to address it. Usually to do that we'd write a one-pager in the crossplane/crossplane repo, that provider owners can adopt.

@ulucinar is the TL of Upbound's extensions team (he owns tools like upjet that generate a lot of the other providers). He might be a good person to review a one-pager if you're interested in writing one.

@max-melentyev max-melentyev requested a review from negz April 5, 2024 09:44
@MisterMX
Copy link
Collaborator

MisterMX commented Apr 8, 2024

This looks like a very drastic change to me. And I honestly don't think that we are going merge that since it is very custom solution for a single provider which does not comply to the other providers.

I would prefer a more general approach that can be included in https://github.com/crossplane/crossplane-runtime. As @negz suggested it is probably good to start with a design proposal in https://github.com/crossplane/crossplane.

@chlunde
Copy link
Collaborator

chlunde commented Apr 12, 2024

Also discussed here:
#1625 (comment)

@MisterMX
Copy link
Collaborator

Closing this for now as the goal is rather to find a generic solution that works across all Crossplane providers.

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.

4 participants