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

Consume upjet ProviderScheduler #627

Merged
merged 1 commit into from
Mar 27, 2023
Merged

Conversation

ulucinar
Copy link
Collaborator

Description of your changes

Associated with: #325

This PR reenables the shared gRPC server runtime previously disabled due to external resource leakage issues. It consumes the new provider scheduler from upjet and:

  • Enables the shared server runtime
  • Adds --provider-ttl command-line option through which the max TTL used by the shared scheduler can be configured.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

This PR has been tested in the experiments done here:
#576

@ulucinar
Copy link
Collaborator Author

/test-examples="examples/ec2/vpc.yaml"

Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar I left a comment.

@@ -51,6 +51,7 @@ const (
errAWSConfigUpbound = "failed to get AWS config using Upbound identity"

upboundProviderIdentityTokenFile = "/var/run/secrets/upbound.io/provider/token"
defaultIdentityTokenFile = "/var/run/secrets/eks.amazonaws.com/serviceaccount/token"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that we do not use this constant. Could you please remove it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Thanks for catching it.

@sergenyalcin
Copy link
Collaborator

Some test results that are done via this branch content:

#576 (comment)
#576 (comment)
#576 (comment)

- Enables the shared server runtime
- Adds --provider-ttl command-line option

Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
@ulucinar
Copy link
Collaborator Author

/test-examples="examples/ec2/vpc.yaml"

Copy link
Collaborator

@sergenyalcin sergenyalcin left a comment

Choose a reason for hiding this comment

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

Thanks @ulucinar LGTM!

@ulucinar
Copy link
Collaborator Author

make lint runs successfully locally, merging the PR:
image

@ulucinar ulucinar merged commit 08ed95f into crossplane-contrib:main Mar 27, 2023
@ulucinar ulucinar deleted the fix-325 branch March 27, 2023 18:51
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.

None yet

2 participants