Skip to content

feat: initial implementation#2

Merged
smira merged 1 commit intomainfrom
feat/initial-implementation
Nov 3, 2025
Merged

feat: initial implementation#2
smira merged 1 commit intomainfrom
feat/initial-implementation

Conversation

@smira
Copy link
Copy Markdown
Member

@smira smira commented Oct 27, 2025 •
edited
Loading

Initial implementation goal: pass conformance tests.

Pending items:

  • optimize list/watch queries
  • add some benchmarks
  • consider some optimizations/tuning
  • compact events table
  • proper in-process notifications

@smira smira force-pushed the feat/initial-implementation branch 3 times, most recently from 23916f0 to bd95746 Compare October 28, 2025 09:44
@smira smira mentioned this pull request Oct 29, 2025
Closed
@smira smira force-pushed the feat/initial-implementation branch 4 times, most recently from a2f8e39 to 2f3523d Compare October 30, 2025 13:10
frezbo
frezbo reviewed Oct 30, 2025
View reviewed changes
@smira smira force-pushed the feat/initial-implementation branch 6 times, most recently from c5c647d to 9c03560 Compare October 31, 2025 15:31
smira added a commit to smira/omni that referenced this pull request Oct 31, 2025
@smira
feat: use sqlite as secondary resource storage
08ffcca 
This pulls in cosi-project/state-sqlite#2

This is just PoC for now, it misses the migration step.

See siderolabs#1770

See siderolabs#1768

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
@smira smira mentioned this pull request Oct 31, 2025
Merged
smira added a commit to smira/omni that referenced this pull request Oct 31, 2025
@smira
feat: use sqlite as secondary resource storage
5278520 
This pulls in cosi-project/state-sqlite#2

This is just PoC for now, it misses the migration step.

See siderolabs#1770

See siderolabs#1768

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
utkuozdemir
utkuozdemir reviewed Nov 3, 2025
View reviewed changes
utkuozdemir
utkuozdemir reviewed Nov 3, 2025
View reviewed changes
utkuozdemir
utkuozdemir reviewed Nov 3, 2025
View reviewed changes
Slessi
Slessi reviewed Nov 3, 2025
View reviewed changes
pkg/state/impl/sqlite/read.go

