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

Call configure in sequence (instead of in parallel) #2738

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Dec 12, 2024

It looks like GCP has introduced a dependency on configuring SDK and then PF into their provider. This commit is being used to check if ordering will fix the panics that the GCP upgrade is seeing in tests.

pulumi/pulumi-gcp#2733 shows that this is a necessary change.

Copy link

codecov bot commented Dec 12, 2024

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 69.64%. Comparing base (7b3ace6) to head (0f94385).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
pkg/x/muxer/muxer.go 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2738   +/-   ##
=======================================
  Coverage   69.63%   69.64%           
=======================================
  Files         301      301           
  Lines       38725    38721    -4     
=======================================
- Hits        26967    26966    -1     
+ Misses      10240    10238    -2     
+ Partials     1518     1517    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@iwahbe iwahbe force-pushed the iwahbe/sync-configure branch from af6d06d to 19c794d Compare December 12, 2024 13:39
@iwahbe iwahbe self-assigned this Dec 12, 2024
@iwahbe iwahbe marked this pull request as ready for review December 12, 2024 14:54
@iwahbe iwahbe requested a review from a team December 12, 2024 14:54
@iwahbe iwahbe changed the title Call configure in sequence (not in parallel) Call configure in sequence (instead of in parallel) Dec 12, 2024
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

How does this impact AWS? I remember there were some performance issues with Configure there. Very fuzzy on the details but I believe one of the Configure calls might hang sometimes

@iwahbe iwahbe force-pushed the iwahbe/sync-configure branch from 71d3e6f to 0f94385 Compare December 12, 2024 15:12
@iwahbe
Copy link
Member Author

iwahbe commented Dec 12, 2024

I believe that AWS solved that by not verifying twice. This will result in a Configure call no slower then TF. If Configure hung, then it would have hung before this change as well.

@iwahbe
Copy link
Member Author

iwahbe commented Dec 12, 2024

This will be marginally slower, but it should still be pretty fast. AWS had major issues with start-up time, but this doesn't effect that at all.

@iwahbe
Copy link
Member Author

iwahbe commented Dec 12, 2024

@t0yv0 Might have a better memory for AWS's performance issues.

@iwahbe iwahbe requested review from VenelinMartinov and a team December 12, 2024 18:17
@t0yv0
Copy link
Member

t0yv0 commented Dec 12, 2024

Yeah we might pay few more milliseconds on startup for this in AWS. I guess the behavior here could be opt-in so only GCP opts in? Or do you think it's generally the right thing to do for most providers?

I suspect in the case of AWS what we might actually want is removing the PF Configure call entirely or making it a no-op in AWS. I think it might still work if both PF and SDKv2 providers in the AWS internals are wrapping the same state bag for the configuration information.

@t0yv0
Copy link
Member

t0yv0 commented Dec 12, 2024

I think we need to own gating CI on pulumi-aws wrt the performance issues, so we just come back to this convo as needed when breaching thresholds. I'm taking a quick look.

@iwahbe iwahbe merged commit b9a6a39 into master Dec 13, 2024
17 checks passed
@iwahbe iwahbe deleted the iwahbe/sync-configure branch December 13, 2024 09:31
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.98.0.

guineveresaenger pushed a commit to pulumi/pulumi-gcp that referenced this pull request Dec 16, 2024
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