server,storage,sql: record node {de,re}commissioning in the event log #18178

benesch · 2017-09-02T04:42:25Z

/cc @a-robinson @tschottdorf can one or both of you review? This wasn't quite as simple as I'd hoped. The naive solution spams the event log with duplicate entries because ./node decommission FOO fires off many decommissioning requests in quick succession.

Also, note that TestDecommission is currently t.Skipped, so the test won't run until #17995 is fixed. (I did manually verify my addition by replacing the decommission --wait all that hangs forever with a decommission --wait none and verifying that the CheckQueryResults succeeds.)

cockroach-teamcity · 2017-09-02T04:42:33Z

This change is

bdarnell · 2017-09-03T20:15:26Z

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from Reviewable

a-robinson

Nice solution! LGTM, just a few small comments.

a-robinson · 2017-09-04T22:43:24Z

pkg/server/server.go

+					ctx, txn, eventType, int32(nodeID), int32(nodeID), struct{}{},
+				)
+			}); err != nil {
+				log.Errorf(ctx, "unable to record commissioning event for node %d: %s", nodeID, err)


nit, but it'd be nice to include the event type here as well.

a-robinson · 2017-09-04T22:48:31Z

pkg/ui/src/util/eventTypes.ts

@@ -33,11 +33,16 @@ export const FINISH_SCHEMA_CHANGE = "finish_schema_change";
 export const NODE_JOIN = "node_join";
 // Recorded when an existing node rejoins the cluster after being offline.
 export const NODE_RESTART = "node_restart";
+// Recorded when a node is marked as decommissioning.
+export const NODE_DECOMMISSIONED = "node_decommissioned";
+// EventLogNodeRecommissioned is recorded when a decommissioned node is


s/EventLogNodeRecommissioned is recorded/Recorded/

Thanks. Done.

a-robinson · 2017-09-04T22:52:08Z

pkg/server/server.go

+			// than to slow down future node liveness transactions.
+			if err := s.db.Txn(ctx, func(ctx context.Context, txn *client.Txn) error {
+				return eventLogger.InsertEventRecord(
+					ctx, txn, eventType, int32(nodeID), int32(nodeID), struct{}{},


The reporting argument shouldn't be the nodeID being commissioned, it should be s.NodeID().

Nice catch! Done.

tbg · 2017-09-05T01:45:01Z

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

pkg/server/server.go, line 1172 at r1 (raw file):

			// If we die right now or if this transaction fails to commit, the
			// commissioning event will not be recorded to the event log. While we
			// could insert the event record in the same transaction as the liveness

There's also the problem that intents on the node liveness range are not allowed. Not 100% sure that's true, though I think I remember it.

pkg/ui/src/views/cluster/containers/events/index.tsx, line 110 at r1 (raw file):

      break;
    case eventTypes.NODE_DECOMMISSIONED:
      content = <span>Node Decommissioned: Node {targetId} was decommissioned</span>;

The message seems oddly repetitive, but apparently that's true for its neighbors as well.

Comments from Reviewable

benesch · 2017-09-05T13:46:32Z

Review status: 8 of 10 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

pkg/server/server.go, line 1172 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

There's also the problem that intents on the node liveness range are not allowed. Not 100% sure that's true, though I think I remember it.

Is that enforced somewhere other than this block of code?

cockroach/pkg/storage/node_liveness.go

Lines 647 to 660 in d65ed58

    
           // Use a trigger on EndTransaction to indicate that node liveness should 
        
           // be re-gossiped. Further, require that this transaction complete as a 
        
           // one phase commit to eliminate the possibility of leaving write intents. 
        
           b.AddRawRequest(&roachpb.EndTransactionRequest{ 
        
           	Commit:     true, 
        
           	Require1PC: true, 
        
           	InternalCommitTrigger: &roachpb.InternalCommitTrigger{ 
        
           		ModifiedSpanTrigger: &roachpb.ModifiedSpanTrigger{ 
        
           			NodeLivenessSpan: &roachpb.Span{ 
        
           				Key:    key, 
        
           				EndKey: key.Next(), 
        
           			}, 
        
           		}, 
        
           	},

pkg/ui/src/views/cluster/containers/events/index.tsx, line 110 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

The message seems oddly repetitive, but apparently that's true for its neighbors as well.

Yeah, I find the event log in its current form to be rather useless, but ¯_(ツ)_/¯

Comments from Reviewable

tbg · 2017-09-05T14:17:49Z

Review status: 8 of 10 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

pkg/server/server.go, line 1172 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Is that enforced somewhere other than this block of code?

cockroach/pkg/storage/node_liveness.go

Lines 647 to 660 in d65ed58

// Use a trigger on EndTransaction to indicate that node liveness should

// be re-gossiped. Further, require that this transaction complete as a

// one phase commit to eliminate the possibility of leaving write intents.

b.AddRawRequest(&roachpb.EndTransactionRequest{

Commit: true,

Require1PC: true,

InternalCommitTrigger: &roachpb.InternalCommitTrigger{

ModifiedSpanTrigger: &roachpb.ModifiedSpanTrigger{

NodeLivenessSpan: &roachpb.Span{

Key: key,

EndKey: key.Next(),

},

},

},

Yep, that's the location.

Comments from Reviewable

Fixes cockroachdb#17677.

benesch · 2017-09-05T15:40:00Z

Ok, merging before someone touches embedded.go!

Review status: 8 of 11 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

pkg/server/server.go, line 1172 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Yep, that's the location.

Let me know if you have a rewording for this comment, @tschottdorf. It seems correct to me, even with the above comment in mind.

Comments from Reviewable

tbg · 2017-09-05T17:34:20Z

Review status: 8 of 11 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

pkg/server/server.go, line 1172 at r1 (raw file):

Previously, benesch (Nikhil Benesch) wrote…

Let me know if you have a rewording for this comment, @tschottdorf. It seems correct to me, even with the above comment in mind.

It's incorrect in that it suggests that it would slow down node liveness when in fact it would break it. But don't think that matters here.
See #14250 (comment) for archaeology.

Comments from Reviewable

The new bits of the test introduced in cockroachdb#18178 were flaky since RPCs may be sent to the down nodes.

benesch requested review from a team September 2, 2017 04:42

benesch force-pushed the commissioning-events branch from 464cf70 to 0d72faa Compare September 2, 2017 05:10

a-robinson approved these changes Sep 4, 2017

View reviewed changes

benesch force-pushed the commissioning-events branch from 0d72faa to ecc8dd2 Compare September 5, 2017 13:41

server,storage,sql: record node {de,re}commissioning in the event log

2da77e9

Fixes cockroachdb#17677.

benesch force-pushed the commissioning-events branch from ecc8dd2 to 2da77e9 Compare September 5, 2017 14:27

benesch merged commit 2f476ff into cockroachdb:master Sep 5, 2017

benesch deleted the commissioning-events branch September 5, 2017 15:40

tbg added a commit to tbg/cockroach that referenced this pull request Sep 6, 2017

acceptance: deflake decommissioning event log test

d037599

The new bits of the test introduced in cockroachdb#18178 were flaky since RPCs may be sent to the down nodes.

tbg mentioned this pull request Sep 6, 2017

acceptance: de-flake TestDecommission #18220

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server,storage,sql: record node {de,re}commissioning in the event log #18178

server,storage,sql: record node {de,re}commissioning in the event log #18178

benesch commented Sep 2, 2017 •

edited

Loading

cockroach-teamcity commented Sep 2, 2017

bdarnell commented Sep 3, 2017

a-robinson left a comment

a-robinson Sep 4, 2017

benesch Sep 5, 2017

a-robinson Sep 4, 2017

benesch Sep 5, 2017

a-robinson Sep 4, 2017

benesch Sep 5, 2017

tbg commented Sep 5, 2017

benesch commented Sep 5, 2017

tbg commented Sep 5, 2017

benesch commented Sep 5, 2017

tbg commented Sep 5, 2017

server,storage,sql: record node {de,re}commissioning in the event log #18178

server,storage,sql: record node {de,re}commissioning in the event log #18178

Conversation

benesch commented Sep 2, 2017 • edited Loading

cockroach-teamcity commented Sep 2, 2017

bdarnell commented Sep 3, 2017

a-robinson left a comment

Choose a reason for hiding this comment

a-robinson Sep 4, 2017

Choose a reason for hiding this comment

benesch Sep 5, 2017

Choose a reason for hiding this comment

a-robinson Sep 4, 2017

Choose a reason for hiding this comment

benesch Sep 5, 2017

Choose a reason for hiding this comment

a-robinson Sep 4, 2017

Choose a reason for hiding this comment

benesch Sep 5, 2017

Choose a reason for hiding this comment

tbg commented Sep 5, 2017

benesch commented Sep 5, 2017

tbg commented Sep 5, 2017

benesch commented Sep 5, 2017

tbg commented Sep 5, 2017

benesch commented Sep 2, 2017 •

edited

Loading