Add initial SQLite-backed state implementation #17586

mheon · 2023-02-20T17:55:46Z

This contains the implementation of (most) container functions, with stubs for all pod and volume functions. Presently accessed via environment variable only for testing purposes.

Podman can now use a SQLite database as a backend for increased stability.

openshift-ci · 2023-02-20T17:55:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mheon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vrothberg

A couple of minor nits and questions.

vrothberg · 2023-02-21T09:47:10Z

libpod/boltdb_state.go

@@ -570,6 +565,61 @@ func (s *BoltState) GetName(id string) (string, error) {
 	return name, nil
 }

+// GetPodName returns the name associated with a given ID.
+// Returns ErrNoSuchPor if the ID does not exist.


s/ErrNoSuchPor/ErrNoSuchPoD/

vrothberg · 2023-02-21T09:50:22Z

libpod/runtime.go

@@ -415,10 +427,9 @@ func makeRuntime(runtime *Runtime) (retErr error) {
 		logrus.Errorf("Runtime paths differ from those stored in database, storage reset may not remove all files")
 	}

-	if err := runtime.state.SetNamespace(runtime.config.Engine.Namespace); err != nil {
-		return fmt.Errorf("setting libpod namespace in state: %w", err)
+	if runtime.config.Engine.Namespace != "" {


The error message isn't that helpful for users who have a namespace configured in containers.conf. I suggest two changes:

Update the error message and point to containers.conf

Update containers.conf (i.e., c/common/pkg/config) and turn the namespace into a NOP along with removing it from the docs etc.

libpod/runtime_ctr.go

vrothberg · 2023-02-21T09:57:47Z

libpod/sqlite_state.go

+		return nil, fmt.Errorf("cannot connect to database: %w", err)
+	}
+
+	if _, err := state.conn.Exec("PRAGMA foreign_keys = ON;"); err != nil {


Please add comments to what these settings do. Ideally with links to docs.

Can the Execs be bundle into one?

I don't think so, one statement per exec seems to be safest.

"seems" or "is"? I don't want to challenge but am mostly looking for cases that could be further tweaked for performance.

Seems. I'm not 100% on this - my reading on the subject is not conclusive.

vrothberg · 2023-02-21T10:03:25Z

libpod/sqlite_state_internal.go

+        CREATE TABLE IF NOT EXISTS DBConfig(
+                Id            INTEGER PRIMARY KEY NOT NULL,
+                SchemaVersion INTEGER NOT NULL,
+                Os            TEXT    NOT NULL,


Can we call it ID and OS? Maybe just my personal preference but Id and Os always look wrong to me.

libpod/sqlite_state_internal.go

vrothberg · 2023-02-21T10:12:01Z

libpod/sqlite_state_internal.go

+		"PodConfig": podConfig,
+		"PodState": podState,
+		"VolumeConfig": volumeConfig,
+		"volumeState": volumeState,


The key is lower caps. Is that intended?

libpod/sqlite_state_internal.go

libpod/state.go

edsantiago · 2023-02-21T21:02:43Z

CI seems stuck?

This contains the implementation of (most) container functions, with stubs for all pod and volume functions. Presently accessed via environment variable only for testing purposes. Signed-off-by: Matt Heon <[email protected]>

This has been broken since we added Volumes - so, Podman v0.12.1 (so, around 5 years). I have no evidence anyone is using it in the wild. It doesn't really function as expected. And it's a lot of extraneous code and tests for the database. Rip it out entirely, we can re-add once BoltDB is gone if there is a requirement to do so. Signed-off-by: Matt Heon <[email protected]>

- Added a mechanism to check schema version and migrate (no migrations yet since schema hasn't changed yet). - Added pod support to AddContainer, and unified AddContainer and RemoveContainer between containers and pods. - Fixed newly-added GetPodName and GetCtrName in BoltDB so they only return pod/container names. Signed-off-by: Matt Heon <[email protected]>

Signed-off-by: Matt Heon <[email protected]>

mheon · 2023-02-22T18:25:52Z

Removing WIP. This is good to review now.

@Luap99 @baude @vrothberg PTAL

baude · 2023-02-22T19:58:04Z

libpod/sqlite_state.go

+	runtime *Runtime
+}
+
+// NewBoltState creates a new bolt-backed state database


bolt references

baude · 2023-02-22T20:01:39Z

libpod/sqlite_state.go

+		runtimeOS                                                          = goruntime.GOOS
+		runtimeStaticDir                                                   = filepath.Clean(s.runtime.config.Engine.StaticDir)
+		runtimeTmpDir                                                      = filepath.Clean(s.runtime.config.Engine.TmpDir)
+		runtimeGraphRoot                                                   = filepath.Clean(s.runtime.StorageConfig().GraphRoot)


long-term these could be ripe for a struct, esp bc you do a bunch of validation afterwards, take it or leave it

baude · 2023-02-22T20:05:03Z

libpod/sqlite_state.go

+		return "", fmt.Errorf("looking up container %s name: %w", id, err)
+	}
+
+	return name, nil


would it be faster to put the return name after the query and deal with the errors afterwards?

Can you describe this better? I can't really visualize what you're looking for.

baude · 2023-02-22T20:05:18Z

libpod/sqlite_state.go

+		return "", fmt.Errorf("looking up pod %s name: %w", id, err)
+	}
+
+	return name, nil


same as above, about early return

baude · 2023-02-22T20:08:55Z

libpod/sqlite_state.go

+	foundResult := false
+	for rows.Next() {
+		if foundResult {
+			return "", fmt.Errorf("more than one result for container %q: %w", idOrName, define.ErrCtrExists)


should this be more about unable to find unique container from %s ? Also, ErrCtrExists ... is that the correct error ?

This matches what Bolt did.

baude · 2023-02-22T20:10:44Z

libpod/sqlite_state.go

+	if !s.valid {
+		return nil, define.ErrDBClosed
+	}
+


can this use Lookupcontainerorid? Or perhaps an unexported func that prevents the duplicate query between funcs?

Duplication saves us one query, so it's a bit faster this was, but we can probably share parts of the logic.

baude · 2023-02-22T20:14:53Z

libpod/sqlite_state.go

+// WARNING: This function is DANGEROUS. Do not use without reading the full
+// comment on this function in state.go.
+// TODO: Once BoltDB is removed, this can be combined with SafeRewriteContainerConfig.
+func (s *SQLiteState) RewriteContainerConfig(ctr *Container, newCfg *ContainerConfig) error {


does it need to be exported?

To meet the interface, yes.

baude · 2023-02-22T20:15:15Z

libpod/sqlite_state.go

+// DO NOT USE TO: Change container dependencies, change pod membership, change
+// locks, change container ID.
+// TODO: Once BoltDB is removed, this can be combined with RewriteContainerConfig.
+func (s *SQLiteState) SafeRewriteContainerConfig(ctr *Container, oldName, newName string, newCfg *ContainerConfig) error {


same question wrt "dangerous" methods

It's not accessible outside of Libpod because the DB is never passed out of libpod.

baude · 2023-02-22T20:18:42Z

libpod/sqlite_state.go

+
+// LookupPod retrieves a pod from a full or unique partial ID, or a name.
+func (s *SQLiteState) LookupPod(idOrName string) (*Pod, error) {
+	if idOrName == "" {


this same boilerplate is used each time. i could some beauty in something like:

if err := s.validate(idOrName); err != nil { return nil, err}

baude · 2023-02-22T20:22:10Z

LGTM, made suggestions for consideration. Not required for merge.

Signed-off-by: Matt Heon <[email protected]>

vrothberg · 2023-02-23T08:07:05Z

/lgtm

Let's merge so I can do the Pod plumbing. Once that's in, we can tackle refactoring and code clones.

mtrmac

(I was just curious, and I have no expertise in this area of Podman nor specifics of SQLite. Consider these as just mild suggestions.)

mtrmac · 2023-02-23T16:52:04Z

libpod/sqlite_state.go

+        INSERT INTO DBconfig VALUES (
+                ?, ?, ?,
+                ?, ?, ?,
+                ?, ?, ?
+        );`


(Maybe this should specify the destination columns explicitly, to account for possible future writers that add more columns to the table. I suppose that it should never happen that DBconfig schema is defined while there are no rows in that table… still, it seems to me that naming the destination columns is a good habit in general.

E.g. naming columns explicitly means that the Go code doesn’t need to worry about the column ordering in the schema at all.

To an extent, applies to all INSERTs, although the others should be even less affected than this one.)

mtrmac · 2023-02-23T16:54:13Z

libpod/sqlite_state.go

+		return "", define.ErrDBClosed
+	}
+
+	rows, err := s.conn.Query("SELECT ID FROM ContainerConfig WHERE ContainerConfig.Name=? OR (ContainerConfig.ID LIKE ?);", idOrName, idOrName)


Doesn’t this allow users to inject SQL metacharacters into the LIKE pattern?

(Sure, Podman itself probably doesn’t care. I’m worried that someone builds a system with half a dozen abstraction layers on top, and this ends up enabling some kind of sandbox / namespaced account escape. So I’d be more comfortable with either some code that escapes pattern characters in idOrName, or ideally restricting idOrName to an explicitly-allowed set of characters.)

(Applies to LIKE ? throughout.)

mtrmac · 2023-02-23T17:05:33Z

libpod/sqlite_state.go

+	tx, err := s.conn.Begin()
+	if err != nil {
+		return fmt.Errorf("beginning transaction to add exit code: %w", err)
+	}
+	defer func() {
+		if defErr != nil {
+			if err := tx.Rollback(); err != nil {
+				logrus.Errorf("Rolling back transaction to add exit code: %v", err)
+			}
+		}
+	}()


The begin/rollback/exec/commit boilerplate really strongly suggests to me the need for an execInTransaction(command string, params any...) error helper that implements that once instead of copy&pasting.

A lower-level execFnInTransaction(func () error) error that just encapsulates the begin/rollback/commit parts, allowing the caller to perform multiple operations, might also pay off.

mtrmac · 2023-02-23T17:07:05Z

libpod/sqlite_state.go

+	row := s.conn.QueryRow("SELECT ContainerID FROM ContainerExecSession WHERE ID=?;", id)
+
+	var ctrID string
+	if err := row.Scan(&ctrID); err != nil {
+		if errors.Is(err, sql.ErrNoRows) {
+			return "", fmt.Errorf("no exec session with ID %s found: %w", id, define.ErrNoSuchExecSession)
+		}
+		return "", fmt.Errorf("retrieving exec session %s from database: %w", id, err)
+	}


A getSingleSelectResult(dest *any, query string, params any...) might also pay off.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Feb 20, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2023

mheon force-pushed the add_sql_state branch from fc6e29f to 9ebc732 Compare February 20, 2023 18:21

baude added the bloat_approved Approve a PR in which binary file size grows by over 50k label Feb 20, 2023

mheon force-pushed the add_sql_state branch 2 times, most recently from 48d1d85 to 533aafc Compare February 20, 2023 21:16

vrothberg reviewed Feb 21, 2023

View reviewed changes

mheon added 11 commits February 22, 2023 11:00

Add initial SQLite-backed state implementation

1b968c6

This contains the implementation of (most) container functions, with stubs for all pod and volume functions. Presently accessed via environment variable only for testing purposes. Signed-off-by: Matt Heon <[email protected]>

Fix various lint issues

8ab18d8

Signed-off-by: Matt Heon <[email protected]>

Implement Network Connect/Modify for SQLite state

939a4cc

Signed-off-by: Matt Heon <[email protected]>

Implement network disconnect for SQLite state

97499a7

Signed-off-by: Matt Heon <[email protected]>

Get E2E tests to pass

89d0ccd

Signed-off-by: Matt Heon <[email protected]>

Remove --namespace flag from Podman root

c4fe0af

Signed-off-by: Matt Heon <[email protected]>

Various fixes from code review

627a5b7

Signed-off-by: Matt Heon <[email protected]>

Implement exec session handling in SQL database

c0b92bd

Signed-off-by: Matt Heon <[email protected]>

Add support for volume operations to SQLite state

59a54f3

Signed-off-by: Matt Heon <[email protected]>

mheon force-pushed the add_sql_state branch from b5656e7 to 680ed75 Compare February 22, 2023 16:05

Add support for containers.conf database setting

eb228f2

Signed-off-by: Matt Heon <[email protected]>

mheon force-pushed the add_sql_state branch from 680ed75 to eb228f2 Compare February 22, 2023 17:59

mheon changed the title ~~[WIP] Add initial SQLite-backed state implementation~~ Add initial SQLite-backed state implementation Feb 22, 2023

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 22, 2023

baude reviewed Feb 22, 2023

View reviewed changes

libpod/sqlite_state.go Outdated

runtime *Runtime

}

// NewBoltState creates a new bolt-backed state database

Copy link

Member

baude Feb 22, 2023

Choose a reason for hiding this comment

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

bolt references

baude reviewed Feb 22, 2023

View reviewed changes

Fix an incorrect comment on NewSqliteState

03aaa8d

Signed-off-by: Matt Heon <[email protected]>

openshift-ci bot assigned vrothberg Feb 23, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023

openshift-merge-robot merged commit 3796e22 into containers:main Feb 23, 2023

mtrmac reviewed Feb 23, 2023

View reviewed changes

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 8, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial SQLite-backed state implementation #17586

Add initial SQLite-backed state implementation #17586

mheon commented Feb 20, 2023

openshift-ci bot commented Feb 20, 2023

vrothberg left a comment

vrothberg Feb 21, 2023

vrothberg Feb 21, 2023

vrothberg Feb 21, 2023

vrothberg Feb 21, 2023

mheon Feb 21, 2023

vrothberg Feb 21, 2023

mheon Feb 21, 2023

vrothberg Feb 21, 2023

vrothberg Feb 21, 2023

edsantiago commented Feb 21, 2023

mheon commented Feb 22, 2023

baude Feb 22, 2023

baude Feb 22, 2023

baude Feb 22, 2023

mheon Feb 23, 2023

baude Feb 22, 2023

baude Feb 22, 2023

mheon Feb 23, 2023

baude Feb 22, 2023

mheon Feb 23, 2023

baude Feb 22, 2023

mheon Feb 23, 2023

baude Feb 22, 2023

mheon Feb 23, 2023

baude Feb 22, 2023 •

edited

Loading

baude commented Feb 22, 2023

vrothberg commented Feb 23, 2023

mtrmac left a comment

mtrmac Feb 23, 2023

mtrmac Feb 23, 2023

mtrmac Feb 23, 2023

mtrmac Feb 23, 2023 •

edited

Loading

Add initial SQLite-backed state implementation #17586

Add initial SQLite-backed state implementation #17586

Conversation

mheon commented Feb 20, 2023

openshift-ci bot commented Feb 20, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Feb 21, 2023

mheon commented Feb 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baude Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

baude commented Feb 22, 2023

vrothberg commented Feb 23, 2023

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

baude Feb 22, 2023 •

edited

Loading

mtrmac Feb 23, 2023 •

edited

Loading