rows, err := st.db.QueryContext(ctx, `SELECT spec
FROM `+st.options.TablePrefix+`resources
WHERE namespace = ? AND type = ? AND `+filter.CompileLabelQueries(options.LabelQueries),
Copy link
Copy Markdown

@Slessi Slessi Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

isn't this label query opening up to sql injection? shouldnt you parameterise it with ? ?

Copy link
Copy Markdown
Member Author

@smira smira Nov 3, 2025

Choose a reason for hiding this comment

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

not really, if you look into filter package - it creates a valid/escaped/quoted SQL which we inject here.

We can't use ? for a part of SQL query

Copy link
Copy Markdown
Member Author

@smira smira Nov 3, 2025

Choose a reason for hiding this comment

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

I updated with more tests which verify explicitly escaping

utkuozdemir
utkuozdemir approved these changes Nov 3, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@utkuozdemir utkuozdemir left a comment

Choose a reason for hiding this comment

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

really cool 🔥

Unix4ever
Unix4ever reviewed Nov 3, 2025
View reviewed changes
Unix4ever
Unix4ever reviewed Nov 3, 2025
View reviewed changes
pkg/state/impl/sqlite/compact.go
// pick cutoff event ID based on max events to keep
cutoffEventID := maxEventID - int64(s.options.CompactMaxEvents) + 1

// perform binary search on events table in the range [minEventID, cutoffEventID)
Copy link
Copy Markdown
Member

@Unix4ever Unix4ever Nov 3, 2025

Choose a reason for hiding this comment

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

Couldn't we use index on the events timestamp and just select the events without filtering them on the client?

Copy link
Copy Markdown
Member Author

@smira smira Nov 3, 2025

Choose a reason for hiding this comment

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

we could, but this index would add overhead on each operation (insert, modify delete of resources) , while deleting lots of events from the table from my point of view is anyways closer to full scan, so we don't add extra indexes which will be hard to maintain due to high rate of changes to the table and the index

@smira smira force-pushed the feat/initial-implementation branch from 9c03560 to d710fda Compare November 3, 2025 13:29
@smira
feat: initial implementation
ff68677 
Initial implementation goal: pass conformance tests.

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
@smira smira force-pushed the feat/initial-implementation branch from d710fda to ff68677 Compare November 3, 2025 13:32
@smira
Copy link
Copy Markdown
Member Author

smira commented Nov 3, 2025

/m

1 similar comment
@smira
Copy link
Copy Markdown
Member Author

smira commented Nov 3, 2025

/m

@smira smira merged commit ff68677 into main Nov 3, 2025
15 checks passed
smira added a commit to smira/omni that referenced this pull request Nov 4, 2025
@smira
feat: use sqlite as secondary resource storage
42502ad 
This pulls in cosi-project/state-sqlite#2

This is just PoC for now, it misses the migration step.

See siderolabs#1770

See siderolabs#1768

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/omni that referenced this pull request Nov 4, 2025
@smira
feat: use sqlite as secondary resource storage
7f73c5c 
This pulls in cosi-project/state-sqlite#2

This is just PoC for now, it misses the migration step.

See siderolabs#1770

See siderolabs#1768

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/omni that referenced this pull request Nov 4, 2025
@smira
feat: use sqlite as secondary resource storage
f6277fd 
This pulls in cosi-project/state-sqlite#2

This is just PoC for now, it misses the migration step.

See siderolabs#1770

See siderolabs#1768

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/omni that referenced this pull request Nov 4, 2025
@smira
feat: use sqlite as secondary resource storage
d06a3c3 
This pulls in cosi-project/state-sqlite#2

This is just PoC for now, it misses the migration step.

See siderolabs#1770

See siderolabs#1768

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/omni that referenced this pull request Nov 4, 2025
@smira
feat: use sqlite as secondary resource storage
2c4b1d1 
This pulls in cosi-project/state-sqlite#2

This is just PoC for now, it misses the migration step.

See siderolabs#1770

See siderolabs#1768

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/omni that referenced this pull request Nov 5, 2025
@smira
feat: use sqlite as secondary resource storage
a14abb9 
This pulls in cosi-project/state-sqlite#2

Fixes siderolabs#1770

See siderolabs#1768

Sample migration logs:

```
2025-11-05T11:18:47.340Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:122        migrated resources from BoltDB to SQLite        {"namespace": "metrics"
, "type": "EtcdBackupOverallStatuses.omni.sidero.dev", "count": 1}
2025-11-05T11:18:47.340Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:122        migrated resources from BoltDB to SQLite        {"namespace": "metrics"
, "type": "EtcdBackupStatuses.omni.sidero.dev", "count": 0}
2025-11-05T11:18:47.342Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:122        migrated resources from BoltDB to SQLite        {"namespace": "metrics"
, "type": "MachineStatusLinks.omni.sidero.dev", "count": 2}
2025-11-05T11:18:47.342Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:67 removed old BoltDB database after migration     {"path": "_out/secondary-storag
e/bolt.db"}
```

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
smira added a commit to smira/omni that referenced this pull request Nov 5, 2025
@smira
feat: use sqlite as secondary resource storage
75a9f3e 
This pulls in cosi-project/state-sqlite#2

Fixes siderolabs#1770

See siderolabs#1768

Sample migration logs:

```
2025-11-05T11:18:47.340Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:122        migrated resources from BoltDB to SQLite        {"namespace": "metrics"
, "type": "EtcdBackupOverallStatuses.omni.sidero.dev", "count": 1}
2025-11-05T11:18:47.340Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:122        migrated resources from BoltDB to SQLite        {"namespace": "metrics"
, "type": "EtcdBackupStatuses.omni.sidero.dev", "count": 0}
2025-11-05T11:18:47.342Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:122        migrated resources from BoltDB to SQLite        {"namespace": "metrics"
, "type": "MachineStatusLinks.omni.sidero.dev", "count": 2}
2025-11-05T11:18:47.342Z        ESC[34mINFOESC[0m       omni/state_sqlite.go:67 removed old BoltDB database after migration     {"path": "_out/secondary-storag
e/bolt.db"}
```

Signed-off-by: Andrey Smirnov <andrey.smirnov@siderolabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@utkuozdemir utkuozdemir utkuozdemir approved these changes

@Unix4ever Unix4ever Unix4ever approved these changes

@frezbo frezbo frezbo approved these changes

@shanduur shanduur shanduur approved these changes

+1 more reviewer

@Slessi Slessi Slessi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@smira @Unix4ever @utkuozdemir @Slessi @frezbo @shanduur