search

mozilla-services / syncstorage-rs

Sync Storage server in Rust
Mozilla Public License 2.0
973 stars 49 forks source link

feat: Syncstorage inital sqlite support #1520

Open Eragonfr opened 8 months ago

Eragonfr commented 8 months ago

Description

Re-implementation of #935 on the newer codebase.
Thanks for @mqus for the first implementation.

Issue(s)

Partially fixes #498

Todo

Current state is that it runs on my computer, but it's probably not clean at all, there's still a lot of duplicate code, and some SQL queries are still in MySQL-flavored SQL but could be used by the SQLite backend.

Eragonfr commented 8 months ago

To pass the checks I have the choice between upgrading diesel or ignoring the issue with libsqlite-sys (which tbh is probably a non-issue because we should not be using SQLite printf anyway).

Eragonfr commented 5 months ago

I choose to ignore the libsqlite3-sys CVE because it's not impacting this project. That should let me run the other tests in the CI.

bcspragu commented 2 months ago

I'm really jazzed about this PR, thanks for all the work on it! I noticed this in the PR desc:

  • [ ] docker container (Help needed)

If this is a question of building a Docker container from this codebase that bundles all the SQLite deps and nothing more, I'm happy to take on that work.

Eragonfr commented 2 months ago

I'm really jazzed about this PR, thanks for all the work on it! I noticed this in the PR desc:

  • [ ] docker container (Help needed)

If this is a question of building a Docker container from this codebase that bundles all the SQLite deps and nothing more, I'm happy to take on that work.

Something like that yes. There might be some docker-compose configuration files to make too. I gave multiple tries to Docker but I never seems to get a "standard" docker environment. I always get something with some broken networking, nuked firewall or something else. At this point I've given up on that technology. (docker should release VMs that I can start and get a "standard" env with networking and stuff)

That would leave me to clean up my code

bcspragu commented 2 months ago

Sounds good, I'll give it a shot!

bcspragu commented 2 months ago

Okay, I think I've got something approximately working in https://github.com/Eragonfr/syncstorage-rs/compare/syncstorage-sqlite...bcspragu:syncstorage-rs:syncstorage-sqlite

E.g.

docker build -t mozsync-sqlite --build-arg DATABASE_BACKEND=sqlite .

# The --userns line is a rootless Podman thing, can be ignored mostly
docker run --rm -it \
    --userns=keep-id:uid=10001,gid=10001 \
    -v $PWD/temp-data-do-not-submit:/data \
    -e "SYNC_HOST=0.0.0.0" \
    -e "SYNC_MASTER_SECRET=secret0" \
    -e "SYNC_SYNCSTORAGE__DATABASE_URL=sqlite:///data/syncdb.sqlite" \
    -e "SYNC_TOKENSERVER__DATABASE_URL=sqlite:///data/tokenserverdb.sqlite" \
    -e "SYNC_TOKENSERVER__RUN_MIGRATIONS=true" \
    mozsync-sqlite

Seems to start up successfully, but I'm not brave enough to connect it to my real Firefox sync yet

Eragonfr commented 2 months ago

Thanks @bcspragu !

bcspragu commented 2 months ago

For sure! Also, looking at the current ci/circleci: build-and-test failure, that's a result of missing --features flags here: https://github.com/mozilla-services/syncstorage-rs/blob/4a503f8c36fe070e11df43a8ce0b3c71358e983c/.circleci/config.yml#L67

Not sure what the team would prefer here, but there's two options:

  1. If you're just looking for a sanity check, test against MySQL (e.g --features=syncstorage-db/mysql,tokenserver-db/mysql)
  2. Run separate builds for each DB backend
  3. [Actually, this is probably what we want] Fix the default flags to use MySQL correctly

Unrelated, one thing I forgot to mention is that regardless of which features are enabled, MySQL client libs are required on the host (e.g. there are places where it isn't gated behind the feature flag, it gets linked in regardless). I started adding the relevant #cfg(feature(...)) in places where I noticed it, but again, not sure what the team would prefer there.

Eragonfr commented 2 months ago

I thought of dropping the default flags because the user should choose their storage backend instead of relying on a default one.

We want to have the proper #cfg(feature(...)) when it's needed without breaking the spanner backend. (It uses MySQL for the token server database)

Eragonfr commented 1 month ago

The ci fails on sqlite e2e tests. I think it is because the /data/ folder is missing, which means that the database can't be created and that makes the ci to fail. I need some help to figure out how to either create this folder or to not need it in the container.

The checks are unrelated and are probably failing on the master branch too.

Eragonfr commented 1 month ago

:woman_facepalming: I'm dumb the e2e tests can't work because they require a connection to the database from the python script and syncserver at the same time. Which is not possible with sqlite.
There was no need for my last height commits.
But the e2e tests are not usable with sqlite.

JulianWgs commented 2 weeks ago

Hi @Eragonfr , thanks for your hard work! I am trying to get this to run locally, but I can't execute this query against the token database, because the database file is empty. Am I doing something wrong? Could you try running the server with a missing token database and check if the server correctly sets it up? Thanks again! I am really looking forward to this!

Eragonfr commented 2 weeks ago

@JulianWgs I also have the issue. I think that I broke the auto migration when doing some refactor. The tokenserver part is giving me some trouble to get it to use SQLite queries, it doesn't pass its unit tests and there is quite a lot of its queries that doesn't run.

EDIT: Can't reproduce that bug anymore… wtf

Eragonfr commented 1 week ago

I really don't like how I do my rebase. It's seems to be quite messy.

jrconlin commented 2 days ago

I really don't like how I do my rebase. It's seems to be quite messy.

I don't think anyone ever does like how they do their rebases.

Would you like me to give it a quick review to see if there's anything I might be able to help with?

Eragonfr commented 2 days ago

Would you like me to give it a quick review to see if there's anything I might be able to help with?

If you have any advices about how I can fix the tests for sqlite. Because we can't have both the server and the integration test script connected to the database. The only thing that I know we can do is to run the query, shutdown the server, then connect to the db, check the db content, disconnect, restart the server, back to the start.