Add RPC modifications to include the lock work #17697

Juanadelacuesta · 2023-06-23T12:11:58Z

There are no tests in this PR because some parts of the core implementation are missing, they will be added further on.

Related: #17449
Targets: feature branch

helper/raftutil/msgtypes.go

jrasell

Thanks for taking this work on @Juanadelacuesta, I've given it an initial high level pass with some inline comments as well as those below.

I think we will need different validation logic for the acquire/release lock. When attempting to acquire a lock, we would need to check whether the lock is held or not, before continuing with any potential state change. Releasing, would need to at least ensure the caller is the holder of the lock, before allowing it to continue. There are probably other caveats in this that I am missing.

There would also need to validation on the renew, to ensure the caller is the holder of the lock. I have made some additional notes about the renewal which might help decide how this implementation would look.

Give me a shout if you want to go over any of this in more detail.

nomad/structs/structs.go

nomad/structs/variables.go

nomad/variables_endpoint.go

command/agent/variable_endpoint.go

…onse

…d add testing

…oid panics

…void panics

tgross

LGTM! I've left a few comments but nothing super-blocking, especially as this is going to a feature branch and we can review the whole when we're done.

One recommendation in terms of reviewability for future PRs: this is a huge PR but a lot of the diffs are refactoring by moving large blocks of code around. That's fine! But it would help reviewing if you separate the refactoring into separate commits in the same PR rather than mingling it all together. That way reviewers can focus on the important changes.

nomad/variables_endpoint.go

tgross · 2023-07-31T14:44:18Z

nomad/variables_endpoint.go

 	reply.Index = encryptedVar.ModifyIndex
 	return nil
 }
