From 1f941bbec73ea22487db0c8ed28d583e9c5d61b9 Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 14 Oct 2024 08:05:47 +0200 Subject: [PATCH 1/2] fix(server/v2): use only one logger for app and server (#22241) --- server/v2/commands.go | 8 ++--- server/v2/server.go | 9 +++--- server/v2/server_test.go | 7 +++-- server/v2/util.go | 52 +++++++++++++++++++++----------- simapp/v2/simdv2/cmd/commands.go | 8 +---- simapp/v2/simdv2/cmd/config.go | 2 +- simapp/v2/simdv2/cmd/testnet.go | 3 +- 7 files changed, 50 insertions(+), 39 deletions(-) diff --git a/server/v2/commands.go b/server/v2/commands.go index 0357fe180d93..86f8e400467c 100644 --- a/server/v2/commands.go +++ b/server/v2/commands.go @@ -13,7 +13,6 @@ import ( "github.com/spf13/viper" "cosmossdk.io/core/transaction" - "cosmossdk.io/log" ) // Execute executes the root command of an application. @@ -37,7 +36,6 @@ func Execute(rootCmd *cobra.Command, envPrefix, defaultHome string) error { func AddCommands[T transaction.Tx]( rootCmd *cobra.Command, newApp AppCreator[T], - logger log.Logger, globalServerCfg ServerConfig, components ...ServerComponent[T], ) error { @@ -45,7 +43,7 @@ func AddCommands[T transaction.Tx]( return errors.New("no components provided") } - server := NewServer(logger, globalServerCfg, components...) + server := NewServer(globalServerCfg, components...) originalPersistentPreRunE := rootCmd.PersistentPreRunE rootCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error { // set the default command outputs @@ -170,12 +168,12 @@ func configHandle[T transaction.Tx](s *Server[T], cmd *cobra.Command) error { return err } - log, err := NewLogger(v, cmd.OutOrStdout()) + logger, err := NewLogger(v, cmd.OutOrStdout()) if err != nil { return err } - return SetCmdServerContext(cmd, v, log) + return SetCmdServerContext(cmd, v, logger) } // findSubCommand finds a sub-command of the provided command whose Use diff --git a/server/v2/server.go b/server/v2/server.go index a628fbdf1000..486f5c0ceecb 100644 --- a/server/v2/server.go +++ b/server/v2/server.go @@ -63,18 +63,15 @@ var _ ServerComponent[transaction.Tx] = (*Server[transaction.Tx])(nil) // Server is the top-level server component which contains all other server components. type Server[T transaction.Tx] struct { - logger log.Logger components []ServerComponent[T] config ServerConfig } func NewServer[T transaction.Tx]( - logger log.Logger, config ServerConfig, components ...ServerComponent[T], ) *Server[T] { return &Server[T]{ - logger: logger, config: config, components: components, } @@ -86,7 +83,8 @@ func (s *Server[T]) Name() string { // Start starts all components concurrently. func (s *Server[T]) Start(ctx context.Context) error { - s.logger.Info("starting servers...") + logger := GetLoggerFromContext(ctx) + logger.With(log.ModuleKey, s.Name()).Info("starting servers...") g, ctx := errgroup.WithContext(ctx) for _, mod := range s.components { @@ -106,7 +104,8 @@ func (s *Server[T]) Start(ctx context.Context) error { // Stop stops all components concurrently. func (s *Server[T]) Stop(ctx context.Context) error { - s.logger.Info("stopping servers...") + logger := GetLoggerFromContext(ctx) + logger.With(log.ModuleKey, s.Name()).Info("stopping servers...") g, ctx := errgroup.WithContext(ctx) for _, mod := range s.components { diff --git a/server/v2/server_test.go b/server/v2/server_test.go index b67fd209e1bd..c35dea1fc627 100644 --- a/server/v2/server_test.go +++ b/server/v2/server_test.go @@ -65,6 +65,10 @@ func TestServer(t *testing.T) { cfg := v.AllSettings() logger := log.NewLogger(os.Stdout) + + ctx, err := serverv2.SetServerContext(context.Background(), v, logger) + require.NoError(t, err) + grpcServer := grpc.New[transaction.Tx]() err = grpcServer.Init(&mockApp[transaction.Tx]{}, cfg, logger) require.NoError(t, err) @@ -76,7 +80,6 @@ func TestServer(t *testing.T) { mockServer := &mockServer{name: "mock-server-1", ch: make(chan string, 100)} server := serverv2.NewServer( - logger, serverv2.DefaultServerConfig(), grpcServer, storeServer, @@ -97,7 +100,7 @@ func TestServer(t *testing.T) { require.Equal(t, v.GetString(grpcServer.Name()+".address"), grpc.DefaultConfig().Address) // start empty - ctx, cancelFn := context.WithCancel(context.TODO()) + ctx, cancelFn := context.WithCancel(ctx) go func() { // wait 5sec and cancel context <-time.After(5 * time.Second) diff --git a/server/v2/util.go b/server/v2/util.go index 72335621b44b..d02ea30125e5 100644 --- a/server/v2/util.go +++ b/server/v2/util.go @@ -13,44 +13,62 @@ import ( "cosmossdk.io/log" ) +// SetServerContext sets the logger and viper in the context. +// The server manager expects the logger and viper to be set in the context. +func SetServerContext(ctx context.Context, viper *viper.Viper, logger log.Logger) (context.Context, error) { + if ctx == nil { + ctx = context.Background() + } + + ctx = context.WithValue(ctx, corectx.LoggerContextKey, logger) + ctx = context.WithValue(ctx, corectx.ViperContextKey, viper) + return ctx, nil +} + // SetCmdServerContext sets a command's Context value to the provided argument. +// The server manager expects the logger and viper to be set in the context. // If the context has not been set, set the given context as the default. func SetCmdServerContext(cmd *cobra.Command, viper *viper.Viper, logger log.Logger) error { - var cmdCtx context.Context - if cmd.Context() == nil { - cmdCtx = context.Background() - } else { - cmdCtx = cmd.Context() + ctx, err := SetServerContext(cmd.Context(), viper, logger) + if err != nil { + return err } - - cmdCtx = context.WithValue(cmdCtx, corectx.LoggerContextKey, logger) - cmdCtx = context.WithValue(cmdCtx, corectx.ViperContextKey, viper) - cmd.SetContext(cmdCtx) - + cmd.SetContext(ctx) return nil } -func GetViperFromCmd(cmd *cobra.Command) *viper.Viper { - value := cmd.Context().Value(corectx.ViperContextKey) +// GetViperFromContext returns the viper instance from the context. +func GetViperFromContext(ctx context.Context) *viper.Viper { + value := ctx.Value(corectx.ViperContextKey) v, ok := value.(*viper.Viper) if !ok { - panic(fmt.Sprintf("incorrect viper type %T: expected *viper.Viper. Have you forgot to set the viper in the command context?", value)) + panic(fmt.Sprintf("incorrect viper type %T: expected *viper.Viper. Have you forgot to set the viper in the context?", value)) } return v } -func GetLoggerFromCmd(cmd *cobra.Command) log.Logger { - v := cmd.Context().Value(corectx.LoggerContextKey) +// GetViperFromCmd returns the viper instance from the command context. +func GetViperFromCmd(cmd *cobra.Command) *viper.Viper { + return GetViperFromContext(cmd.Context()) +} + +// GetLoggerFromContext returns the logger instance from the context. +func GetLoggerFromContext(ctx context.Context) log.Logger { + v := ctx.Value(corectx.LoggerContextKey) logger, ok := v.(log.Logger) if !ok { - panic(fmt.Sprintf("incorrect logger type %T: expected log.Logger. Have you forgot to set the logger in the command context?", v)) + panic(fmt.Sprintf("incorrect logger type %T: expected log.Logger. Have you forgot to set the logger in the context?", v)) } return logger } +// GetLoggerFromCmd returns the logger instance from the command context. +func GetLoggerFromCmd(cmd *cobra.Command) log.Logger { + return GetLoggerFromContext(cmd.Context()) +} + // ExternalIP https://stackoverflow.com/questions/23558425/how-do-i-get-the-local-ip-address-in-go -// TODO there must be a better way to get external IP func ExternalIP() (string, error) { ifaces, err := net.Interfaces() if err != nil { diff --git a/simapp/v2/simdv2/cmd/commands.go b/simapp/v2/simdv2/cmd/commands.go index 3fcbed114d9e..8d4186be8d19 100644 --- a/simapp/v2/simdv2/cmd/commands.go +++ b/simapp/v2/simdv2/cmd/commands.go @@ -55,11 +55,6 @@ func initRootCmd[T transaction.Tx]( NewTestnetCmd(moduleManager), ) - logger, err := serverv2.NewLogger(viper.New(), rootCmd.OutOrStdout()) - if err != nil { - panic(fmt.Sprintf("failed to create logger: %v", err)) - } - // add keybase, auxiliary RPC, query, genesis, and tx child commands rootCmd.AddCommand( genesisCommand(moduleManager), @@ -70,10 +65,9 @@ func initRootCmd[T transaction.Tx]( ) // wire server commands - if err = serverv2.AddCommands( + if err := serverv2.AddCommands( rootCmd, newApp, - logger, initServerConfig(), cometbft.New( &genericTxDecoder[T]{txConfig}, diff --git a/simapp/v2/simdv2/cmd/config.go b/simapp/v2/simdv2/cmd/config.go index 51a7adb178e9..16cb19db93e7 100644 --- a/simapp/v2/simdv2/cmd/config.go +++ b/simapp/v2/simdv2/cmd/config.go @@ -79,7 +79,7 @@ func initCometConfig() cometbft.CfgOption { cfg := cmtcfg.DefaultConfig() // display only warn logs by default except for p2p and state - cfg.LogLevel = "*:warn,p2p:info,state:info" + cfg.LogLevel = "*:warn,server:info,p2p:info,state:info" // increase block timeout cfg.Consensus.TimeoutCommit = 5 * time.Second // overwrite default pprof listen address diff --git a/simapp/v2/simdv2/cmd/testnet.go b/simapp/v2/simdv2/cmd/testnet.go index f030db8e5956..7b71e98b588d 100644 --- a/simapp/v2/simdv2/cmd/testnet.go +++ b/simapp/v2/simdv2/cmd/testnet.go @@ -15,7 +15,6 @@ import ( "github.com/spf13/pflag" "cosmossdk.io/core/transaction" - "cosmossdk.io/log" "cosmossdk.io/math" "cosmossdk.io/math/unsafe" runtimev2 "cosmossdk.io/runtime/v2" @@ -344,7 +343,7 @@ func initTestnetFiles[T transaction.Tx]( ) storeServer := store.New[T]() grpcServer := grpc.New[T](grpc.OverwriteDefaultConfig(grpcConfig)) - server := serverv2.NewServer[T](log.NewNopLogger(), serverCfg, cometServer, grpcServer, storeServer) + server := serverv2.NewServer[T](serverCfg, cometServer, grpcServer, storeServer) err = server.WriteConfig(filepath.Join(nodeDir, "config")) if err != nil { return err From 0b43fcc2164c34b67c9bd0b5795a9c84afb6d16c Mon Sep 17 00:00:00 2001 From: Julien Robert Date: Mon, 14 Oct 2024 08:07:34 +0200 Subject: [PATCH 2/2] fix(server/v2/cometbft): wire app closer (#22240) --- server/v2/cometbft/abci.go | 3 +++ server/v2/cometbft/abci_test.go | 6 +++--- server/v2/cometbft/server.go | 1 + server/v2/testdata/app.toml | 2 +- server/v2/types.go | 1 + simapp/v2/app_di.go | 10 ++++++++++ store/v2/commitment/metadata.go | 1 + store/v2/root/factory.go | 4 ++-- store/v2/root/migrate_test.go | 2 +- store/v2/root/store.go | 7 +++++++ store/v2/root/store_test.go | 6 +++--- store/v2/root/upgrade_test.go | 4 ++-- tools/confix/data/v2-app.toml | 2 +- 13 files changed, 36 insertions(+), 13 deletions(-) diff --git a/server/v2/cometbft/abci.go b/server/v2/cometbft/abci.go index 95eeec9bb5da..75618e296022 100644 --- a/server/v2/cometbft/abci.go +++ b/server/v2/cometbft/abci.go @@ -43,6 +43,7 @@ type Consensus[T transaction.Tx] struct { logger log.Logger appName, version string app *appmanager.AppManager[T] + appCloser func() error txCodec transaction.Codec[T] store types.Store streaming streaming.Manager @@ -77,6 +78,7 @@ func NewConsensus[T transaction.Tx]( logger log.Logger, appName string, app *appmanager.AppManager[T], + appCloser func() error, mp mempool.Mempool[T], indexedEvents map[string]struct{}, queryHandlersMap map[string]appmodulev2.Handler, @@ -89,6 +91,7 @@ func NewConsensus[T transaction.Tx]( appName: appName, version: getCometBFTServerVersion(), app: app, + appCloser: appCloser, cfg: cfg, store: store, logger: logger, diff --git a/server/v2/cometbft/abci_test.go b/server/v2/cometbft/abci_test.go index 3af3fbec8f29..9a42948c6c61 100644 --- a/server/v2/cometbft/abci_test.go +++ b/server/v2/cometbft/abci_test.go @@ -19,7 +19,7 @@ import ( "cosmossdk.io/core/store" "cosmossdk.io/core/transaction" "cosmossdk.io/log" - am "cosmossdk.io/server/v2/appmanager" + "cosmossdk.io/server/v2/appmanager" "cosmossdk.io/server/v2/cometbft/handlers" cometmock "cosmossdk.io/server/v2/cometbft/internal/mock" "cosmossdk.io/server/v2/cometbft/mempool" @@ -672,7 +672,7 @@ func setUpConsensus(t *testing.T, gasLimit uint64, mempool mempool.Mempool[mock. sc := cometmock.NewMockCommiter(log.NewNopLogger(), string(actorName), "stf") mockStore := cometmock.NewMockStore(ss, sc) - b := am.Builder[mock.Tx]{ + b := appmanager.Builder[mock.Tx]{ STF: s, DB: mockStore, ValidateTxGasLimit: gasLimit, @@ -688,7 +688,7 @@ func setUpConsensus(t *testing.T, gasLimit uint64, mempool mempool.Mempool[mock. am, err := b.Build() require.NoError(t, err) - return NewConsensus[mock.Tx](log.NewNopLogger(), "testing-app", am, mempool, map[string]struct{}{}, nil, mockStore, Config{AppTomlConfig: DefaultAppTomlConfig()}, mock.TxCodec{}, "test") + return NewConsensus[mock.Tx](log.NewNopLogger(), "testing-app", am, func() error { return nil }, mempool, map[string]struct{}{}, nil, mockStore, Config{AppTomlConfig: DefaultAppTomlConfig()}, mock.TxCodec{}, "test") } // Check target version same with store's latest version diff --git a/server/v2/cometbft/server.go b/server/v2/cometbft/server.go index 8bd2935149e7..20cd63d07ca4 100644 --- a/server/v2/cometbft/server.go +++ b/server/v2/cometbft/server.go @@ -107,6 +107,7 @@ func (s *CometBFTServer[T]) Init(appI serverv2.AppI[T], cfg map[string]any, logg s.logger, appI.Name(), appI.GetAppManager(), + appI.Close, s.serverOptions.Mempool(cfg), indexEvents, appI.GetQueryHandlers(), diff --git a/server/v2/testdata/app.toml b/server/v2/testdata/app.toml index 094ac068d94a..3d32be7e57ef 100644 --- a/server/v2/testdata/app.toml +++ b/server/v2/testdata/app.toml @@ -25,7 +25,7 @@ minimum-gas-prices = '0stake' app-db-backend = 'goleveldb' [store.options] -# SState storage database type. Currently we support: "sqlite", "pebble" and "rocksdb" +# State storage database type. Currently we support: "sqlite", "pebble" and "rocksdb" ss-type = 'sqlite' # State commitment database type. Currently we support: "iavl" and "iavl-v2" sc-type = 'iavl' diff --git a/server/v2/types.go b/server/v2/types.go index e628fb30c90f..5406313b080e 100644 --- a/server/v2/types.go +++ b/server/v2/types.go @@ -21,4 +21,5 @@ type AppI[T transaction.Tx] interface { GetQueryHandlers() map[string]appmodulev2.Handler GetStore() store.RootStore GetSchemaDecoderResolver() decoding.DecoderResolver + Close() error } diff --git a/simapp/v2/app_di.go b/simapp/v2/app_di.go index b1dfc26f99ff..3c9933721f12 100644 --- a/simapp/v2/app_di.go +++ b/simapp/v2/app_di.go @@ -218,6 +218,16 @@ func (app *SimApp[T]) TxConfig() client.TxConfig { return app.txConfig } +// GetStore returns the root store. func (app *SimApp[T]) GetStore() store.RootStore { return app.store } + +// Close overwrites the base Close method to close the stores. +func (app *SimApp[T]) Close() error { + if err := app.store.Close(); err != nil { + return err + } + + return app.App.Close() +} diff --git a/store/v2/commitment/metadata.go b/store/v2/commitment/metadata.go index 642e6b630dac..a054acf26e89 100644 --- a/store/v2/commitment/metadata.go +++ b/store/v2/commitment/metadata.go @@ -17,6 +17,7 @@ const ( ) // MetadataStore is a store for metadata related to the commitment store. +// It isn't metadata store role to close the underlying KVStore. type MetadataStore struct { kv corestore.KVStoreWithBatch } diff --git a/store/v2/root/factory.go b/store/v2/root/factory.go index 9e1f76a36ceb..2511a53b434e 100644 --- a/store/v2/root/factory.go +++ b/store/v2/root/factory.go @@ -35,7 +35,7 @@ const ( // Options are the options for creating a root store. type Options struct { - SSType SSType `mapstructure:"ss-type" toml:"ss-type" comment:"SState storage database type. Currently we support: \"sqlite\", \"pebble\" and \"rocksdb\""` + SSType SSType `mapstructure:"ss-type" toml:"ss-type" comment:"State storage database type. Currently we support: \"sqlite\", \"pebble\" and \"rocksdb\""` SCType SCType `mapstructure:"sc-type" toml:"sc-type" comment:"State commitment database type. Currently we support: \"iavl\" and \"iavl-v2\""` SSPruningOption *store.PruningOption `mapstructure:"ss-pruning-option" toml:"ss-pruning-option" comment:"Pruning options for state storage"` SCPruningOption *store.PruningOption `mapstructure:"sc-pruning-option" toml:"sc-pruning-option" comment:"Pruning options for state commitment"` @@ -177,5 +177,5 @@ func CreateRootStore(opts *FactoryOptions) (store.RootStore, error) { } pm := pruning.NewManager(sc, ss, storeOpts.SCPruningOption, storeOpts.SSPruningOption) - return New(opts.Logger, ss, sc, pm, nil, nil) + return New(opts.SCRawDB, opts.Logger, ss, sc, pm, nil, nil) } diff --git a/store/v2/root/migrate_test.go b/store/v2/root/migrate_test.go index 0cc20ae940d9..9bd8dfdd2ef8 100644 --- a/store/v2/root/migrate_test.go +++ b/store/v2/root/migrate_test.go @@ -80,7 +80,7 @@ func (s *MigrateStoreTestSuite) SetupTest() { pm := pruning.NewManager(sc, ss, nil, nil) // assume no storage store, simulate the migration process - s.rootStore, err = New(testLog, ss, orgSC, pm, migrationManager, nil) + s.rootStore, err = New(dbm.NewMemDB(), testLog, ss, orgSC, pm, migrationManager, nil) s.Require().NoError(err) } diff --git a/store/v2/root/store.go b/store/v2/root/store.go index c39119a9aaf3..7447c60ae7b2 100644 --- a/store/v2/root/store.go +++ b/store/v2/root/store.go @@ -5,6 +5,7 @@ import ( "crypto/sha256" "errors" "fmt" + "io" "sync" "time" @@ -33,6 +34,9 @@ type Store struct { logger corelog.Logger initialVersion uint64 + // holds the db instance for closing it + dbCloser io.Closer + // stateStorage reflects the state storage backend stateStorage store.VersionedDatabase @@ -68,6 +72,7 @@ type Store struct { // // NOTE: The migration manager is optional and can be nil if no migration is required. func New( + dbCloser io.Closer, logger corelog.Logger, ss store.VersionedDatabase, sc store.Committer, @@ -76,6 +81,7 @@ func New( m metrics.StoreMetrics, ) (store.RootStore, error) { return &Store{ + dbCloser: dbCloser, logger: logger, initialVersion: 1, stateStorage: ss, @@ -92,6 +98,7 @@ func New( func (s *Store) Close() (err error) { err = errors.Join(err, s.stateStorage.Close()) err = errors.Join(err, s.stateCommitment.Close()) + err = errors.Join(err, s.dbCloser.Close()) s.stateStorage = nil s.stateCommitment = nil diff --git a/store/v2/root/store_test.go b/store/v2/root/store_test.go index e8cf34e014e0..9f925b098b9c 100644 --- a/store/v2/root/store_test.go +++ b/store/v2/root/store_test.go @@ -59,7 +59,7 @@ func (s *RootStoreTestSuite) SetupTest() { s.Require().NoError(err) pm := pruning.NewManager(sc, ss, nil, nil) - rs, err := New(noopLog, ss, sc, pm, nil, nil) + rs, err := New(dbm.NewMemDB(), noopLog, ss, sc, pm, nil, nil) s.Require().NoError(err) s.rootStore = rs @@ -84,7 +84,7 @@ func (s *RootStoreTestSuite) newStoreWithPruneConfig(config *store.PruningOption pm := pruning.NewManager(sc, ss, config, config) - rs, err := New(noopLog, ss, sc, pm, nil, nil) + rs, err := New(dbm.NewMemDB(), noopLog, ss, sc, pm, nil, nil) s.Require().NoError(err) s.rootStore = rs @@ -93,7 +93,7 @@ func (s *RootStoreTestSuite) newStoreWithPruneConfig(config *store.PruningOption func (s *RootStoreTestSuite) newStoreWithBackendMount(ss store.VersionedDatabase, sc store.Committer, pm *pruning.Manager) { noopLog := coretesting.NewNopLogger() - rs, err := New(noopLog, ss, sc, pm, nil, nil) + rs, err := New(dbm.NewMemDB(), noopLog, ss, sc, pm, nil, nil) s.Require().NoError(err) s.rootStore = rs diff --git a/store/v2/root/upgrade_test.go b/store/v2/root/upgrade_test.go index a4aee1d5bcfa..47c2882dc2ef 100644 --- a/store/v2/root/upgrade_test.go +++ b/store/v2/root/upgrade_test.go @@ -50,7 +50,7 @@ func (s *UpgradeStoreTestSuite) SetupTest() { sc, err := commitment.NewCommitStore(multiTrees, nil, s.commitDB, testLog) s.Require().NoError(err) pm := pruning.NewManager(sc, ss, nil, nil) - s.rootStore, err = New(testLog, ss, sc, pm, nil, nil) + s.rootStore, err = New(s.commitDB, testLog, ss, sc, pm, nil, nil) s.Require().NoError(err) // commit changeset @@ -92,7 +92,7 @@ func (s *UpgradeStoreTestSuite) loadWithUpgrades(upgrades *corestore.StoreUpgrad sc, err := commitment.NewCommitStore(multiTrees, oldTrees, s.commitDB, testLog) s.Require().NoError(err) pm := pruning.NewManager(sc, s.rootStore.GetStateStorage().(store.Pruner), nil, nil) - s.rootStore, err = New(testLog, s.rootStore.GetStateStorage(), sc, pm, nil, nil) + s.rootStore, err = New(s.commitDB, testLog, s.rootStore.GetStateStorage(), sc, pm, nil, nil) s.Require().NoError(err) } diff --git a/tools/confix/data/v2-app.toml b/tools/confix/data/v2-app.toml index ef144a0b90dd..8a35d32f543b 100644 --- a/tools/confix/data/v2-app.toml +++ b/tools/confix/data/v2-app.toml @@ -50,7 +50,7 @@ minimum-gas-prices = '0stake' app-db-backend = 'goleveldb' [store.options] -# SState storage database type. Currently we support: "sqlite", "pebble" and "rocksdb" +# State storage database type. Currently we support: "sqlite", "pebble" and "rocksdb" ss-type = 'sqlite' # State commitment database type. Currently we support: "iavl" and "iavl-v2" sc-type = 'iavl'