From f15f93f4794a7ca6b7075aa3b1cecf66b4319f54 Mon Sep 17 00:00:00 2001 From: Oleksii Kliukin Date: Thu, 10 Aug 2017 10:10:00 +0200 Subject: [PATCH] Bugfix/close db connections (#78) Open and close DB connections on-demand. Previously, we used to leave the DB connection open while the cluster was registered with the operator, potentially resutling in dangled connections if the operator terminates abnormally. Small refactoring around the role syncing code. --- pkg/cluster/cluster.go | 9 +++---- pkg/cluster/pg.go | 10 +++++++ pkg/cluster/resources.go | 7 ++--- pkg/cluster/sync.go | 57 +++++++++++++++++++++------------------- 4 files changed, 45 insertions(+), 38 deletions(-) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index 6fc0d1654..d33aaeac4 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -143,6 +143,7 @@ func (c *Cluster) setStatus(status spec.PostgresStatus) { } } +// initUsers populates c.systemUsers and c.pgUsers maps. func (c *Cluster) initUsers() error { c.initSystemUsers() @@ -204,7 +205,7 @@ func (c *Cluster) Create() error { if err = c.initUsers(); err != nil { return err } - c.logger.Infof("User secrets have been initialized") + c.logger.Infof("Users have been initialized") if err = c.applySecrets(); err != nil { return fmt.Errorf("could not create secrets: %v", err) @@ -226,11 +227,7 @@ func (c *Cluster) Create() error { c.logger.Infof("pods are ready") if !(c.masterLess || c.databaseAccessDisabled()) { - err = c.initDbConn() - if err != nil { - return fmt.Errorf("could not init db connection: %v", err) - } - err = c.createUsers() + err = c.createRoles() if err != nil { return fmt.Errorf("could not create users: %v", err) } diff --git a/pkg/cluster/pg.go b/pkg/cluster/pg.go index 7664a0ed9..e5712952e 100644 --- a/pkg/cluster/pg.go +++ b/pkg/cluster/pg.go @@ -46,6 +46,7 @@ func (c *Cluster) initDbConn() (err error) { if err != nil { return err } + c.logger.Debug("new database connection") err = conn.Ping() if err != nil { if err2 := conn.Close(); err2 != nil { @@ -60,6 +61,15 @@ func (c *Cluster) initDbConn() (err error) { return nil } +func (c *Cluster) closeDbConn() (err error) { + if c.pgDb != nil { + c.logger.Debug("closing database connection") + return c.pgDb.Close() + } + c.logger.Warning("attempted to close an empty db connection object") + return nil +} + func (c *Cluster) readPgUsersFromDatabase(userNames []string) (users spec.PgUserMap, err error) { var rows *sql.Rows users = make(spec.PgUserMap) diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go index e42ae94f0..a3b7b00a5 100644 --- a/pkg/cluster/resources.go +++ b/pkg/cluster/resources.go @@ -415,10 +415,7 @@ func (c *Cluster) deleteSecret(secret *v1.Secret) error { return err } -func (c *Cluster) createUsers() (err error) { +func (c *Cluster) createRoles() (err error) { // TODO: figure out what to do with duplicate names (humans and robots) among pgUsers - reqs := c.userSyncStrategy.ProduceSyncRequests(nil, c.pgUsers) - err = c.userSyncStrategy.ExecuteSyncRequests(reqs, c.pgDb) - - return err + return c.syncRoles(false) } diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index 409a55934..973889d7a 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -3,6 +3,7 @@ package cluster import ( "fmt" + "github.com/zalando-incubator/postgres-operator/pkg/spec" "github.com/zalando-incubator/postgres-operator/pkg/util" "github.com/zalando-incubator/postgres-operator/pkg/util/k8sutil" "github.com/zalando-incubator/postgres-operator/pkg/util/volumes" @@ -19,8 +20,14 @@ func (c *Cluster) Sync() error { c.logger.Errorf("could not load resources: %v", err) } + if err = c.initUsers(); err != nil { + return err + } + c.logger.Debugf("Syncing secrets") - if err := c.syncSecrets(); err != nil { + + //TODO: mind the secrets of the deleted/new users + if err := c.applySecrets(); err != nil { if !k8sutil.ResourceAlreadyExists(err) { return fmt.Errorf("could not sync secrets: %v", err) } @@ -59,11 +66,8 @@ func (c *Cluster) Sync() error { } if !c.databaseAccessDisabled() { - if err := c.initDbConn(); err != nil { - return fmt.Errorf("could not init db connection: %v", err) - } c.logger.Debugf("Syncing roles") - if err := c.syncRoles(); err != nil { + if err := c.syncRoles(true); err != nil { return fmt.Errorf("could not sync roles: %v", err) } } @@ -76,17 +80,6 @@ func (c *Cluster) Sync() error { return nil } -func (c *Cluster) syncSecrets() error { - //TODO: mind the secrets of the deleted/new users - if err := c.initUsers(); err != nil { - return err - } - - err := c.applySecrets() - - return err -} - func (c *Cluster) syncService(role postgresRole) error { cSpec := c.Spec if c.Service[role] == nil { @@ -193,21 +186,31 @@ func (c *Cluster) syncStatefulSet() error { return nil } -func (c *Cluster) syncRoles() error { - var userNames []string +func (c *Cluster) syncRoles(readFromDatabase bool) error { + var ( + err error + dbUsers spec.PgUserMap + userNames []string + ) - if err := c.initUsers(); err != nil { - return err - } - for _, u := range c.pgUsers { - userNames = append(userNames, u.Name) - } - dbUsers, err := c.readPgUsersFromDatabase(userNames) + err = c.initDbConn() if err != nil { - return fmt.Errorf("error getting users from the database: %v", err) + return fmt.Errorf("could not init db connection: %v", err) } + defer c.closeDbConn() + + if readFromDatabase { + for _, u := range c.pgUsers { + userNames = append(userNames, u.Name) + } + dbUsers, err = c.readPgUsersFromDatabase(userNames) + if err != nil { + return fmt.Errorf("error getting users from the database: %v", err) + } + } + pgSyncRequests := c.userSyncStrategy.ProduceSyncRequests(dbUsers, c.pgUsers) - if err := c.userSyncStrategy.ExecuteSyncRequests(pgSyncRequests, c.pgDb); err != nil { + if err = c.userSyncStrategy.ExecuteSyncRequests(pgSyncRequests, c.pgDb); err != nil { return fmt.Errorf("error executing sync statements: %v", err) } return nil