Allow lookups based on short identifiers #575

iverberk · 2015-12-12T17:36:36Z

This change introduces the ability to specify identifiers for jobs, allocs, evals and nodes on the command line with as little as one or two characters, provided that it uniquely identifies the resource. An error with the possible results will be provided when the short identifier has multiple results.

This is the first part of an implementation for #54. Truncating the UUID will be implemented separatly. This pull-request requires changes in go-memdb and go-immutable-radix that are specified in pull-request hashicorp/go-immutable-radix#3 and hashicorp/go-memdb#8.

This change introduces the ability to specify identifiers for jobs, allocs, evals and nodes on the command line with as little as one character, provided that it uniquely identifies the resource. An error with the possible results will be provided when the short identifier has multiple results.

dadgar · 2015-12-15T21:19:13Z

Hey @iverberk, I closed your other PRs for immutable-radix and go-memdb. They both have the constructs we need to accomplish this already. Going to leave some comments to guide how we can do this.

dadgar · 2015-12-15T21:24:40Z

nomad/state/state_store.go

 	if err != nil {
 		return nil, fmt.Errorf("node lookup failed: %v", err)
 	}

 	if existing != nil {
-		return existing.(*structs.Node), nil
+		// Return exact match directly


So it is best if we revert all these to their original state and then we can add additional methods such as NodeByIDPrefix. These methods can return a memdb.ResultIterator and use memdb.Txn().Get() and use the "_prefix" operator.

We can then add HTTP endpoints that would take a prefix and would call these new methods. And then they could return back a fixed set to the client. This would then let the user refine the search if multiple are returned or if just one is returned, the cli could just call with the full ID.

dadgar · 2015-12-15T21:25:05Z

Let me know if you have any questions

iverberk · 2015-12-16T22:03:25Z

Hi Alex, thanks for the feedback. Great to hear that the prefix searching is already supported by the db libraries, I overlooked it somehow. So before I update the pull-request, a couple of questions:

From quickly looking at the memdb code I gather that the indexer must implement the PrefixFromArgs method in order to be searchable by prefix. Currently this is only implemented by the StringFieldIndex and CompoundIndex. This means that jobs and nodes will be prefix-searchable, but the allocs and evals won't be because they use the UUIDFieldIndex, which does not implement the PrefixIndexer interface. Is this correct and if so, how would you suggest we go about this? I was also wondering if there is a specific reason why the indexer type for the node identifier is not also an UUIDFieldIndex?
It's not entirely clear to me what you want the end-user interaction to look like. The original goal was to have short identifiers for the cli 'node-status', 'eval-monitor' and 'alloc-status' commands. So the user would enter the first few characters of the identifier and it would execute the command if it uniquely identified the resource. Otherwise an error would be returned with possible options. Is this indeed the desired interaction? If so, in which method would the additional HTTP endpoints be called?

I'll start working on the implementation tomorrow. If you could answer these questions for me that would be great, thanks!

dadgar · 2015-12-17T19:01:43Z

Yes we will have to make UUIDFieldIndexer meet that interface. I will add that for you. The reason is node id is user specifiable.
So with alloc-status and eval-monitor you can just detect if the string is less than 36 characters (since it is a UUID) and then call the new endpoint to get a list of the possible matches. If that returns a single match you can call the GetEval or GetAlloc method with the exact UUID, otherwise display the options to the user.

For node-status, it is a little tricker. You could assume exact and if there is an error try the prefix endpoint, or if you wanted to be fancy you could do the requests in parallel.

dadgar · 2015-12-18T01:04:33Z

@iverberk: I made the UUIDFieldIndex support PrefixFromArgs. So you are good to go on that front.

* Use go-memdb prefix indexer for lookups * Add Job lookups * Update state store with new ByIDPrefix get methods * Call new methods when exact lookup fails or is not applicable

iverberk · 2015-12-20T17:08:05Z

The branch has been updated according to the proposed design. It also incorporates a change to display short node identifiers when possible. Please let me know what you think and if anything needs changing.

iverberk · 2015-12-20T21:24:03Z

The tests seem to fail but I'm unable to even make the tests pass on master, either on OSX or in the included Vagrant virtual machine. So I think the errors are actually not related to the changes in this pull-request but I might be wrong. Please let me know if tests break due to changes in this PR.

dadgar · 2015-12-21T23:33:24Z

command/node_status.go

@@ -160,3 +164,27 @@ func (c *NodeStatusCommand) Run(args []string) int {
 	}
 	return 0
 }
+


I would remove both these methods and replace them with one that takes the original nodes and returns a map of their original ids to their shortened ones.

shortIds(nodes []*api.NodeListStub) map[string]string

With this method I think you could be a bit more advanced with how you shorten. You could start at 8 characters and if there is a collision, then up it to 9, etc, until you get no collisions. Further it is probably still worth while to shorten the non-custom ones if there exists a custom one.

This reverts commit 5d3bd1b.

* Reverted changes to get methods * Added prefix query parameter * Updated node status to use prefix based searching * Fixed tests * Removed truncation logic

iverberk · 2015-12-22T22:55:27Z

Hi Alex, thanks for your patience in guiding me through this. I've updated the pull-request based on your comments. I've only updated the node status-command fully because I want to verify the changes before I implement the other commands similarly. I've removed the truncation from this pull-request, it will be done in another pull-request to not mix things up to much. Tests seem to be happy as well now. Please let me know if the current changes are in line with your thoughts!

iverberk · 2015-12-22T22:59:54Z

command/node_drain.go

+	// Check if node exists
+	node, _, err := client.Nodes().Info(nodeID, nil)
+	if err != nil {
+		c.Ui.Error(fmt.Sprintf("Error toggling drain mode: %s", err))


This is strictly not an error for toggling the drain mode, but rather for looking up the node. However, the testcase checks for this string so I'd like to hear your thoughts on this.

I think this is fine as the err will be something like "node with id "foobar" not found"

dadgar · 2015-12-23T01:10:18Z

@iverberk: Great work! This is exactly what I was envisioning! Hope you also agree that is cleaner!

dadgar · 2015-12-23T01:17:54Z

command/node_status.go

+		}
+		// Return error if no nodes are found
+		if len(nodes) == 0 {
+			c.Ui.Error(fmt.Sprintf("Node not found"))


I would make this more verbose. fmt.Sprintf("No node(s) with prefix %q found", nodeID)

dadgar · 2015-12-23T01:19:38Z

Also down the line please update the docs. For example here

* Refactor other cli commands to new design * Add PrefixList method to api package * Add more tests

iverberk · 2015-12-24T20:37:27Z

Another big update. This mostly just adds similar functionality to the allocations, evaluations and jobs. I had to add and modify tests to cover all the new branches. I will add another commit to update the docs. Let me know what you think and merry christmas! :-)

iverberk · 2015-12-24T22:17:12Z

@dadgar: no idea why that particular test is failing. I can't reproduce it locally (all tests are passing, including the erroneous one). Can you help me investigate?

iverberk · 2015-12-29T11:27:31Z

I've added updates to the documentation. Feel free to adjust or comment on wording. The documentation updates seemed to have fixed the tests as well....

dadgar · 2016-01-04T18:24:52Z

@iverberk: Awesome work! I will try to look at it soon! Just got back from vacation so lots to get through. The reason the tests weren't passing for a bit was from an upstream bug that is fixed now!

dadgar · 2016-01-06T19:32:45Z

api/nodes_test.go

