fix: restore portable backup error #20115

rickif · 2020-11-20T07:40:25Z

This PR is to solve restore problem in influxdb 1.x

influxd restore -portable -host [host] -db [db] -newdb [newdb] [backup_path]

Restoring portable backup with this command may get error as below

2020/11/20 13:55:02 error updating meta: updating metadata on influxd service failed: err=read tcp x.x.x.x:52286->x.x.x.x:8088: read: connection reset by peer, n=16
restore: updating metadata on fctsdb service failed: err=read tcp x.x.x.x:52286->x.x.x.x:8088: read: connection reset by peer, n=16

Accordingly, the influxdb error log is as below

ts=2020-11-20T13:55:02.555262Z lvl=info msg="failed to decode meta: proto: meta.ShardInfo: illegal tag 0 (wire type 0)" log_id=0QQzP3qG000 service=snapshot

This problem happens at random, but more frequently in network with high latency. The net.func (*TCPConn) Read dose not promise that exactly len(buf) bytes is read when the call is returned, which leading this problem. Replacing the net.func (*TCPConn) Read with io.ReadFull could fix this.

CHANGELOG.md updated with a link to the PR (not the Issue)
Well-formatted commit messages
Rebased/mergeable
Tests pass
http/swagger.yml updated (if modified Go structs or API)
Feature flagged (if modified API)
Documentation updated or issue created (provide link to issue/pr)
Signed CLA (if not already signed)

danxmoran · 2020-11-20T15:09:56Z

Hi @rickif thanks for the contribution! While we review, could you push an update to the CHANGELOG.md file with a reference to this PR?

rickif · 2020-11-21T06:59:28Z

@danxmoran OK, the CHANGELOG.md is updated now.

danxmoran

@rickif this change makes sense to me in theory from reading the docs on Read and ReadFull, but I'm not sure what to expect in practice from the use-case you mention (restoring on a high-latency network). In situations where Read used to return "early", will ReadFull now sit and wait for more data? Or will it still result in an error, but the error will be easier to understand?

Do you feel up to try adding tests to services/snapshotter/service.go? The 2 cases I'd ideally want covered are:

conn closes before ReadFull reads enough bytes to fill typ[:]
conn doesn't "contain" enough bytes to fill typ[:] when ReadFull is called, but more data arrives afterwards

rickif · 2020-11-24T11:13:16Z

@danxmoran I agree with u and have added some test codes .But I have doubt about the cases you mentioned.

Is it necessary to cover the first case? According to the godoc, If an EOF happens after reading some but not all the bytes, ReadFull returns ErrUnexpectedEOF. The result is clear and well-defined by golang.
On Linux platform, the call to read() may read less than the requested number of bytes, which may occur if there are fewer bytes available in the socket than were requested in the read() call. The possible cause of this is happened in the Linux Kernel Mode, which is not easy to add test to simulate this circumstance. I tried to do this by dividing the payload and sending the pieces one by one, meanwhile Sleep() is added after every sending. But it does not work at all. I wonder whether you have any good idea about this?

danxmoran · 2020-11-24T14:29:49Z

Is it necessary to cover the first case? According to the godoc, If an EOF happens after reading some but not all the bytes, ReadFull returns ErrUnexpectedEOF. The result is clear and well-defined by golang.

A test case covering only a call to ReadFull would indeed be redundant. The pieces I'm interested in covering are the functions wrapping the ReadFull calls. My impression is that before this change, those functions would either complete successfully (but return partial/corrupted data) or fail with a confusing error. The functions still fail with this change, but with (hopefully) a more clear explanation in the error. If it's easy to capture the new expectation in a test, it'd help guard against future regressions.

That said, I don't want to gatekeep your contribution for too long. If setting up test cases is going to take a significant amount of mocking & other test machinery, I'm ok merging without additional tests. In that case, could you add a comment above each of the ReadFull calls explaining why ReadFull over Read?

danxmoran · 2020-11-24T14:31:17Z

But it does not work at all. I wonder whether you have any good idea about this?

It sounds like this will be more trouble than it's worth, I wouldn't spend more time on it. Thanks for investigating!

rickif · 2020-11-24T15:16:37Z

The functions still fail with this change, but with (hopefully) a more clear explanation in the error

This change is not aiming at providing a more clear explanation, but is mainly to solve the problem that the conn.Read() may return too early without enough data, which leds a data corruption error even if the data is valid. As you mentioned before, the io.ReadFull would block until expected data is read, in which the data could be parsed correctly.

danxmoran

Thanks @rickif, one suggestion to curate the CHANGELOG, and some questions in the new test code.

CHANGELOG.md

danxmoran · 2020-11-30T15:32:41Z

services/snapshotter/service_test.go