+
+func isVarLocked(req *structs.VariablesApplyRequest, respVarMeta *structs.VariableMetadata) bool {


If I'm understanding correctly, this is really checking if the variable is locked by someone else. The name of this method is a little confusing because we already have a method on Variable to check if the variable is locked.

tgross · 2023-07-31T14:49:50Z

nomad/variables_endpoint_test.go

+	t.Run("lock-acquire/read token/new", func(t *testing.T) {
+		must.NotNil(t, svHold)
+		sv := *sv3
+
+		sv.Items["upsert"] = "read"
+		sv.VariableMetadata.Lock = &structs.VariableLock{
+			TTL:       24 * time.Hour,
+			LockDelay: 15 * time.Second,
+		}
+
+		applyReq := structs.VariablesApplyRequest{
+			Op:  structs.VarOpLockAcquire,
+			Var: &sv,
+			WriteRequest: structs.WriteRequest{
+				Region:    "global",
+				AuthToken: readToken.SecretID,
+			},
+		}
+
+		applyResp := new(structs.VariablesApplyResponse)
+		err := msgpackrpc.CallWithCodec(codec, structs.VariablesApplyRPCMethod, &applyReq, &applyResp)
+		must.EqError(t, err, structs.ErrPermissionDenied.Error())
+	})


We moved a bunch of test code around but it looks like we're not adding new tests for the logic around whether or not to show the Lock ID for non-management tokens that successfully lock/release a lock.

If the lock acquire was successful, it will always have the lock ID, and no matter if the release was successful or not, it will never return the lock ID, no matter if the token is management or not. The show or no show is more important for the list and get endpoints and this test is for the apply one. It was more for completing the ACL tests and verifying that it is correctly handled and nothing breaks.

tgross · 2023-07-31T14:54:26Z

nomad/structs/variables.go

+	}
+
+	if aux.TTL != nil {
+		switch v := aux.TTL.(type) {


Does this handle invalid JSON like TTL: [] safely?

it does:

~ % curl -X PUT -H 'Content-Type: application/json' -d '{"Lock": {"TTL":[], "LockDelay":"10s"}}' "http://localhost:4646/v1/var/nomad/jobs/blah?lock-acquire" | jq '.' { "CreateIndex": 12, "CreateTime": 1690818288003482000, "Items": {}, "Lock": { "TTL": "15s", "LockDelay": "10s", "ID": "0a575be4-abe1-333f-440d-9b267085df1d" }, "ModifyIndex": 16, "ModifyTime": 1690818329917095000, "Namespace": "default", "Path": "nomad/jobs/blah" }

tgross · 2023-07-31T14:56:38Z

command/agent/variable_endpoint_test.go

+			badBuf := encodeBrokenReq(&svLA)
+
+			req, err := http.NewRequest("PUT", "/v1/var/"+sv1.Path+"?cas=1&"+acquireLockQueryParam, badBuf)
+			require.NoError(t, err)


For new tests we're trying to use shoenig/test

Co-authored-by: Tim Gross <[email protected]>

* temp: remove when real work is added * func: add acquier/release to apply rpc function * Delete lock.go * func: add new rpc endpoint * func: add http wrapper for command agent * func: remove lock related item * fix: sync msg types * func: remove the lock key form the url * fix: fix the flow of the preapply function * remove raft message for renew lock * style: refactor and spell mistakes * func: separate validation for lock acquier * fix: update agent command for lock after rpc endpoint update from query to write * func: add test for validate lock * style: clean unsed code and update comments * func: remove raft message type for renew lock, not needed * fix: update logic to distinguish upsert variable and lock operation on the http endpoint * fix: adjust the look up for query params to only be done on put and posts to simplify the logic * fix: move the rpc metrics up on the renew lock to include auth errors * func: add acquire and release lock functions on fsm * func: fix the query for missing lock to avoid nil points in case of a missing lock * func: add happy path test for acquire and release flows on non existing variable * fix: solve rebase with rpc conflicts * func: add testing for release lock * fix: remove debug file * Update nomad/fsm.go Co-authored-by: Tim Gross <[email protected]> * func: change the return when lock ID is wrong from error to conflict Also hide the variable items * func: add lock ID verification for variable upserts * fix: check for empty lock instead of empty lock ID on setCas to avoid nil pointer * fix: separate the table update for the lock release, the var update cant be used because of the missing lock ID * func: move lock ID check to the inner most tx function, not the CAS wrappers * func: remove the lock ID check from the CAS delete * func: allow for users to overide the variable data when locking, the responsibility is left for teh user * fix: make acquire lock function somewhat idempotente * func: add timer operations to variable locks * style: remove redundant code * Add acquire and release lock functions on the state store (#17794) * fix: move the rpc metrics up on the renew lock to include auth errors * func: add acquire and release lock functions on fsm * func: fix the query for missing lock to avoid nil points in case of a missing lock * func: add happy path test for acquire and release flows on non existing variable * fix: solve rebase with rpc conflicts * func: add testing for release lock * style: typo * fix: remove debug file * Update nomad/fsm.go Co-authored-by: Tim Gross <[email protected]> * func: change the return when lock ID is wrong from error to conflict Also hide the variable items * func: add lock ID verification for variable upserts * fix: check for empty lock instead of empty lock ID on setCas to avoid nil pointer * fix: separate the table update for the lock release, the var update cant be used because of the missing lock ID * func: add tests for delete CAS with lock ID check * func: move lock ID check to the inner most tx function, not the CAS wrappers * fix: add lock not nil check on release to avoid nill pointers * style: function rename * func: remove the lock ID check from the CAS delete * func: allow for users to overide the variable data when locking, the responsibility is left for teh user --------- Co-authored-by: Tim Gross <[email protected]> * func: merge the timer functions with the leader functions and add lock check for var apply * fix: add a restriction for CAS and lock, only one at the time and avoid overwriting data when lock release * fix: add existing timer check for lock acquire * func: add tests for the creation and renewal of the timers * fix: add path to renew lock http handler * fix: avoif panics by reading path and namespace from req and not response * func: add functional tests to lock * fix: remove duplicated code after rebase * fix: remove the lock info from the correct return value on listing and add testing * fix: update test after making TimerNum public * func: only return values after the renew was successful, and add tests * style: typo * fix: add empty lock when no lock is provided for a lock acquire to avoid panics * fix: use lockID function instead of calling the lock ID directly to avoid panics * func: add tests for command agent lock actions * Update nomad/variables_endpoint.go Co-authored-by: Tim Gross <[email protected]> * Update nomad/variables_endpoint.go Co-authored-by: Tim Gross <[email protected]> * fix: typo --------- Co-authored-by: James Rasell <[email protected]> Co-authored-by: Tim Gross <[email protected]>

Juanadelacuesta changed the base branch from main to f-gh-17449 June 23, 2023 12:13

vercel bot deployed to Preview – nomad-storybook-and-ui June 23, 2023 12:17 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 23, 2023 12:51 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 26, 2023 16:22 View deployment

Juanadelacuesta changed the title ~~F gh 17449 rpc~~ Add RPC modifications to include the lock work Jun 26, 2023

Juanadelacuesta force-pushed the f-gh-17449-rpc branch 2 times, most recently from eb4ef7a to 41e79b6 Compare June 26, 2023 16:34

vercel bot deployed to Preview – nomad-storybook-and-ui June 26, 2023 16:38 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 27, 2023 08:43 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 27, 2023 08:59 View deployment

Juanadelacuesta commented Jun 27, 2023

View reviewed changes

helper/raftutil/msgtypes.go Outdated Show resolved Hide resolved

Juanadelacuesta marked this pull request as ready for review June 27, 2023 09:02

Juanadelacuesta requested review from lgfa29 and jrasell June 27, 2023 09:03

vercel bot deployed to Preview – nomad-storybook-and-ui June 28, 2023 08:00 View deployment

Juanadelacuesta force-pushed the f-gh-17449-rpc branch from a00bbc8 to ffae0f3 Compare June 28, 2023 16:34

vercel bot deployed to Preview – nomad-storybook-and-ui June 28, 2023 16:36 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 28, 2023 19:26 View deployment

Juanadelacuesta force-pushed the f-gh-17449-rpc branch from 302ff54 to 15ac582 Compare June 29, 2023 07:11

vercel bot deployed to Preview – nomad-storybook-and-ui June 29, 2023 07:13 View deployment

Juanadelacuesta force-pushed the f-gh-17449-rpc branch from 15ac582 to a044238 Compare June 29, 2023 07:40

vercel bot deployed to Preview – nomad-storybook-and-ui June 29, 2023 07:43 View deployment

jrasell requested changes Jun 29, 2023

View reviewed changes

vercel bot deployed to Preview – nomad-storybook-and-ui June 29, 2023 10:41 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 29, 2023 10:55 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 30, 2023 08:53 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 30, 2023 08:57 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 30, 2023 09:30 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 30, 2023 10:20 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui June 30, 2023 10:29 View deployment

Juanadelacuesta added 3 commits July 27, 2023 15:58

fix: avoif panics by reading path and namespace from req and not resp…

83d9d7d

…onse

func: add functional tests to lock

6440af0

fix: remove duplicated code after rebase

c769d5b

Juanadelacuesta force-pushed the f-gh-17449-rpc branch from aa9fc53 to c769d5b Compare July 27, 2023 14:04

vercel bot deployed to Preview – nomad-storybook-and-ui July 27, 2023 14:06 View deployment

fix: remove the lock info from the correct return value on listing an…

276b9f5

…d add testing

vercel bot deployed to Preview – nomad-storybook-and-ui July 28, 2023 08:50 View deployment

fix: update test after making TimerNum public

8124801

vercel bot deployed to Preview – nomad-storybook-and-ui July 28, 2023 14:03 View deployment

vercel bot deployed to Preview – nomad-storybook-and-ui July 28, 2023 14:10 View deployment

func: only return values after the renew was successful, and add tests

950749a

Juanadelacuesta force-pushed the f-gh-17449-rpc branch from 3ac1855 to 950749a Compare July 28, 2023 14:30

style: typo

ef7cffe

vercel bot deployed to Preview – nomad-storybook-and-ui July 28, 2023 14:36 View deployment

Juanadelacuesta added 3 commits July 31, 2023 11:14

fix: add empty lock when no lock is provided for a lock acquire to av…

35b309a

…oid panics

fix: use lockID function instead of calling the lock ID directly to a…

8496b6d

…void panics

func: add tests for command agent lock actions

6c0eab9

vercel bot deployed to Preview – nomad-storybook-and-ui July 31, 2023 09:19 View deployment

tgross approved these changes Jul 31, 2023

View reviewed changes

Juanadelacuesta and others added 2 commits July 31, 2023 17:11

Update nomad/variables_endpoint.go

dfd9507

Co-authored-by: Tim Gross <[email protected]>

Update nomad/variables_endpoint.go

dfc61ad

Co-authored-by: Tim Gross <[email protected]>

vercel bot deployed to Preview – nomad-storybook-and-ui July 31, 2023 15:18 View deployment

fix: typo

d46dd2a

vercel bot deployed to Preview – nomad-storybook-and-ui July 31, 2023 15:43 View deployment

Juanadelacuesta merged commit 15696f5 into f-gh-17449 Jul 31, 2023
19 checks passed

Juanadelacuesta deleted the f-gh-17449-rpc branch July 31, 2023 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RPC modifications to include the lock work #17697

Add RPC modifications to include the lock work #17697

Juanadelacuesta commented Jun 23, 2023 •

edited

Loading

jrasell left a comment

tgross left a comment

tgross Jul 31, 2023

tgross Jul 31, 2023

Juanadelacuesta Jul 31, 2023 •

edited

Loading

tgross Jul 31, 2023

Juanadelacuesta Jul 31, 2023 •

edited

Loading

tgross Jul 31, 2023

Add RPC modifications to include the lock work #17697

Add RPC modifications to include the lock work #17697

Conversation

Juanadelacuesta commented Jun 23, 2023 • edited Loading

jrasell left a comment

Choose a reason for hiding this comment

tgross left a comment

Choose a reason for hiding this comment

tgross Jul 31, 2023

Choose a reason for hiding this comment

tgross Jul 31, 2023

Choose a reason for hiding this comment

Juanadelacuesta Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

tgross Jul 31, 2023

Choose a reason for hiding this comment

Juanadelacuesta Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

tgross Jul 31, 2023

Choose a reason for hiding this comment

Juanadelacuesta commented Jun 23, 2023 •

edited

Loading

Juanadelacuesta Jul 31, 2023 •

edited

Loading

Juanadelacuesta Jul 31, 2023 •

edited

Loading