Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Storage: local JSON file #123

Merged
merged 4 commits into from
Aug 24, 2023

Conversation

nazarewk
Copy link
Contributor

@nazarewk nazarewk commented Aug 23, 2023

This project seems like the only candidate so far to build a simple local IPAM (with "database" checked into git repository), but it was missing this type of storage, so I have implemented it as a local JSON file:

  • uses internal Memory backend for operating on the data
    • preserved it's internal data structure in the output
  • "refreshing" from file on reads and fully reloading on writes based on tracking of modification times
  • json.go for serialization and deserialization of Prefixes

It is pretty barebones and could be improved, but should be good enough for easy evaluation of the project/library with persistence enabled.

some improvements I thought of:

  • fix Dump()/Load() losing some informations (eg: Namespaces) and reuse this logic as a JSON data structure
  • could involve file system locking of files (didn't go with it at the start because it's pretty unix'y feature)
  • could be more performant:
    • split into multiple files for smaller edits
    • more efficient reload (eg: diff the data instead of clearing and loading whole file)

@nazarewk nazarewk requested a review from a team as a code owner August 23, 2023 12:25
Copy link
Contributor

@majst01 majst01 left a comment

Choose a reason for hiding this comment

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

Nice addition, had only a first look for cosmetic stuff, deeper look later

file.go Outdated Show resolved Hide resolved
file.go Show resolved Hide resolved
file.go Outdated Show resolved Hide resolved
file.go Outdated Show resolved Hide resolved
file.go Outdated Show resolved Hide resolved
testing_test.go Outdated Show resolved Hide resolved
testing_test.go Outdated Show resolved Hide resolved
cmd/server/main.go Outdated Show resolved Hide resolved
@majst01
Copy link
Contributor

majst01 commented Aug 23, 2023

Linter complains:

  Error: G306: Expect WriteFile permissions to be 0600 or less (gosec)
  Error: type `testableFileStorage` is unused (unused)
  Error: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
  Error: comparing with != will fail on wrapped errors. Use errors.Is to check for a specific error (errorlint)
  
  Error: issues found

file.go Outdated Show resolved Hide resolved
@majst01
Copy link
Contributor

majst01 commented Aug 24, 2023

I think there is nothing wrong with your approach, but have you considered using SQLite instead, this would also be a local storage solution but with all the downsides you mentioned fixed ?

I started such a support back then, but havent enough time to finish that.

@nazarewk
Copy link
Contributor Author

nazarewk commented Aug 24, 2023

Yeah my first thought was SQLite, but it would not advance in any meaningful way my use case of checking state into the repository in a human readable format.

file.go Outdated Show resolved Hide resolved
@nazarewk
Copy link
Contributor Author

not sure what is this error ERROR: denied: installation not allowed to Write organization package

@majst01
Copy link
Contributor

majst01 commented Aug 24, 2023

not sure what is this error ERROR: denied: installation not allowed to Write organization package

PRs from outside the metal-stack org are not allowed to push to our registry, this is nothing to worry about. We where not able to implement this in the CI that the push is not executed in such a case.

@codecov-commenter
Copy link

Codecov Report

Merging #123 (04f4405) into master (6f010ba) will decrease coverage by 0.73%.
The diff coverage is 57.57%.

❗ Current head 04f4405 differs from pull request most recent head bb6d2bd. Consider uploading reports for the commit bb6d2bd to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   64.85%   64.13%   -0.73%     
==========================================
  Files          10       11       +1     
  Lines        1491     1656     +165     
==========================================
+ Hits          967     1062      +95     
- Misses        341      380      +39     
- Partials      183      214      +31     
Files Changed Coverage Δ
file.go 57.57% <57.57%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@majst01
Copy link
Contributor

majst01 commented Aug 24, 2023

Nice, its still quite fast:

BenchmarkNewPrefix/MongoDB-2    	     662	   1790806 ns/op	   34140 B/op	     397 allocs/op
BenchmarkAcquireIP/Memory-2     	  255164	      4491 ns/op	    2224 B/op	      43 allocs/op
BenchmarkAcquireIP/File-2       	    2041	    543635 ns/op	   20134 B/op	     210 allocs/op
BenchmarkAcquireIP/Postgres-2   	     310	   3839610 ns/op	   10969 B/op	     262 allocs/op
BenchmarkAcquireIP/Cockroach-2  	     135	   9741293 ns/op	   10964 B/op	     261 allocs/op
BenchmarkAcquireIP/Redis-2      	     921	   1340158 ns/op	   11822 B/op	     272 allocs/op
BenchmarkAcquireIP/Etcd-2       	     345	   3552269 ns/op	   63949 B/op	    1073 allocs/op
BenchmarkAcquireIP/KeyDB-2      	     872	   1487534 ns/op	   11820 B/op	     272 allocs/op
BenchmarkAcquireIP/MongoDB-2    	     651	   1842363 ns/op	   40701 B/op	     532 allocs/op

Can you add the filesytem backend into the Readme, skip the benchmark numbers for now.

@nazarewk
Copy link
Contributor Author

nazarewk commented Aug 24, 2023

Nice, its still quite fast:

BenchmarkNewPrefix/MongoDB-2    	     662	   1790806 ns/op	   34140 B/op	     397 allocs/op
BenchmarkAcquireIP/Memory-2     	  255164	      4491 ns/op	    2224 B/op	      43 allocs/op
BenchmarkAcquireIP/File-2       	    2041	    543635 ns/op	   20134 B/op	     210 allocs/op
BenchmarkAcquireIP/Postgres-2   	     310	   3839610 ns/op	   10969 B/op	     262 allocs/op
BenchmarkAcquireIP/Cockroach-2  	     135	   9741293 ns/op	   10964 B/op	     261 allocs/op
BenchmarkAcquireIP/Redis-2      	     921	   1340158 ns/op	   11822 B/op	     272 allocs/op
BenchmarkAcquireIP/Etcd-2       	     345	   3552269 ns/op	   63949 B/op	    1073 allocs/op
BenchmarkAcquireIP/KeyDB-2      	     872	   1487534 ns/op	   11820 B/op	     272 allocs/op
BenchmarkAcquireIP/MongoDB-2    	     651	   1842363 ns/op	   40701 B/op	     532 allocs/op

Can you add the filesytem backend into the Readme, skip the benchmark numbers for now.

Done, I guess it highly depends on the backing storage and it currently lands on a ramdisk (tmpfs for /tmp/)

file.go Show resolved Hide resolved
file.go Outdated
data, err = json.Marshal(storage)
}
if err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe give some contextual message to the error

file.go Show resolved Hide resolved
file.go Outdated Show resolved Hide resolved
Copy link
Contributor

@majst01 majst01 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution !

@majst01 majst01 merged commit ca86640 into metal-stack:master Aug 24, 2023
2 of 3 checks passed
@nazarewk nazarewk deleted the feature/local-file-backend branch August 24, 2023 12:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants