-
Notifications
You must be signed in to change notification settings - Fork 113
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
backport: fix micro ocdav service and nats key encoding #4842
backport: fix micro ocdav service and nats key encoding #4842
Conversation
fa93401
to
fd88b89
Compare
fd88b89
to
2e11551
Compare
06455e8
to
ab72ea7
Compare
pkg/store/store.go
Outdated
@@ -152,6 +152,7 @@ func Create(opts ...microstore.Option) microstore.Store { | |||
return natsjskv.NewStore( | |||
append(opts, | |||
natsjskv.NatsOptions(natsOptions), // always pass in properly initialized default nats options | |||
natsjskv.EncodeKeys(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this a breaking change? Existing stores will not be able to read their keys any more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not relevant if nats-js-kv anyways didn't work in 5.x
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT the store is used for caches. If they cannot find a key they will reinsert them.
The store is also used by the nats-js-kl registry, which we had to make aware of versions. That introduced a seperator that needs to be encoded for nats. The old registry does not forget nodes and without this backport is causing a lot off I/O errors when services try to connect to no longer existing pods.
But let me double check what happens if I upgrade a running instance to this version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that stores are also used as key-value stores. For example in the event history or userlog service. If you encode the keys these stores can no longer read old keys
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damn, right. I was confusing this with the registry, but it instantiates its own store and usese the EncodeKeys option ...
unfortunately, something else changed. without this option we can no longer store anything because nats does not like our keys anymore:
2024-09-17T14:09:40+02:00 ERR error uploading file error="Failed to store data in bucket 'spaces/f1/bdd61a-da7c-49fc-8203-0558109d1b4f/nodes/64/bb/19/df/-1598-4691-a07a-d2c2f5714815.REV.2024-09-17T08:56:50.792574043Z': nats: invalid key" datatx=simple line=/home/jfd/Repositories/reva/pkg/rhttp/datatx/manager/simple/simple.go:147 pkg=rhttp service=storage-system traceid=49388ac282e8ffca3a5165cb484669b1
but ... why? did nats change? did we change the keys we store in nats? I can reproduce this when running ocis with
"OCIS_CACHE_STORE": "nats-js-kv",
"OCIS_CACHE_STORE_NODES":"localhost:9233",
"STORAGE_USERS_ID_CACHE_STORE":"nats-js-kv"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracked in owncloud/ocis#10098
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original issue was owncloud/ocis#9114
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while STORAGE_USERS_FILEMETADATA_CACHE_STORE is not used in the helm charts OCIS_CACHE_STORE is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm so for the backport we could leave out this line.
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: jkoberg <jkoberg@owncloud.com> Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> bump nats-js-kv Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
This reverts commit df0ba6d. reva 2.19 does not encode the keys in the store
184eb0d
to
9c13e84
Compare
backporting to stable5