+	}
+
+	c := snapshotter.NewClient(l.Addr().String())
+	if _, err := c.UpdateMeta(req, bytes.NewReader(metaBytes)); err != nil {


Is there anything useful we could assert about the success case here?

Yeah, I've add some codes to cover the success case

services/snapshotter/service_test.go

rickif · 2020-12-02T14:54:43Z

Thanks @rickif, one suggestion to curate the CHANGELOG, and some questions in the new test code.

@danxmoran Thanks for suggestions. The PR has been updated!

danxmoran

LGTM!

rickif · 2020-12-30T10:53:51Z

@danxmoran Hi, the PR has been pending for a long time. Is there any problem?

rickif · 2021-04-01T02:23:57Z

@dgnorton Hi, would you please do review for this PR?

danxmoran · 2021-07-29T19:50:44Z

@rickif very sorry for the delay, I'm not sure how I lost track of this one. I'll do a final review pass now.

rickif · 2021-07-30T09:34:45Z

@rickif very sorry for the delay, I'm not sure how I lost track of this one. I'll do a final review pass now.

that would very nice.

lesam · 2021-07-30T18:00:28Z

Re-opened this PR as #21991 so that I can update the changelog, otherwise it is good.

rickif force-pushed the fix/restore-portable-backup branch from 2a56a3a to 69e2054 Compare November 20, 2020 07:47

danxmoran requested review from danxmoran and dgnorton November 20, 2020 14:27

rickif force-pushed the fix/restore-portable-backup branch from 69e2054 to 8f961fa Compare November 21, 2020 06:56

rickif changed the title ~~fix: restore portable backup failed~~ fix: restore portable backup error Nov 21, 2020

danxmoran reviewed Nov 23, 2020

View reviewed changes

rickif force-pushed the fix/restore-portable-backup branch from 409486a to e7ef85d Compare November 24, 2020 10:06

rickif force-pushed the fix/restore-portable-backup branch from 43a6e42 to ce0d052 Compare November 24, 2020 15:19

danxmoran reviewed Nov 30, 2020

View reviewed changes

rickif force-pushed the fix/restore-portable-backup branch from af50c02 to 1336bc0 Compare December 2, 2020 14:42

danxmoran previously approved these changes Dec 2, 2020

View reviewed changes

danxmoran mentioned this pull request Dec 2, 2020

Portable backup can fail with data corruption on high-latency networks [forward-port 1.x] #20231

Closed

rickif added 3 commits April 1, 2021 10:14

fix: read exact length data when restore meta

acc8a0d

test: cover updating meta

7ed747d

test: replace metaclient's data with pointer

671aaa7

rickif force-pushed the fix/restore-portable-backup branch 2 times, most recently from 2e38acd to 67bb4d3 Compare April 1, 2021 02:21

rickif force-pushed the fix/restore-portable-backup branch from 67bb4d3 to 671aaa7 Compare April 1, 2021 02:42

rickif added 2 commits April 1, 2021 10:47

docs: update change log

561622f

test: remove extra code

1aa3707

rickif force-pushed the fix/restore-portable-backup branch from df29ce3 to 1aa3707 Compare April 1, 2021 02:53

danxmoran dismissed their stale review via 1aa3707 July 28, 2021 00:37

lesam mentioned this pull request Jul 29, 2021

fix: restore portable backup #21978

Merged

4 tasks

lesam self-requested a review July 29, 2021 21:17

lesam closed this Jul 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: restore portable backup error #20115

fix: restore portable backup error #20115

rickif commented Nov 20, 2020 •

edited

Loading

danxmoran commented Nov 20, 2020

rickif commented Nov 21, 2020

danxmoran left a comment

rickif commented Nov 24, 2020

danxmoran commented Nov 24, 2020

danxmoran commented Nov 24, 2020

rickif commented Nov 24, 2020

danxmoran left a comment

danxmoran Nov 30, 2020

rickif Dec 2, 2020 •

edited

Loading

rickif commented Dec 2, 2020 •

edited

Loading

danxmoran left a comment

rickif commented Dec 30, 2020

rickif commented Apr 1, 2021

danxmoran commented Jul 29, 2021

rickif commented Jul 30, 2021

lesam commented Jul 30, 2021

fix: restore portable backup error #20115

fix: restore portable backup error #20115

Conversation

rickif commented Nov 20, 2020 • edited Loading

danxmoran commented Nov 20, 2020

rickif commented Nov 21, 2020

danxmoran left a comment

Choose a reason for hiding this comment

rickif commented Nov 24, 2020

danxmoran commented Nov 24, 2020

danxmoran commented Nov 24, 2020

rickif commented Nov 24, 2020

danxmoran left a comment

Choose a reason for hiding this comment

danxmoran Nov 30, 2020

Choose a reason for hiding this comment

rickif Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

rickif commented Dec 2, 2020 • edited Loading

danxmoran left a comment

Choose a reason for hiding this comment

rickif commented Dec 30, 2020

rickif commented Apr 1, 2021

danxmoran commented Jul 29, 2021

rickif commented Jul 30, 2021

lesam commented Jul 30, 2021

rickif commented Nov 20, 2020 •

edited

Loading

rickif Dec 2, 2020 •

edited

Loading

rickif commented Dec 2, 2020 •

edited

Loading