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

rpcenc: Support reader redirect #6952

Merged
merged 3 commits into from
Aug 20, 2021
Merged

rpcenc: Support reader redirect #6952

merged 3 commits into from
Aug 20, 2021

Conversation

magik6k
Copy link
Contributor

@magik6k magik6k commented Jul 30, 2021

Includes docs and tests!

This should allow market processes to send piece bytes directly to workers


Closes #7107

@magik6k magik6k requested review from raulk and aarshkshah1992 July 30, 2021 11:31
@magik6k magik6k requested a review from a team as a code owner July 30, 2021 11:31
@magik6k magik6k force-pushed the feat/jsonrpc-redir branch from e7470ed to 8426a62 Compare July 30, 2021 11:35
@magik6k
Copy link
Contributor Author

magik6k commented Jul 30, 2021

(note that this should not require api version bump, as older clients should still "just work", they just won't support redirects)

@magik6k
Copy link
Contributor Author

magik6k commented Jul 30, 2021

Here's a wireshark capture of a TestReaderRedirect run just to confirm that it does exactly what's expeced:

2021-07-30-133835_1190x399_scrot

@aarshkshah1992
Copy link
Contributor

cc @jacobheun for tracking.

@jennijuju
Copy link
Member

jennijuju commented Aug 2, 2021

This won't be in v1.11.1.
@jacobheun @raulk however, I think this would be really nice to have as most of the miners uses sealing workers and this would keep even less work on the main miner process IIUC. having it in one of the M1 tags just to test it out will also be nice.
Im adding the M1 label for now just for tracking purpose, feel free to remove it if you this is out of scope.

@jennijuju jennijuju added M1-release P2 P2: Should be resolved labels Aug 2, 2021
@jennijuju jennijuju added this to the v1.11.2 milestone Aug 2, 2021
@raulk
Copy link
Member

raulk commented Aug 2, 2021

This would be great to have, but it's an optimisation and we should consider for 1.11.3.

@jennijuju
Copy link
Member

@raulk is this still need if storage.Json is copied overs/shared to the market nodes too?

@magik6k
Copy link
Contributor Author

magik6k commented Aug 10, 2021

Yes, when adding pieces without this data would still flow through the main miner process

@benjaminh83
Copy link

@jennijuju I would like to challenge the decision on postponing for future releases.
IRL, the main reason for splitting out the market-node, is to protect the miner-node from deal overload. If all deal data still have to passthrough the miner-node in the add piece process, then we are only half way.
We need this implemented into production along with the market-split upgrade! I and multiple miners will be happy to test it out in M1, just to make sure its working. We want to help, as it is very important for us that this lands in a production release asap!

@jennijuju jennijuju modified the milestones: v1.11.2, v1.11.3 Aug 11, 2021
Comment on lines +62 to +66
3.1. If the reader is of type `*sealing.NullReader`, the resulting object
is `ReaderStream{ Type: "null", Info: "[base 10 number of bytes]" }`
3.2. If the reader is of type `*RpcReader`, and it wasn't read from, we
notify that RpcReader to go a different push endpoint, and return
a `ReaderStream` object like in 3.4.
Copy link
Member

Choose a reason for hiding this comment

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

When do each of these cases happen?

Comment on lines +46 to +48
c.CheckRedirect = func(req *http.Request, via []*http.Request) error {
return http.ErrUseLastResponse
}
Copy link
Member

Choose a reason for hiding this comment

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

Won't this interrupt the redirect flow on the first occurrence? 🤔

Copy link
Contributor Author

@magik6k magik6k Aug 12, 2021

Choose a reason for hiding this comment

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

It's meant to, we want to handle the redirect ourselves in https://github.com/filecoin-project/lotus/pull/6952/files#diff-309dd201d9384284851cf021bce98a146c76f5db8a982949b87f1e4b3d63cb26R138 (we need to save the last URL form the HEAD requests for the final POST. Unfortunately we can't just do a POST and have Go handle the redirects, because Go's http client will send the request body with the first request, evan if it then gets the redirect response - which is why we do this whole HEAD request dance here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(and we have to use this weird CheckRedirect to be able to save the last redirect Location header, as otherwise if you just let the http client handle the redirect, there is no way to get that URL)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this error has a very special meaning, perhaps not obvious:

ErrUseLastResponse can be returned by Client.CheckRedirect hooks to control how redirects are processed. If returned, the next request is not sent and the most recent response is returned with its body unclosed

func ReaderParamEncoder(addr string) jsonrpc.Option {
// Client side parameter encoder. Runs on the rpc client side. io.Reader -> ReaderStream{}
Copy link
Member

Choose a reason for hiding this comment

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

Hoist to godoc for this method.

@jennijuju
Copy link
Member

Hey team - what’s the status on this PR? Is anyone actively working on it and address the comments?

@jacobheun jacobheun modified the milestones: v1.11.3, v1.11.2 Aug 16, 2021
@jacobheun
Copy link
Contributor

We'll aim to land this in 1.11.3 as the 1.11.2-rc is going out today and more testing is needed here, so we don't need to worry about backporting this to the 1.11.2 release.

@raulk raulk added the team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs label Aug 17, 2021
@nonsense
Copy link
Member

I validated this on sofiaminer (with nethogs) with a lotus-worker process alongside a lotus-miner (which does no sealing operations), and confirmed that we are no longer seeing IP traffic between markets process and lotus-miner, when there is a worker involved on AddPiece.

@magik6k magik6k merged commit 5bdc186 into master Aug 20, 2021
@magik6k magik6k deleted the feat/jsonrpc-redir branch August 20, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 P2: Should be resolved team/ignite Issues and PRs being tracked by Team Ignite at Protocol Labs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support direct writing to workers from markets process
7 participants