From 8f377a1cb161c316388c6e9e071126f552ff566b Mon Sep 17 00:00:00 2001 From: Kuba Martin Date: Fri, 1 Dec 2023 17:41:18 +0100 Subject: [PATCH] Improve comments around the global and local provider schema cache. (#958) Signed-off-by: Jakub Martin --- internal/plugin/grpc_provider.go | 23 ++++++++++++++++++----- internal/plugin6/grpc_provider.go | 23 ++++++++++++++++++----- internal/tofu/context_plugins.go | 5 +++++ 3 files changed, 41 insertions(+), 10 deletions(-) diff --git a/internal/plugin/grpc_provider.go b/internal/plugin/grpc_provider.go index e7c58181de..0ce56000b9 100644 --- a/internal/plugin/grpc_provider.go +++ b/internal/plugin/grpc_provider.go @@ -79,8 +79,12 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp p.mu.Lock() defer p.mu.Unlock() - // check the global cache + // First, we check the global cache. + // The cache could contain this schema if an instance of this provider has previously been started. if !p.Addr.IsZero() { + // Even if the schema is cached, GetProviderSchemaOptional could be false. This would indicate that once instantiated, + // this provider requires the get schema call to be made at least once, as it handles part of the provider's setup. + // At this point, we don't know if this is the first call to a provider instance or not, so we don't use the result in that case. if schemaCached, ok := providers.SchemaCache.Get(p.Addr); ok && schemaCached.ServerCapabilities.GetProviderSchemaOptional { logger.Trace("GRPCProvider: GetProviderSchema: serving from global schema cache", "address", p.Addr) return schemaCached @@ -88,7 +92,8 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp } // If the local cache is non-zero, we know this instance has called - // GetProviderSchema at least once and we can return early. + // GetProviderSchema at least once, so has satisfied the possible requirement of `GetProviderSchemaOptional=false`. + // This means that we can return early now using the locally cached schema, without making this call again. if p.schema.Provider.Block != nil { return p.schema } @@ -142,13 +147,21 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp resp.ServerCapabilities.GetProviderSchemaOptional = protoResp.ServerCapabilities.GetProviderSchemaOptional } - // set the global cache if we can + // Set the global provider cache so that future calls to this provider can use the cached value. + // Crucially, this doesn't look at GetProviderSchemaOptional, because the layers above could use this cache + // *without* creating an instance of this provider. And if there is no instance, + // then we don't need to set up anything (cause there is nothing to set up), so we need no call + // to the providers GetSchema rpc. if !p.Addr.IsZero() { providers.SchemaCache.Set(p.Addr, resp) } - // always store this here in the client for providers that are not able to - // use GetProviderSchemaOptional + // Always store this here in the client for providers that are not able to use GetProviderSchemaOptional. + // Crucially, this indicates that we've made at least one call to GetProviderSchema to this instance of the provider, + // which means in the future we'll be able to return using this cache + // (because the possible setup contained in the GetProviderSchema call has happened). + // If GetProviderSchemaOptional is true then this cache won't actually ever be used, because the calls to this method + // will be satisfied by the global provider cache. p.schema = resp return resp diff --git a/internal/plugin6/grpc_provider.go b/internal/plugin6/grpc_provider.go index 29b2d49322..cac937f1bb 100644 --- a/internal/plugin6/grpc_provider.go +++ b/internal/plugin6/grpc_provider.go @@ -79,8 +79,12 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp p.mu.Lock() defer p.mu.Unlock() - // check the global cache + // First, we check the global cache. + // The cache could contain this schema if an instance of this provider has previously been started. if !p.Addr.IsZero() { + // Even if the schema is cached, GetProviderSchemaOptional could be false. This would indicate that once instantiated, + // this provider requires the get schema call to be made at least once, as it handles part of the provider's setup. + // At this point, we don't know if this is the first call to a provider instance or not, so we don't use the result in that case. if schemaCached, ok := providers.SchemaCache.Get(p.Addr); ok && schemaCached.ServerCapabilities.GetProviderSchemaOptional { logger.Trace("GRPCProvider: GetProviderSchema: serving from global schema cache", "address", p.Addr) return schemaCached @@ -88,7 +92,8 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp } // If the local cache is non-zero, we know this instance has called - // GetProviderSchema at least once and we can return early. + // GetProviderSchema at least once, so has satisfied the possible requirement of `GetProviderSchemaOptional=false`. + // This means that we can return early now using the locally cached schema, without making this call again. if p.schema.Provider.Block != nil { return p.schema } @@ -142,13 +147,21 @@ func (p *GRPCProvider) GetProviderSchema() (resp providers.GetProviderSchemaResp resp.ServerCapabilities.GetProviderSchemaOptional = protoResp.ServerCapabilities.GetProviderSchemaOptional } - // set the global cache if we can + // Set the global provider cache so that future calls to this provider can use the cached value. + // Crucially, this doesn't look at GetProviderSchemaOptional, because the layers above could use this cache + // *without* creating an instance of this provider. And if there is no instance, + // then we don't need to set up anything (cause there is nothing to set up), so we need no call + // to the providers GetSchema rpc. if !p.Addr.IsZero() { providers.SchemaCache.Set(p.Addr, resp) } - // always store this here in the client for providers that are not able to - // use GetProviderSchemaOptional + // Always store this here in the client for providers that are not able to use GetProviderSchemaOptional. + // Crucially, this indicates that we've made at least one call to GetProviderSchema to this instance of the provider, + // which means in the future we'll be able to return using this cache + // (because the possible setup contained in the GetProviderSchema call has happened). + // If GetProviderSchemaOptional is true then this cache won't actually ever be used, because the calls to this method + // will be satisfied by the global provider cache. p.schema = resp return resp diff --git a/internal/tofu/context_plugins.go b/internal/tofu/context_plugins.go index ae79002d55..b188f64e5c 100644 --- a/internal/tofu/context_plugins.go +++ b/internal/tofu/context_plugins.go @@ -70,6 +70,11 @@ func (cp *contextPlugins) ProviderSchema(addr addrs.Provider) (providers.Provide // This cache is only written by the provider client, and transparently // used by GetProviderSchema, but we check it here because at this point we // may be able to avoid spinning up the provider instance at all. + // + // It's worth noting that ServerCapabilities.GetProviderSchemaOptional is ignored here. + // That is because we're checking *prior* to the provider's instantiation. + // GetProviderSchemaOptional only says that *if we instantiate a provider*, + // then we need to run the get schema call at least once. schemas, ok := providers.SchemaCache.Get(addr) if ok { log.Printf("[TRACE] tofu.contextPlugins: Serving provider %q schema from global schema cache", addr)