etcd-dump-logs: add entry-type flag to list entries of specific types… #9628

wenjiaswe · 2018-04-25T21:48:31Z

etcd-dump-logs: add entry-type flag to list entries of specific types and add test

Added "entry-type" flag so user could use the tool to just list and count one or more types of interested entries. If not set, all entries are listed.
Added test as well.

Fixes #9295

@jpbetz @gyuho

gyuho

Some minor nits. Otherwise, LGTM.
Defer to @jpbetz

Thanks!

gyuho · 2018-04-25T22:02:35Z

tools/etcd-dump-logs/main.go

+	var r etcdserverpb.Request
+	if err := r.Unmarshal(entry.Data); err == nil {
+		fmt.Printf("%4d\t%10d\tnorm", entry.Term, entry.Index)
+		//cnt ++


unused comment?

gyuho · 2018-04-25T22:11:42Z

tools/etcd-dump-logs/etcd-dump-log_test.go

+	"go.uber.org/zap"
+)
+
+func TestEtcdDumpLogEntryType(t *testing.T) {


Check at the beginning of tests, if etcd-dump-logs exists?

See https://github.com/coreos/etcd/blob/62a385b1ca2560847ec5b82ec0b40253320fd4e1/tests/e2e/etcd_release_upgrade_test.go#L32-L35

gyuho · 2018-04-25T22:15:34Z

I tried this, and very useful. Would be great if we can add Compaction, LeaseGrant, and LeaseRevoke since we've recently hit quite a few issues around lease.

Thanks!

wenjiaswe · 2018-04-25T22:31:40Z

@gyuho Thanks for your prompt review! I will apply your comments now.

wenjiaswe · 2018-04-25T23:24:27Z

@gyuho Are these (Compaction, LeaseGrant, and LeaseRevoke) recorded in WAL log?

gyuho · 2018-04-25T23:51:13Z

@wenjiaswe Lease has a separate storage layer. I was confused with dump db tool, so never mind.

Update: lease grants are recorded in WAL since they are processed by Raft layer. But we can add this later.

wenjiaswe · 2018-04-25T23:58:29Z

@gyuho just make sure I didn't miss anything here:) db might be another area we will would like to analyse anyway. I am willing to fix that in etcd-dump-db later as well.

For this PR, I have fixed the other two comments you had. Does that mean this is ready for merge once all the test passed?

gyuho · 2018-04-25T23:59:55Z

tools/etcd-dump-logs/etcd-dump-log_test.go

+			cmd := exec.Command(dumpLogsBinary, argtest.args...)
+			actual, err := cmd.CombinedOutput()
+			if err != nil {
+				t.Fatal("%v", err)


t.Fatalf?

Good catch, fixed.

gyuho · 2018-04-26T00:00:00Z

tools/etcd-dump-logs/etcd-dump-log_test.go

+			}
+			expected, err := ioutil.ReadFile(path.Join(binDir, argtest.fileExpected))
+			if err != nil {
+				t.Fatal("%v", err)


t.Fatalf?

Or just t.Fatal(err)

gyuho · 2018-04-26T00:01:38Z

@wenjiaswe Yeah let's improve dump db tool later.

Just a few minor nits, and should be ready to merge.

Thanks.

xiang90 · 2018-04-26T04:27:00Z

tools/etcd-dump-logs/expectedoutput/listAll.output

+   6	        12	norm	ID:7 delete_range:<key:"0" range_end:"9" prev_kv:true > 
+   7	        13	norm	ID:8 txn:<success:<request_delete_range:<key:"a" range_end:"b" > > failure:<request_delete_range:<key:"a" range_end:"b" > > > 
+
+Entry types () count is : 13


hmm... ignore this. just noticed that this is testing output data.

xiang90 · 2018-04-26T04:27:10Z

tools/etcd-dump-logs/expectedoutput/listNormal.output

+   6	        12	norm	ID:7 delete_range:<key:"0" range_end:"9" prev_kv:true > 
+   7	        13	norm	ID:8 txn:<success:<request_delete_range:<key:"a" range_end:"b" > > failure:<request_delete_range:<key:"a" range_end:"b" > > > 
+
+Entry types (Normal) count is : 9


gyuho · 2018-04-26T15:44:54Z

tools/etcd-dump-logs/main.go

+
+//  listEntriesType filters and prints entries based on the entry-type flag,
+func listEntriesType(entrytype string, ents []raftpb.Entry) {
+	entryFilters := evaluateEntrytypeFlag(entrytype)


Can evaluateEntrytypeFlag handle entrytype == ""?

I double-checked our code, and lease grant does get recorded in WAL. And etcd-dump-logs ./data-dir with no --entry-type flag would omit those entries, while current master branch etcd-dump-logs tool prints them out. If --entry-type is empty, it should prints out all entries by default (e.g. lease grant, revoke, etc.).

Thanks!

Yes, you are right! Thanks for the catch! I have fixed that and commented in place.

wenjiaswe · 2018-04-27T00:39:49Z

tools/etcd-dump-logs/etcd-dump-log_test.go

+	if !fileutil.Exist(dumpLogsBinary) {
+		t.Skipf("%q does not exist", dumpLogsBinary)
+	}
+


@gyuho Here I added the check to see if etcd-dump-logs exists.

wenjiaswe

@gyuho I have addressed the review comments. Thanks!

wenjiaswe · 2018-04-27T00:43:51Z

tools/etcd-dump-logs/etcd-dump-log_test.go

+		{"lease grant entry-type", []string{"-entry-type", "IRRLeaseGrant", p}, "expectedoutput/listIRRLeaseGrant.output"},
+		{"lease revoke entry-type", []string{"-entry-type", "IRRLeaseRevoke", p}, "expectedoutput/listIRRLeaseRevoke.output"},
+		{"confchange and txn entry-type", []string{"-entry-type", "ConfigChange,IRRCompaction", p}, "expectedoutput/listConfigChangeIRRCompaction.output"},
+	}


@gyuho

Thank you for catching the regression that when no entry-type is defined, the tool did not list all the entries. Now I have fixed that and added all types of InternalRaftRequest and made sure that no entries are missing in cases like: 1. no entry-type defined, 2. -entry-type Normal, 3. -entry-type InternalRaftRequest.

I added filters for compaction, leasegrant and leaserevoke types and added tests for those.

wenjiaswe · 2018-04-27T00:44:39Z

tools/etcd-dump-logs/etcd-dump-log_test.go

+		{ID: 27, AuthRoleGrantPermission: irrauthrolegrantpermission},
+		{ID: 28, AuthRoleRevokePermission: irrauthrolerevokepermission},
+	}
+


Here I added all internalraftrequest types for testing.

wenjiaswe · 2018-04-27T00:45:35Z

tools/etcd-dump-logs/etcd-dump-log_test.go

+	currentry.Type = raftpb.EntryNormal
+	currentry.Data = []byte("?")
+	*ents = append(*ents, currentry)
+}


