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

Remove Extraneous Caching of Resource Schemas #168

Closed
bflad opened this issue Jun 26, 2023 · 1 comment · Fixed by #169
Closed

Remove Extraneous Caching of Resource Schemas #168

bflad opened this issue Jun 26, 2023 · 1 comment · Fixed by #169
Assignees
Labels
bug Something isn't working
Milestone

Comments

@bflad
Copy link
Contributor

bflad commented Jun 26, 2023

terraform-plugin-mux version

v0.10.0

Relevant provider source code

N/A

Terraform Configuration Files

N/A

Expected Behavior

terraform-plugin-mux should not cache resource schemas unless there is a really good reason as it inflates peak memory usage for providers. Currently all resource schemas are cached, which is problematic with larger providers (either in resource count and/or schema size). We are exploring whether Terraform itself can limit the amount of resource schema data being requested from providers to reduce overall memory utilization on both sides of the protocol. The resource schema caching in this Go module hinders any efforts in that regard though.

Actual Behavior

terraform-plugin-mux caches all resource schemas when creating a mux server. This caching is currently used with:

  • Pre-loading the GetProviderSchema RPC response based on all the underlying providers on mux server creation, which is itself because the mux server creation has historically implemented validation logic such as duplicate resource type detection
  • For protocol version 5 servers, verifies whether a PlanResourceChange RPC being sent to the underlying provider is a destroy plan, which is server capability based and the detection is based on fully decoding the ProposedNewState data using the actual schema

The pre-loaded GetProviderSchema response is unnecessary and can likely be wholly deferred until after server startup and the actual RPC is called. The mux server already has a separate, small cache map for duplicate checking and raising implementation error diagnostics. There may be additional protocol changes necessary if the protocol implements "limited" GetProviderSchema RPC requests though.

For the PlanResourceChange cache usage:

  • For protocol version 6, the caching is unnecessary and it can likely be removed
  • For protocol version 5, it may be possible to avoid the caching based on the server capability response of an underlying provider as a "quick" fix for some situations (mainly muxed framework providers), but that doesn't help terraform-plugin-sdk based providers which do not enable the server capability
  • It may be further possible to either enhance terraform-plugin-go or write internal logic which only peeks ahead into the PlanResourceChange JSON/msgpack data for nullity, rather than trying to fully unmarshal the data with the real schema.

Steps to Reproduce

  1. terraform validate / terraform plan / terraform apply

References

@bflad bflad added the bug Something isn't working label Jun 26, 2023
@bflad bflad added this to the v0.11.0 milestone Jun 26, 2023
@bflad bflad self-assigned this Jun 26, 2023
bflad added a commit that referenced this issue Jun 27, 2023
Reference: #168

This change removes the `GetProviderSchema` RPC-based caching logic in `NewMuxServer` and directly calls the underlying providers within that RPC. To fully support the removal of resource schema caching, the `PlanResourceChange` logic for checking the `ProposedNewState` data for a destroy plan was optimized to no longer require decoding the data with the type information.

For larger scale providers (either number of resources and/or resources with large schemas), this should reduce resident memory utilization with little performance penalty.
bflad added a commit that referenced this issue Jun 28, 2023
Reference: #168

This change removes the `GetProviderSchema` RPC-based caching logic in `NewMuxServer` and directly calls the underlying providers within that RPC. To fully support the removal of resource schema caching, the `PlanResourceChange` logic for checking the `ProposedNewState` data for a destroy plan was optimized to no longer require decoding the data with the type information.

For larger scale providers (either number of resources and/or resources with large schemas), this should reduce resident memory utilization with little performance penalty.
bflad added a commit that referenced this issue Jun 28, 2023
Reference: #168

This change removes the `GetProviderSchema` RPC-based caching logic in `NewMuxServer` and directly calls the underlying providers within that RPC. To fully support the removal of resource schema caching, the `PlanResourceChange` logic for checking the `ProposedNewState` data for a destroy plan was optimized to no longer require decoding the data with the type information.

For larger scale providers (either number of resources and/or resources with large schemas), this should reduce resident memory utilization with little performance penalty.
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant