remove deprecated `Drain` field from `structs.Node` #10202

cgbaker · 2021-03-19T21:47:55Z

Note: upgrade testing on this PR should verify that .SecretID is not emitted by the API (using curl, not the Nomad CLI), and that .Drain is emitted.

This is an attempt to remove the Drain field from structs.Node, while still keeping backwards compatibility with the api.Node.Drain field. The questions at play are:

do we want to actually delete this field?
do we like this JSON encoding extensions approach for API backwards compatibility?

Here's what the PR does:

Remove deprecated Drain field from structs/Node and deleted code/tests associated with this field.
Adds encoding extensions to put the Node.Drain field back to API responses
Modifies the HTTP agent and the event stream to use encoders with these extensions
Also removed special code for emptying structs.Node.SecretID, instead allowing the encoder to remove it using a json tag on the struct, which works because (:til:) we have configured msgpack encoding for raft to exclusively consider codec tags
Updates the Node drain FSM applier to use msgtype on txn so that drain events are emitted
Updates the contributing guide with all this, to guide folks in the future on structs-to-api options.

I added some benchmarking to see whether the JSON encoding extensions were expensive, with the following results:

BenchmarkHTTPServer_JSONEncodingWithExtensions-12       	   92414	     12141 ns/op
BenchmarkHTTPServer_JSONEncodingWithoutExtensions-12    	   94684	     11569 ns/op

So, 600ns cost with the extensions. Do with that what you will. Note, this doesn't compare against doing a struct-to-API conversion as part of encoding (because I didn't want to write it). Also, it's possible that this is slower because of neglected generated structs; I didn't investigate because of https://m.xkcd.com/1319/

nomad/state/events_test.go

node drain: use msgtype on txn so that events are emitted wip: encoding extension to add Node.Drain field back to API responses new approach for hiding Node.SecretID in the API, using `json` tag documented this approach in the contributing guide refactored the JSON handlers with extensions modified event stream encoding to use the go-msgpack encoders with the extensions

initialization

cgbaker · 2021-03-23T17:59:39Z

nomad/state/events_test.go

@@ -94,57 +94,6 @@ func TestEventFromChange_ACLTokenSecretID(t *testing.T) {
 	require.Empty(t, tokenEvent2.ACLToken.SecretID)
 }

-// TestEventFromChange_NodeSecretID ensures that a node's secret ID is not


this check is provided in a new test:
https://github.com/hashicorp/nomad/pull/10202/files#diff-4dd5ab69dbb20720939d5d0edc23108a3b6427943ff1b71f2fea90a79481c80dR168-R169

along with a sibling test for /v1/node/:id provided here:
https://github.com/hashicorp/nomad/pull/10202/files#diff-0acc6b10aaabc14cceeb1cbb9b9ded8935451d1f6b4514ee1806b5c9af1ff252R181

cgbaker · 2021-03-23T18:00:10Z

nomad/state/state_store.go

@@ -966,9 +966,10 @@ func (s *StateStore) BatchUpdateNodeDrain(msgType structs.MessageType, index uin
 // UpdateNodeDrain is used to update the drain of a node
 func (s *StateStore) UpdateNodeDrain(msgType structs.MessageType, index uint64, nodeID string, drain *structs.DrainStrategy, markEligible bool, updatedAt int64, event *structs.NodeEvent) error {

-	txn := s.db.WriteTxn(index)
+	txn := s.db.WriteTxnMsgT(msgType, index)


this was preventing Node drain completion from firing an event

contributing/checklist-jobspec.md

cgbaker · 2021-03-26T17:20:15Z

nomad/plan_apply.go

-	} else if node.Drain {
-		// Deprecate in favor of scheduling eligibility and remove post-0.8
-		return false, "node is draining", nil
+		return false, "node is not eligible", nil


changed this error message because it seemed wrong. is that allowed? @schmichael , @tgross

Good catch! Your message looks right to me.

cgbaker · 2021-03-26T19:01:25Z

nomad/structs/structs.go

-		}
+	// Ensure SchedulingEligibility is correctly set whenever draining so the plan applier and other scheduling logic
+	// only need to check SchedulingEligibility when determining whether a placement is feasible on a node.
+	if n.DrainStrategy != nil {


@schmichael , added a comment as requested and tightened this up to make sure we honor the invariant.

cgbaker · 2021-04-01T20:42:50Z

@schmichael , i rolled back the suggested changes around RPC and Node.SecretID. I also cleaned up some code and comments. This PR should be ready to go, if you'll re-review! Thanks!

schmichael

Looks great!

this was unecessary after refactoring, so this moves them back to their original location in package structs

The CSI HTTP API has to transform the CSI volume to redact secrets, remove the claims fields, and to consolidate the allocation stubs into a single slice of alloc stubs. This was done manually in #8590 but this is a large amount of code and has proven both very bug prone (see #8659, #8666, #8699, #8735, and #12150) and requires updating lots of code every time we add a field to volumes or plugins. In #10202 we introduce encoding improvements for the `Node` struct that allow a more minimal transformation. Apply this same approach to serializing `structs.CSIVolume` to API responses. Also, the original reasoning behind #8590 for plugins no longer holds because the counts are now denormalized within the state store, so we can simply remove this transformation entirely.

github-actions · 2022-11-26T02:19:21Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

cgbaker changed the title ~~remove deprecated fields in structs.Node~~ WIP: remove deprecated fields in structs.Node Mar 19, 2021

cgbaker force-pushed the f-node-drain-api branch from 3a0e029 to 58bda09 Compare March 20, 2021 09:03

vercel bot temporarily deployed to Preview – nomad March 20, 2021 09:03 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 20, 2021 09:03 View deployment

cgbaker force-pushed the f-node-drain-api branch from 58bda09 to 379281c Compare March 20, 2021 09:36

vercel bot temporarily deployed to Preview – nomad March 20, 2021 09:37 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 20, 2021 09:37 View deployment

cgbaker force-pushed the f-node-drain-api branch from 379281c to 18194d6 Compare March 21, 2021 02:26

vercel bot deployed to Preview – nomad-storybook-and-ui March 21, 2021 02:26 View deployment

vercel bot temporarily deployed to Preview – nomad March 21, 2021 02:27 Inactive

cgbaker commented Mar 21, 2021

View reviewed changes

nomad/state/events_test.go Outdated Show resolved Hide resolved

cgbaker commented Mar 21, 2021

View reviewed changes

nomad/state/events_test.go Outdated Show resolved Hide resolved

cgbaker force-pushed the f-node-drain-api branch from 18194d6 to c93094d Compare March 21, 2021 13:12

vercel bot temporarily deployed to Preview – nomad March 21, 2021 13:12 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 21, 2021 13:12 View deployment

cgbaker added 2 commits March 21, 2021 15:30

updated node drain API test

574d26c

cgbaker force-pushed the f-node-drain-api branch from c93094d to 574d26c Compare March 21, 2021 15:30

vercel bot deployed to Preview – nomad-storybook-and-ui March 21, 2021 15:30 View deployment

vercel bot temporarily deployed to Preview – nomad March 21, 2021 15:30 Inactive

refactor?

ca51bd9

vercel bot temporarily deployed to Preview – nomad March 22, 2021 01:49 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2021 01:49 View deployment

moved JSON handlers and extension code around a bit for proper order of

0cd707e

initialization

vercel bot deployed to Preview – nomad-storybook-and-ui March 22, 2021 14:13 View deployment

vercel bot temporarily deployed to Preview – nomad March 22, 2021 14:13 Inactive

vercel bot temporarily deployed to Preview – nomad March 23, 2021 17:57 Inactive

vercel bot deployed to Preview – nomad-storybook-and-ui March 23, 2021 17:57 View deployment

cgbaker commented Mar 23, 2021

View reviewed changes

vercel bot deployed to Preview – nomad-storybook-and-ui March 26, 2021 17:10 View deployment

vercel bot temporarily deployed to Preview – nomad March 26, 2021 17:10 Inactive

cgbaker commented Mar 26, 2021

View reviewed changes

contributing/checklist-jobspec.md Outdated Show resolved Hide resolved

Update contributing/checklist-jobspec.md

e9c552b

vercel bot deployed to Preview – nomad-storybook-and-ui March 26, 2021 17:13 View deployment

vercel bot temporarily deployed to Preview – nomad March 26, 2021 17:13 Inactive

cgbaker commented Mar 26, 2021

View reviewed changes

reworked Node.Canonicalize() to enforce invariants, fixed a broken test

ed636ad

vercel bot deployed to Preview – nomad-storybook-and-ui March 26, 2021 18:58 View deployment

vercel bot temporarily deployed to Preview – nomad March 26, 2021 18:58 Inactive

cgbaker commented Mar 26, 2021

View reviewed changes

cgbaker requested a review from schmichael March 26, 2021 19:01

cgbaker mentioned this pull request Mar 29, 2021

Node Drain Metadata #10250

Merged

Merge branch 'main' into f-node-drain-api

80066ac

vercel bot deployed to Preview – nomad-storybook-and-ui April 1, 2021 20:23 View deployment

vercel bot deployed to Preview – nomad April 1, 2021 20:23 View deployment

schmichael approved these changes Apr 1, 2021

View reviewed changes

cgbaker merged commit f1cb714 into main Apr 1, 2021

cgbaker deleted the f-node-drain-api branch April 1, 2021 21:48

cgbaker added a commit that referenced this pull request Apr 2, 2021

json handles were moved to a new package in #10202

cb3d6ec

this was unecessary after refactoring, so this moves them back to their original location in package structs

cgbaker mentioned this pull request Apr 2, 2021

move json handle back to structs #10290

Merged

This was referenced Apr 29, 2021

replace structs.CSIPlugin -> api.CSIPlugin conversion #10470

Closed

replace api.Job -> structs.Job conversion #10471

Open

identify nomad/structs -> api/structs conversions #10472

Open

tgross mentioned this pull request Apr 15, 2022

CSI: replace structs->api with serialization extension #12583

Merged

github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove deprecated `Drain` field from `structs.Node` #10202

remove deprecated `Drain` field from `structs.Node` #10202

cgbaker commented Mar 19, 2021 •

edited

cgbaker Mar 23, 2021

cgbaker Mar 23, 2021

cgbaker Mar 26, 2021

schmichael Apr 1, 2021

cgbaker Mar 26, 2021

cgbaker commented Apr 1, 2021 •

edited

schmichael left a comment

github-actions bot commented Nov 26, 2022

remove deprecated Drain field from structs.Node #10202

remove deprecated Drain field from structs.Node #10202

Conversation

cgbaker commented Mar 19, 2021 • edited

cgbaker Mar 23, 2021

Choose a reason for hiding this comment

cgbaker Mar 23, 2021

Choose a reason for hiding this comment

cgbaker Mar 26, 2021

Choose a reason for hiding this comment

schmichael Apr 1, 2021

Choose a reason for hiding this comment

cgbaker Mar 26, 2021

Choose a reason for hiding this comment

cgbaker commented Apr 1, 2021 • edited

schmichael left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 26, 2022

remove deprecated `Drain` field from `structs.Node` #10202

remove deprecated `Drain` field from `structs.Node` #10202

cgbaker commented Mar 19, 2021 •

edited

cgbaker commented Apr 1, 2021 •

edited