I added the check to make sure unknown normal entries are listed as well, like the behavior of the tool before my change.

wenjiaswe · 2018-04-27T00:46:33Z

tools/etcd-dump-logs/main.go

+	var rr etcdserverpb.InternalRaftRequest
+	return entry.Type == raftpb.EntryNormal && rr.Unmarshal(entry.Data) == nil && rr.LeaseRevoke != nil, "InternalRaftRequest"
+}
+


Here I added filters for Compaction, LeaseGrant, and LeaseRevoke. More could be added in future if needed.

wenjiaswe · 2018-04-27T00:47:18Z

tools/etcd-dump-logs/main.go

+
+//  listEntriesType filters and prints entries based on the entry-type flag,
+func listEntriesType(entrytype string, ents []raftpb.Entry) {
+	entryFilters := evaluateEntrytypeFlag(entrytype)


Yes, you are right! Thanks for the catch! I have fixed that and commented in place.

… and add test

codecov-io · 2018-04-27T06:31:42Z

Codecov Report

Merging #9628 into master will increase coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9628      +/-   ##
==========================================
+ Coverage   69.03%   69.22%   +0.19%     
==========================================
  Files         373      373              
  Lines       34048    34048              
==========================================
+ Hits        23506    23571      +65     
+ Misses       8816     8765      -51     
+ Partials     1726     1712      -14

Impacted Files	Coverage Δ
etcdserver/api/v3rpc/util.go	`69.56% <0%> (-8.7%)`	⬇️
clientv3/namespace/watch.go	`72.72% <0%> (-6.07%)`	⬇️
raft/node.go	`90.83% <0%> (-1.6%)`	⬇️
pkg/transport/listener_tls.go	`65.56% <0%> (-0.67%)`	⬇️
clientv3/watch.go	`96.56% <0%> (-0.25%)`	⬇️
etcdserver/server.go	`71.3% <0%> (+0.37%)`	⬆️
etcdserver/api/v2http/client.go	`90.78% <0%> (+0.44%)`	⬆️
etcdserver/v3_server.go	`77.51% <0%> (+0.47%)`	⬆️
clientv3/balancer/grpc1.7-health.go	`88.08% <0%> (+0.58%)`	⬆️
lease/lessor.go	`87.22% <0%> (+0.63%)`	⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02e2e3d...5533257. Read the comment docs.

gyuho · 2018-04-27T15:44:46Z

Great. I've tried it again, and confirmed that it fixes the issue that we mentioned.

Could you add

### Tooling

- Add [`etcd-dump-logs -entry-type`](https://github.com/coreos/etcd/pull/9628) flag to support WAL log filtering by entry type.

around https://github.com/coreos/etcd/blob/master/CHANGELOG-3.4.md#go so people know who worked on it? A separate commit is fine. Then this should be good to merge.

Thanks!

wenjiaswe · 2018-04-27T17:35:35Z

@gyuho Thanks for review, I have updated changelog in #9650

gyuho

lgtm thanks!

CHANGELOG: update from #9628

jpbetz · 2018-04-27T17:39:36Z

lgtm

Thanks @wenjiaswe !

gyuho suggested changes Apr 25, 2018

View reviewed changes

wenjiaswe mentioned this pull request Apr 25, 2018

WAL analysis tool improvements #9295

Closed

wenjiaswe force-pushed the new1 branch from 078cb44 to 29b3b5f Compare April 25, 2018 23:33

gyuho reviewed Apr 25, 2018

View reviewed changes

gyuho reviewed Apr 26, 2018

View reviewed changes

xiang90 reviewed Apr 26, 2018

View reviewed changes

gyuho reviewed Apr 26, 2018

View reviewed changes

wenjiaswe force-pushed the new1 branch 2 times, most recently from 65d6233 to 8a2b347 Compare April 27, 2018 00:37

wenjiaswe commented Apr 27, 2018

View reviewed changes

wenjiaswe force-pushed the new1 branch from 8a2b347 to 0626a1a Compare April 27, 2018 01:44

etcd-dump-logs: add entry-type flag to list entries of specific types…

5533257

… and add test

wenjiaswe force-pushed the new1 branch from 0626a1a to 5533257 Compare April 27, 2018 02:00

gyuho added the Release-Note label Apr 27, 2018

wenjiaswe added a commit to wenjiaswe/etcd that referenced this pull request Apr 27, 2018

CHANGELOG: update from etcd-io#9628

0015608

wenjiaswe mentioned this pull request Apr 27, 2018

CHANGELOG: update from #9628 #9650

Merged

gyuho approved these changes Apr 27, 2018

View reviewed changes

gyuho added a commit that referenced this pull request Apr 27, 2018

Merge pull request #9650 from wenjiaswe/new1ChangeLog

4a09318

CHANGELOG: update from #9628

gyuho merged commit 7582a28 into etcd-io:master Apr 27, 2018

wenjiaswe mentioned this pull request Jun 13, 2018

CHANGELOG: update from #9845 #9846

Merged

wenjiaswe deleted the new1 branch June 19, 2018 22:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcd-dump-logs: add entry-type flag to list entries of specific types… #9628

etcd-dump-logs: add entry-type flag to list entries of specific types… #9628

wenjiaswe commented Apr 25, 2018

gyuho left a comment

gyuho Apr 25, 2018

gyuho Apr 25, 2018

gyuho commented Apr 25, 2018

wenjiaswe commented Apr 25, 2018

wenjiaswe commented Apr 25, 2018

gyuho commented Apr 25, 2018 •

edited

Loading

wenjiaswe commented Apr 25, 2018

gyuho Apr 25, 2018

wenjiaswe Apr 26, 2018

gyuho Apr 26, 2018

gyuho Apr 26, 2018

wenjiaswe Apr 26, 2018

gyuho commented Apr 26, 2018

xiang90 Apr 26, 2018

xiang90 Apr 26, 2018

xiang90 Apr 26, 2018

gyuho Apr 26, 2018 •

edited

Loading

wenjiaswe Apr 27, 2018

wenjiaswe Apr 27, 2018

wenjiaswe left a comment

wenjiaswe Apr 27, 2018

wenjiaswe Apr 27, 2018

wenjiaswe Apr 27, 2018

wenjiaswe Apr 27, 2018

wenjiaswe Apr 27, 2018

codecov-io commented Apr 27, 2018

gyuho commented Apr 27, 2018

wenjiaswe commented Apr 27, 2018

gyuho left a comment

jpbetz commented Apr 27, 2018

etcd-dump-logs: add entry-type flag to list entries of specific types… #9628

etcd-dump-logs: add entry-type flag to list entries of specific types… #9628

Conversation

wenjiaswe commented Apr 25, 2018

gyuho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Apr 25, 2018

wenjiaswe commented Apr 25, 2018

wenjiaswe commented Apr 25, 2018

gyuho commented Apr 25, 2018 • edited Loading

wenjiaswe commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenjiaswe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Apr 27, 2018

Codecov Report

gyuho commented Apr 27, 2018

wenjiaswe commented Apr 27, 2018

gyuho left a comment

Choose a reason for hiding this comment

jpbetz commented Apr 27, 2018

gyuho commented Apr 25, 2018 •

edited

Loading

gyuho Apr 26, 2018 •

edited

Loading