Generic resource cli by RenaudWasTaken · Pull Request #2347 · moby/swarmkit

RenaudWasTaken · 2017-08-09T23:52:06Z

Signed-off-by: Renaud Gaubert [email protected]

What I did:
Changed the CLI format of Generic Resources as suggested by @thaJeztah in docker/cli/pull/429
From --generic-resources "apple=2;orange={blue,red,green}"
To --generic-resources "apple=2,orange=blue,orange=red,orange=green"

This new format albeit a bit more verbose is the CSV format and is already used for --mount

See here for tracking
See here for design document

How to verify it:

$ swarmd ... --generic-resources "apple=2,orange=blue,orange=red,orange=green"
$ swarmct service create ... --generic-resources "orange=2,apple=2"

ping @aaronlehmann

codecov · 2017-08-10T00:03:06Z

Codecov Report

Merging #2347 into master will decrease coverage by <.01%.
The diff coverage is 77.08%.

@@            Coverage Diff             @@
##           master    #2347      +/-   ##
==========================================
- Coverage   60.52%   60.51%   -0.01%     
==========================================
  Files         128      128              
  Lines       26366    26401      +35     
==========================================
+ Hits        15958    15977      +19     
- Misses       9005     9017      +12     
- Partials     1403     1407       +4

RenaudWasTaken · 2017-09-17T23:05:32Z

Ping @aluzzardi @dongluochen @aaronlehmann :)

aaronlehmann · 2017-09-18T03:31:45Z

api/genericresource/parse.go

+// ParseCmd parses the Generic Resource command line argument
+// and returns a list of *api.GenericResource
+func ParseCmd(cmd string) ([]*api.GenericResource, error) {
+	if strings.Contains(cmd, "\n") {


Seems like a strange thing to check for - I don't know of any CLI arguments where this would be an issue, or where we explicitly check for it. But it shouldn't hurt.

aaronlehmann · 2017-09-18T03:32:03Z

api/genericresource/parse.go

+// and returns a list of *api.GenericResource
+func ParseCmd(cmd string) ([]*api.GenericResource, error) {
+	if strings.Contains(cmd, "\n") {
+		return nil, newParseError("unexpected '\n' character")


You'll want to use backticks instead of double quotes to avoid putting an actual newline inside the erorr stirng.

aaronlehmann · 2017-09-18T03:34:45Z

api/genericresource/parse.go

 		if len(kva) != 2 {
-			return nil, newParseError("incorrect term %s, missing '=' or malformed expr", term)
+			return nil, newParseError("incorrect term %s, missing"+
+				"'=' or malformed expression", term)


There's a missing space between missing and =.

aaronlehmann · 2017-09-18T03:36:19Z

api/genericresource/parse.go

 			if u < 0 {
-				return nil, newParseError("cannot ask for negative resource %s", key)
+				return nil, newParseError("cannot ask for"+
+					"negative resource %s", k)


Missing space between for and negative.

aaronlehmann · 2017-09-18T03:38:40Z

api/genericresource/parse.go

-func Parse(cmd string) ([]*api.GenericResource, error) {
-	var rs []*api.GenericResource
+func namedResourceVal(res string) (int64, error) {
+	return strconv.ParseInt(res, 10, 64)


If this parses as an integer, wouldn't it be a discrete resource, not a named resource as the function name suggests?

aaronlehmann · 2017-09-18T03:43:49Z

api/genericresource/parse.go

 		}

-		return nil, newParseError("could not parse expression '%s'", term)
+		return nil, newParseError("malformed expression '%s=%s'", k, v)


Seems like the only possible error condition is a mix of numeric and non-numeric values. The error message should make this clear.

aaronlehmann · 2017-09-18T03:48:46Z

api/genericresource/parse.go

+
+	var rs []*api.GenericResource
+	for k, v := range tokens {
+		if len(v) == 1 {


How about calling a function here that checks that len(v) == 1 and namedResourceVal does not return an error? It would slightly simplify the control flow.

RenaudWasTaken · 2017-09-20T00:05:59Z

@aaronlehmann fixed !

RenaudWasTaken · 2017-09-26T21:45:45Z

@aluzzardi PTAL :)

RenaudWasTaken · 2017-10-12T19:07:01Z

@aaronlehmann is it possible to merge this PR :) ?

aaronlehmann · 2017-10-12T19:55:22Z

@aaronlehmann is it possible to merge this PR :) ?

I don't have merge access anymore. @stevvooe could you please merge it?

RenaudWasTaken · 2017-10-18T20:50:58Z

ping @stevvooe :)

RenaudWasTaken · 2017-10-24T23:03:07Z

ping @aluzzardi @aaronlehmann @stevvooe

anshulpundir

@RenaudWasTaken some nits.

anshulpundir · 2017-10-24T23:07:14Z

api/genericresource/parse.go

+	return strconv.ParseInt(res, 10, 64)
+}
+
+func allNamedResources(res []string) bool {


anshulpundir · 2017-10-24T23:07:31Z

api/genericresource/parse.go

-// Parse parses the GenericResource resources given by the arguments
-func Parse(cmd string) ([]*api.GenericResource, error) {
-	var rs []*api.GenericResource
+func discreteResourceVal(res string) (int64, error) {


nit: I'd suggest adding a comment. thx!

anshulpundir · 2017-10-24T23:08:54Z

api/genericresource/parse.go

 	return rs, nil
 }
+
+func isDiscreteResource(values []string) (int64, bool) {


same comment.

RenaudWasTaken · 2017-10-24T23:20:14Z

@anshulpundir done

Signed-off-by: Renaud Gaubert <[email protected]>

RenaudWasTaken · 2017-10-25T00:10:32Z

@anshulpundir CI is green can we finally merge this PR :) ?

Upgrade swarmkit dependency. Changes: moby/swarmkit@ce5f7b8a (HEAD -> master, origin/master, origin/HEAD) Merge pull request moby/swarmkit#2411 from crunchywelch/2401-arm64_support moby/swarmkit@b0856099 Merge pull request moby/swarmkit#2423 from thaJeztah/new-misty-handle moby/swarmkit@2bd294fc Update Misty's GitHub handle moby/swarmkit@0769c605 Comments for orphaned state/task reaper. (moby/swarmkit#2421) moby/swarmkit@de950a7e Generic resource cli (moby/swarmkit#2347) moby/swarmkit@312be598 Provide custom gRPC dialer to override default proxy dialer (moby/swarmkit#2419) moby/swarmkit@4f12bf79 Merge pull request moby/swarmkit#2415 from cheyang/master moby/swarmkit@8f9f7dc1 add pid limits moby/swarmkit@da5ee2a6 Merge pull request moby/swarmkit#2409 from dperny/workaround-attachments moby/swarmkit@0c7b2fc2 Delete node attachments when node is removed moby/swarmkit@9d702763 normalize "aarch64" architectures to "arm64" moby/swarmkit@28f91d8...ce5f7b8 Signed-off-by: Marcus Martins <[email protected]>

* Generic resource now uses csv format Signed-off-by: Renaud Gaubert <[email protected]> * Tested new input format Signed-off-by: Renaud Gaubert <[email protected]> * Updated Generic resource design to use new CLI format Signed-off-by: Renaud Gaubert <[email protected]> * Updated swarmctl and swarmd to use the new Generic resource CLI format Signed-off-by: Renaud Gaubert <[email protected]>

RenaudWasTaken mentioned this pull request Aug 10, 2017

Add Support for Swarmkit Generic Resources moby/moby#33439

Open

7 tasks

nishanttotla requested a review from aluzzardi August 11, 2017 17:50

aaronlehmann reviewed Sep 18, 2017

View reviewed changes

RenaudWasTaken force-pushed the generic-resource-cli branch from f9a8e2c to 73b6493 Compare September 19, 2017 23:36

aaronlehmann approved these changes Sep 20, 2017

View reviewed changes

anshulpundir reviewed Oct 24, 2017

View reviewed changes

RenaudWasTaken force-pushed the generic-resource-cli branch from 73b6493 to 370d333 Compare October 24, 2017 23:19

Renaud Gaubert added 4 commits October 25, 2017 01:52

Updated Generic resource design to use new CLI format

afdc823

Signed-off-by: Renaud Gaubert <[email protected]>

Generic resource now uses csv format

31cc4a7

Signed-off-by: Renaud Gaubert <[email protected]>

Tested new input format

08c2854

Signed-off-by: Renaud Gaubert <[email protected]>

Updated swarmctl and swarmd to use the new Generic resource CLI format

c7b06ee

Signed-off-by: Renaud Gaubert <[email protected]>

RenaudWasTaken force-pushed the generic-resource-cli branch from 370d333 to c7b06ee Compare October 24, 2017 23:55

anshulpundir merged commit de950a7 into moby:master Oct 25, 2017

thaJeztah mentioned this pull request Oct 30, 2017

Revendored Swarmkit moby/moby#35326

Merged

marcusmartins mentioned this pull request Nov 3, 2017

Bump docker/swarmkit to ce5f7b8a6f5a1b1e4a29cafdf887304e400e08a9 moby/moby#35405

Closed

thaJeztah mentioned this pull request Nov 10, 2017

support of GPU schedule moby/moby#35457

Closed

Conversation

RenaudWasTaken commented Aug 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RenaudWasTaken commented Sep 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RenaudWasTaken commented Sep 20, 2017

Uh oh!

RenaudWasTaken commented Sep 26, 2017

Uh oh!

RenaudWasTaken commented Oct 12, 2017

Uh oh!

aaronlehmann commented Oct 12, 2017 via email

Uh oh!

RenaudWasTaken commented Oct 18, 2017

Uh oh!

RenaudWasTaken commented Oct 24, 2017

Uh oh!

anshulpundir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RenaudWasTaken commented Oct 24, 2017

Uh oh!

RenaudWasTaken commented Oct 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RenaudWasTaken commented Aug 9, 2017 •

edited

Loading

codecov bot commented Aug 10, 2017 •

edited

Loading