+	})
+
+	// Find node based on four character prefix
+	testutil.WaitForResult(func() (bool, error) {


Why does this have to be wrapped in a WaitForResult?

That's an oversight on my part, fixed.

dadgar · 2016-01-06T19:51:35Z

So excited for this @iverberk! Thank you so much! I left some simple comments but it looks ready to merge 👍

* Fix tests * Update documentation

iverberk · 2016-01-06T21:51:12Z

It think I addressed your comments with the new commit. Please take a look and let me know! :-)

dadgar · 2016-01-06T22:36:16Z

Awesome! Thank you so much @iverberk, this is really great!

Allow lookups based on short identifiers

iverberk · 2016-01-07T06:56:56Z

@dadgar really happy with this merged PR! Thanks so much for reviewing and guidance.

github-actions · 2023-04-30T02:11:11Z

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.

cbednarski added type/enhancement theme/core labels Dec 14, 2015

dadgar reviewed Dec 15, 2015
View reviewed changes

iverberk and others added 5 commits December 19, 2015 21:55

Bring up-to-date with master

cec0170

Short identifiers functionality

91075e1

* Use go-memdb prefix indexer for lookups * Add Job lookups * Update state store with new ByIDPrefix get methods * Call new methods when exact lookup fails or is not applicable

Allow short job identifiers for stop command

bdf4347

On cli node status list print the short Node ID when possible

5d3bd1b

Allow short identifiers for node-drain command

23bfbbf

Some comment corrections and additions

7d2f1c6

Bring up-to-date with master

dcecac1

dadgar reviewed Dec 21, 2015
View reviewed changes

iverberk added 3 commits December 22, 2015 10:15

Bring up-to-date with master

2a809b9

Revert "On cli node status list print the short Node ID when possible"

1af7c57

This reverts commit 5d3bd1b.

Refactoring

e89b5af

* Reverted changes to get methods * Added prefix query parameter * Updated node status to use prefix based searching * Fixed tests * Removed truncation logic

iverberk reviewed Dec 22, 2015
View reviewed changes

dadgar reviewed Dec 23, 2015
View reviewed changes

iverberk added 3 commits December 24, 2015 20:53

Refactoring continued

9057422

* Refactor other cli commands to new design * Add PrefixList method to api package * Add more tests

Bring up-to-date with master

db82564

Fix test (due to merge)

8218442

iverberk added 2 commits December 29, 2015 12:17

Documentation updates for short identifiers in the CLI

a73aaf1

Bring up-to-date with master

c65fdc3

dadgar reviewed Jan 6, 2016
View reviewed changes

Improvements for short identifiers

6295108

* Fix tests * Update documentation

dadgar added a commit that referenced this pull request Jan 6, 2016

Merge pull request #575 from nautsio/f-short-ids

cf3152d

Allow lookups based on short identifiers

dadgar merged commit cf3152d into hashicorp:master Jan 6, 2016

iverberk mentioned this pull request Jan 11, 2016

Truncate UUID to the first 8 characters #54

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow lookups based on short identifiers #575

Allow lookups based on short identifiers #575

iverberk commented Dec 12, 2015

dadgar commented Dec 15, 2015

dadgar Dec 15, 2015

dadgar commented Dec 15, 2015

iverberk commented Dec 16, 2015

dadgar commented Dec 17, 2015

dadgar commented Dec 18, 2015

iverberk commented Dec 20, 2015

iverberk commented Dec 20, 2015

dadgar Dec 21, 2015

iverberk commented Dec 22, 2015

iverberk Dec 22, 2015

dadgar Dec 23, 2015

dadgar commented Dec 23, 2015

dadgar Dec 23, 2015

dadgar commented Dec 23, 2015

iverberk commented Dec 24, 2015

iverberk commented Dec 24, 2015

iverberk commented Dec 29, 2015

dadgar commented Jan 4, 2016

dadgar Jan 6, 2016

iverberk Jan 6, 2016

dadgar commented Jan 6, 2016

iverberk commented Jan 6, 2016

dadgar commented Jan 6, 2016

iverberk commented Jan 7, 2016

github-actions bot commented Apr 30, 2023

Allow lookups based on short identifiers #575

Allow lookups based on short identifiers #575

Conversation

iverberk commented Dec 12, 2015

dadgar commented Dec 15, 2015

dadgar Dec 15, 2015

Choose a reason for hiding this comment

dadgar commented Dec 15, 2015

iverberk commented Dec 16, 2015

dadgar commented Dec 17, 2015

dadgar commented Dec 18, 2015

iverberk commented Dec 20, 2015

iverberk commented Dec 20, 2015

dadgar Dec 21, 2015

Choose a reason for hiding this comment

iverberk commented Dec 22, 2015

iverberk Dec 22, 2015

Choose a reason for hiding this comment

dadgar Dec 23, 2015

Choose a reason for hiding this comment

dadgar commented Dec 23, 2015

dadgar Dec 23, 2015

Choose a reason for hiding this comment

dadgar commented Dec 23, 2015

iverberk commented Dec 24, 2015

iverberk commented Dec 24, 2015

iverberk commented Dec 29, 2015

dadgar commented Jan 4, 2016

dadgar Jan 6, 2016

Choose a reason for hiding this comment

iverberk Jan 6, 2016

Choose a reason for hiding this comment

dadgar commented Jan 6, 2016

iverberk commented Jan 6, 2016

dadgar commented Jan 6, 2016

iverberk commented Jan 7, 2016

github-actions bot commented Apr 30, 2023