Skip to content
New issue

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

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

Already on GitHub? # to your account

Fix rpc error when encoding default is legacy #1046

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

jrray
Copy link
Collaborator

@jrray jrray commented Jun 13, 2024

The rpc code was assuming that a default object header will be in flatbuffers format, but at SPI we have changed it to default to legacy until we are ready to adopt flatbuffer format files. Changing the default causes rpc related tests to fail.

It is possible that rpc does not handle legacy format files without the change.

Since rpc now sends the FlatObject data over the wire as raw bytes, but when using legacy format the header will be set to legacy, the receiving end tries to respect the header and decode the raw bytes as if they are in legacy format, as would be proper behavior when reading a legacy file from disk. But when decoding an rpc message, the payload is always in flatbuffers format.

A new constructor was added that disregards the header's claimed format and always treats the payload as flatbuffers encoded. This also means that the server will write an object with a legacy header out to disk in legacy format. Is this the desired behavior?

Should graph::Object::new_with_default_header be changed to stop assuming what a "default" header is and hard code itself to use the new encoding? It is unclear if the requirements of this method are to produce a header set the new encoding, or to really just respect whatever the default in the code is set to.

The rpc code was assuming that a default object header will be in
flatbuffers format, but at SPI we have changed it to default to legacy
until we are ready to adopt flatbuffer format files. Changing the
default causes rpc related tests to fail.

It is possible that rpc does not handle legacy format files without the change.

Since rpc now sends the FlatObject data over the wire as raw bytes, but
when using legacy format the header will be set to legacy, the receiving
end tries to respect the header and decode the raw bytes as if they are
in legacy format, as would be proper behavior when reading a legacy file
from disk. But when decoding an rpc message, the payload is _always_ in
flatbuffers format.

A new constructor was added that disregards the header's claimed format and
always treats the payload as flatbuffers encoded. This also means that
the server will write an object with a legacy header out to disk in
legacy format. Is this the desired behavior?

Should `graph::Object::new_with_default_header` be changed to stop
assuming what a "default" header is and hard code itself to use the new
encoding? It is unclear if the requirements of this method are to
produce a header set the new encoding, or to really just respect
whatever the default in the code is set to.

Signed-off-by: J Robert Ray <jrray@imageworks.com>
@jrray jrray self-assigned this Jun 13, 2024
@jrray
Copy link
Collaborator Author

jrray commented Jun 13, 2024

For example, the test sync::sync_test::test_sync_ref::case_3_rpc fails when

  1. It tries to write a blob to storage and these bytes are sent over the wire: b"--SPFS--\n\0\0\0\0\0\0\0\0\x10\0\0\0\0\0\0\0\x08\0\x0c\0\x07\0\x08\0\x08\0\0\0\0\0\0\x04\x0c\0\0\0\x08\0,\0$\0\x04\0\x08\0\0\0\t\xca~N\xaan\x8a\xe9\xc7\xd2a\x16q)\x18H\x83dM\x07\xdf\xba|\xbf\xbcL\x8a.\x086\r[\x0c\0\0\0\0\0\0\0".
    The header is in legacy mode but like all FlatObjects the rest of the data is flatbuffers encoded.

  2. Receiving end tries to parse this as legacy data and ends up with:

[crates/spfs/src/server/database.rs:101:13] &object = Blob(
    Blob {
        payload: "CAAAAAAAAAAAACAABQAAOAAIAAEAAAAAAAAAABAMAAAAACAAFQAA====",
        size: 2594077783546134528,
    },
)

It's easy to see the size is wrong, and the digest is wrong too.

  1. Things continue to go wrong and the test fails with:
thread 'sync::sync_test::test_sync_ref::case_3_rpc' panicked at crates/spfs/src/./sync_test.rs:102:10:
called `Result::unwrap()` on an `Err` value: String("String(\"invalid entry kind: \\u{14}\")")

@jrray jrray requested a review from rydrman June 13, 2024 01:11
/// In memory, objects always use the latest flatbuffer
/// format. The given bytes may be discarded or reconstructed
/// if a conversion is necessary, but the header is preserved
/// in order to ensure that the object does not change it's
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
/// in order to ensure that the object does not change it's
/// in order to ensure that the object does not change its

@rydrman
Copy link
Collaborator

rydrman commented Jun 26, 2024

From the meeting today:

  • this change makes sense as a solution to the issue
  • we should add a field to the grpc message to denote what format the buffer is in just in case we have future needs to move away from flat buffers as the in-memory representation

@rydrman
Copy link
Collaborator

rydrman commented Jul 4, 2024

@jrray can we get this merged in? I'd like to get it worked in downstream as it might help resolve an issue that we're seeing now and then

Signed-off-by: J Robert Ray <jrray@jrray.org>
@jrray jrray merged commit 1cad0b6 into main Jul 4, 2024
6 of 7 checks passed
@jrray jrray deleted the fix-rpc-with-legacy-encoding branch July 4, 2024 22:26
@jrray
Copy link
Collaborator Author

jrray commented Jul 4, 2024

@rydrman merged.

CI failed for a spurious error, but it had passed previously and all that changed was some text in a comment.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants