Use : as a config --set slice separator #484

elezar · 2024-05-03T15:34:22Z

This change switches to using : as the separator for slices when calling nvidia-ctk config --set. This works around an issue with the handling of string-slice arguments in the urfave/cli module.

For example, running:

nvidia-ctk config --set nvidia-container-runtime.runtimes=some:list:of:runtimes

yields:

runtimes = ["some", "list", "of", "runtimes"]

Fixes #466

klueska · 2024-05-03T15:40:03Z

I glanced through the linked issue but its still not clear to me why , doesn't work as the separator.

elezar · 2024-05-03T15:44:20Z

@klueska they behaviour for StringSliceFlags was changed in urfave v2.5.1. This was to fix a bug for properly merging repeated flags. We were however depending on the former behaviour where we allow a flag to be repeated, but don't split their values. This meant that this functionality broke when we updated urfave as part of our release process.

There may be a more invasive change that I could make here, but I thought allowing the separator to be customized was a low-risk change.

klueska · 2024-05-06T11:30:26Z

Do you mean v2.25.1? Is this the change that broke things for us: urfave/cli#1710?

klueska · 2024-05-06T11:37:05Z

cmd/nvidia-ctk/config/config.go

@@ -72,9 +76,16 @@ func (m command) build() *cli.Command {
 		},
 		&cli.StringSliceFlag{
 			Name:        "set",
-			Usage:       "Set a config value using the pattern key=value. If value is empty, this is equivalent to specifying the same key in unset. This flag can be specified multiple times",
+			Usage:       "Set a config value using the pattern key=value. If value is empty, this is equivalent to specifying the same key in unset. This flag can be specified multiple times. If the setting represents a slice, the elements are semi-colon-separated.",


Suggested change

Usage: "Set a config value using the pattern key=value. If value is empty, this is equivalent to specifying the same key in unset. This flag can be specified multiple times. If the setting represents a slice, the elements are semi-colon-separated.",

Usage: "Set a config value using the pattern key=value. If value is empty, this is equivalent to specifying the same key in unset. This flag can be specified multiple times. If the setting represents a list, the elements are semi-colon-separated.",

Was semicolon the default previously? I'm still trying to wrap my head around the previous behaviour vs. the new behaviour.

What I think you are saying in another comment is that we previously we always interpreted every input as a single string, and if it was supposed to be slice, we did the separation ourselves. What's not clear (assuming I got this right) is whether we previously did the separation with semicolons or commas, and (if it was commas), why we con't continue to use commas now instead of semilcolons.

Yes, we were previously treating a comma-separated argument as a single string (due to a bug in urfave) and we were splitting this by commas ourselves to construct a slice to insert into the config file. We can't continue to use commas now because this urfave already assumes commas as the separator for repeated args. We could consider overriding this instead, but we can only do this at an Application level and not at a Command level. It may be possible with the alpha v3.0.0 release, but I didn't want to rely on an unstable dependency.

Consider the following code:

package main import ( "fmt" "os" "github.com/urfave/cli/v2" ) func main() { var slice cli.StringSlice c := cli.NewApp() c.Flags = []cli.Flag{ &cli.StringSliceFlag{ Name: "slice-flag", Destination: &slice, }, } c.Action = func(ctx *cli.Context) error { fmt.Printf("slice=%q\n", slice.Value()) return nil } c.Run(os.Args) }

When using v2.5.0 we get:

$ go run ./main.go --slice-flag=foo,bar,baz slice=["foo,bar,baz"] $ go run ./main.go --slice-flag=foo,bar,baz --slice-flag=boom slice=["foo,bar,baz" "boom"]

When updating to v2.5.1 we get:

$ go run ./main.go --slice-flag=foo,bar,baz slice=["foo" "bar" "baz"] $ go run ./main.go --slice-flag=foo,bar,baz --slice-flag=boom slice=["foo" "bar" "baz" "boom"]

clearly indicating that a bug was fixed for handling comma-separated repeated arguments when updating to at least v2.5.1.

We were however depending on the broken behaviour, meaning that when a user ran:

nvidia-ctk config --set nvidia-container-runtime.runtimes=crun,runc

we had a slice with the following contents:

[]string{"nvidia-container-runtime.runtimes=crun,runc"}

When we could process -- first splitting by = and then the second argument by , to generate the eventually slice []string{"crun", "runc"}.

With the update to v2.27.1 when a user runs:

nvidia-ctk config --set nvidia-container-runtime.runtimes=crun,runc

we have a slice with the following contents:

[]string{"nvidia-container-runtime.runtimes=crun" ,"runc"}

Which we cannot effectively parse.

With the switch to ; as our own separator, we no have the inputs:

nvidia-ctk config --set nvidia-container-runtime.runtimes=crun;runc

which would be parsed to:

[]string{"nvidia-container-runtime.runtimes=crun;runc"}

meaning that we can do the same processing as before.

OK, so this is defining the separator we want to use to break apart elements of an embedded list of strings that we don't want to become elements of the overall StringSlice.

Meaning that , is still the default for the top-level separator, and we now defining ; as the embedded separator.

I think this is fine then -- my only worry would be if / where else we rely on this "bug". I seem to remember relying on this functionality in the device-plugin as well, for example.

From my knowledge of our code bases, I would say that this is the only place that we're relying on the broken behaviour since this is the only place we have logic in place to further separate strings that we were expecting to be parsed as different arguments.

I would also futher suggest using : as the separator to match the path-list-separator in linux.

Seems reasonable to me

klueska · 2024-05-06T11:37:26Z

cmd/nvidia-ctk/config/config.go

+			Name:        "set-slice-separator",
+			Usage:       "Specify a separator for slices applied using the set command.",


Suggested change

Name: "set-slice-separator",

Usage: "Specify a separator for slices applied using the set command.",

Name: "set-list-separator",

Usage: "Specify a separator for lists applied using the set command.",

elezar · 2024-05-06T11:47:24Z

Do you mean v2.25.1? Is this the change that broke things for us: urfave/cli#1710?

No, I mean v2.5.1. We were using v2.3.0 when we originally released this functionality and updated directly to v2.27.1 in #309. The following PR urfave/cli#1377 was released as part fo v2.5.1 and after testing urfave updates after v2.3.0 locally found this to be the culprit.

ArangoGutierrez

PR now looks ok to me, and addresses @klueska comments.
Only comment please update the PR description which still uses ;

klueska · 2024-05-08T14:24:32Z

cmd/nvidia-ctk/config/config.go

+			Usage:       "Specify a separator for lists applied using the set command.",
+			Hidden:      true,
+			Value:       ":",
+			Destination: &opts.setSliceSeparator,


should we change the variable to match the flag name?

Sure. Will update.

klueska · 2024-05-08T14:24:45Z

cmd/nvidia-ctk/config/config.go

@@ -96,6 +111,13 @@ func (m command) build() *cli.Command {
 	return &c
 }

+func validateFlags(c *cli.Context, opts *options) error {
+	if opts.setSliceSeparator == "" {
+		return fmt.Errorf("set-slice-separator must be set")


Suggested change

return fmt.Errorf("set-slice-separator must be set")

return fmt.Errorf("set-list-separator must be set")

klueska

Minor comments. LGTM otherwise

This allows settings such as: nvidia-ctk config --set nvidia-container-runtime.runtimes=crun:runc to be applied correctly. Signed-off-by: Evan Lezar <elezar@nvidia.com>

cmd/nvidia-ctk/config/config.go

elezar requested review from ArangoGutierrez, cdesiniotis and tariq1890 May 3, 2024 15:34

elezar self-assigned this May 3, 2024

elezar force-pushed the fix-config-set branch from 1522ab1 to aa8f661 Compare May 3, 2024 15:45

elezar mentioned this pull request May 3, 2024

nvidia-ctk config set list still not work on 1.15.0 #466

Closed

elezar force-pushed the fix-config-set branch from aa8f661 to d715371 Compare May 3, 2024 15:53

elezar requested a review from klueska May 6, 2024 09:34

klueska reviewed May 6, 2024

View reviewed changes

elezar force-pushed the fix-config-set branch from d715371 to 9b99924 Compare May 7, 2024 14:28

elezar changed the title ~~Use ; as a config --set slice separator~~ Use : as a config --set slice separator May 7, 2024

ArangoGutierrez reviewed May 8, 2024

View reviewed changes

klueska reviewed May 8, 2024

View reviewed changes

Use : as a config --set list separator

5a3eda4

This allows settings such as: nvidia-ctk config --set nvidia-container-runtime.runtimes=crun:runc to be applied correctly. Signed-off-by: Evan Lezar <elezar@nvidia.com>

elezar force-pushed the fix-config-set branch from 9b99924 to 5a3eda4 Compare May 8, 2024 15:27

elezar requested review from klueska and ArangoGutierrez May 8, 2024 15:27

ArangoGutierrez approved these changes May 8, 2024

View reviewed changes

tariq1890 reviewed May 8, 2024

View reviewed changes

cmd/nvidia-ctk/config/config.go Show resolved Hide resolved

tariq1890 approved these changes May 10, 2024

View reviewed changes

elezar merged commit f13f1bd into NVIDIA:main May 13, 2024
8 checks passed

elezar mentioned this pull request May 13, 2024

Prepare changes for v1.15.1 release #495

Closed

elezar deleted the fix-config-set branch July 15, 2024 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use : as a config --set slice separator #484

Use : as a config --set slice separator #484

elezar commented May 3, 2024 •

edited

Loading

klueska commented May 3, 2024

elezar commented May 3, 2024

klueska commented May 6, 2024

klueska May 6, 2024

klueska May 6, 2024

elezar May 6, 2024

klueska May 6, 2024

klueska May 6, 2024

klueska May 6, 2024

elezar May 6, 2024

klueska May 6, 2024

klueska May 6, 2024

elezar commented May 6, 2024

ArangoGutierrez left a comment

klueska May 8, 2024

elezar May 8, 2024

elezar May 8, 2024

klueska May 8, 2024

elezar May 8, 2024

klueska left a comment

	Usage: "Set a config value using the pattern key=value. If value is empty, this is equivalent to specifying the same key in unset. This flag can be specified multiple times. If the setting represents a slice, the elements are semi-colon-separated.",
	Usage: "Set a config value using the pattern key=value. If value is empty, this is equivalent to specifying the same key in unset. This flag can be specified multiple times. If the setting represents a list, the elements are semi-colon-separated.",

		Name: "set-slice-separator",
		Usage: "Specify a separator for slices applied using the set command.",

	return fmt.Errorf("set-slice-separator must be set")
	return fmt.Errorf("set-list-separator must be set")

Use : as a config --set slice separator #484

Use : as a config --set slice separator #484

Conversation

elezar commented May 3, 2024 • edited Loading

klueska commented May 3, 2024

elezar commented May 3, 2024

klueska commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elezar commented May 6, 2024

ArangoGutierrez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klueska left a comment

Choose a reason for hiding this comment

elezar commented May 3, 2024 •

edited

Loading