[LU-7796] "lctl set_param jobid_var" should return error about missing value

• Status: Resolved
• Project: Lustre
• Fix Version/s: Lustre 2.9.0
• Type: Bug
• Priority: Minor
• Reporter: Ryan Haasken
• Assignee: Emoly Liu
• Resolution: Fixed
• Severity: 3

Description

lctl set_param jobid_var should return an error and print an error message about a missing value, but instead it is just ignored.

[root@centclient05 LU-7437-test]# lctl get_param jobid_var
jobid_var=disable
[root@centclient05 LU-7437-test]# lctl set_param jobid_var
[root@centclient05 LU-7437-test]# echo $?
0
[root@centclient05 LU-7437-test]# lctl get_param jobid_var
jobid_var=disable

Note, however, that if the jobid_var has a trailing =, it behaves differently, despite meaning the same thing.

[root@centclient05 LU-7437-test]# lctl set_param jobid_var=
error: set_param: setting jobid_var: no value
[root@centclient05 LU-7437-test]# echo $?
22

Any trailing argument of the "set_param" without a corresponding value is ignored. E.g. the following variations also fail to print an error message when they should because jobid_var does not have a value:

lctl set_param ldlm.namespaces.*.lru_size=clear jobid_var
lctl set_param ldlm.namespaces.*.lru_size clear jobid_var
lctl set_param ldlm.namespaces.*osc*.lru_size=clear ldlm.namespaces.*mdc*.lru_size=clear jobid_var

However, if jobid_var contains a trailing '=', all of these examples print an error message and return a non-zero status.

In addition, this behavior is inconsistent and should be fixed:

[root@centclient05 LU-7437-test]# lctl set_param jobid_var=foo=bar
jobid_var=foo=bar
[root@centclient05 LU-7437-test]# lctl get_param jobid_var
jobid_var=foo=bar
[root@centclient05 LU-7437-test]# lctl set_param jobid_var foo=bar
error: set_param: setting jobid_var=foo=bar: bad value
[root@centclient05 LU-7437-test]# echo $?
22
[root@centclient05 LU-7437-test]# lctl get_param jobid_var
jobid_var=foo=bar

The two different lctl set_param commands above should mean the same thing. IMO, lctl set_param should accept values which contain the = character because there is no intrinsic constraint against writing a = to the corresponding procfs/sysfs/debugfs file.

Issue Links

• is related to: LU-7437 "lctl list_param -R" can't list the p...
• is related to: LU-7802 set_param lru_size fails with 'error:...

Comments

[Ryan Haasken]

This was introduced by this patch: http://review.whamcloud.com/#/c/18338/12

[Ryan Haasken]

I am willing to fix this, and I will provide a patch shortly.

Please let me know if you disagree with what I believe to be the correct behavior for lctl set_param. Namely, that it should print an error message and return a nonzero status for trailing valueless parameters of the space-separated form, and that it should allow for = characters in the values in the space-separated format.

[Emoly Liu]

Ryan, when we use "lctl set_param" to set one parameter in one command, we can use the following two formats:

• lctl set_param param_path1=value1
• or old format, lctl set_param param_path1 value1

And when we set multiple parameters, we recommend to use the following format

• lctl set_param <param_path1=value1 param_path2=value2 ...>. However, space-separated format is still supported in this case, but this format is not easy to read.

IMO, "=" in the values will get the format mixed up. adilger, do you have advice on this format?

BTW, "lctl set_param param_path" should return error since there is no valid value. Thanks for pointing this out.

[Andreas Dilger]

My preference is that we don't allow set_param with multiple space-separated arguments, and only as a backward-compatible way of handling single "name value" arguments for older scripts that do not use "name=value". As Emoly writes, the chance of having errors with "name1 value1 name2 value2 name3 value3" becomes significant, doubly so if the values allow an embedded '=' in them (which I had never intended, but I guess was never explicitly forbidden).

I mentioned in another patch that I think "name value" arguments should be deprecated in 2.9 (i.e print a warning to stderr that they should use "name=value") and this format should be dropped completely at some point in the future (say 3.2 or whatever).

[Ryan Haasken]

Thank you both for your input. I agree that the format with parameters and values separated by an '=' is better than the format with them separated by a space. I still plan to implement the deprecation warning for the old format in the patch for LU-5134.

My preference is that we don't allow set_param with multiple space-separated arguments, and only as a backward-compatible way of handling single "name value" arguments for older scripts that do not use "name=value".

I don't think it would be acceptable to change the implementation to reject the old format if more than one parameter-value pair is specified. This would be a change in the actual behavior that could break people's scripts, regardless of the recommended format for multiple param-value pairs.

I also see no reason why an '=' should be explicitly forbidden from appearing in the value. It is possible (although probably not currently required by any Lustre/LNet params) that a value contains an '='. I think the current behavior of an lctl set_param param_path1=value1 when value1 contains an '=' character is correct, and the older format should behave the same way. This is a minor issue. The bigger issue is probably that lctl set_param param_path1 does not print an error message about a missing value and exit with a nonzero status.

[Emoly Liu]

Ryan, I agree to fix that "bigger" issue first. Would you like to provide a patch for that? Or I can do that.

[Ryan Haasken]

I can provide such a patch. Would it make sense to also add the deprecation warning in this patch instead of in the patch for LU-5134? This would only be desirable if this fix will NOT land for 2.8. See line 1725 here for context:

http://review.whamcloud.com/#/c/10555/5/lustre/utils/lustre_cfg.c

Then see PS6 of that change for a possible implementation of the deprecation warning. I am undecided on whether it still makes sense to separate the parsing of the param/value pair into a new function in the fix for this bug. I think it's good to keep the functions shorter, but one could argue it creates an unnecessarily complicated interface in the code. I'll upload a possible patch and look for your feedback this week.

[Gerrit Updater]

Emoly Liu ([e-mail]) uploaded a new patch: http://review.whamcloud.com/18684
Subject: LU-7796 lctl: set_param should report errors for wrong formats
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: dce7003107212cfb07eaa9e968d27a2dfaad9557

[Emoly Liu]

Ryan, I know you are working on LU-5134 http://review.whamcloud.com/#/c/10555/. In case that your big patch won't be landed soon to fix this issue, I created this one. Please review it.

LU-7796 lctl: set_param should report errors for wrong formats

"lctl set_param" should report errors for the following wrong
parameter formats:

• path
• path=
• mix of the above formats, e.g.
  "lctl set_param jobid_var= timeout=20 abde=efg debug"

[Ryan Haasken]

Thanks, Emoly. I had actually planned to submit the fix for this issue in a separate patch from my patch for LU-5134. I reviewed your patch. There are a couple of things I don't agree with about the patch you've uploaded, particularly the handling of equals signs in the values when using the older set_param param value format. I will upload my patch too, and if you could take a look, I would appreciate it. I would prefer if we could land my patch since I think it will be a little cleaner to add the deprecation of the old set_param param value format to my patch. I also had already written up some sanity.sh tests for the problems listed in this bug, which I will include in my patch.

[Gerrit Updater]

Ryan Haasken ([e-mail]) uploaded a new patch: http://review.whamcloud.com/18703
Subject: LU-7796 utils: print error with valueless lctl set_param
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 2f86b817ad4d73c4f2c6112d743d770b8250898a

[Ryan Haasken]

I pushed my version of a fix for this bug here:

http://review.whamcloud.com/#/c/18703

Please take a look. I think it simplifies the code a little. I tried to compromise on allowing '=' in the values in the 'set_param param value' format by printing a warning message in that case.

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch http://review.whamcloud.com/18703/
Subject: LU-7796 utils: print error with valueless lctl set_param
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: e2973925ca550ad8ef7ad9af72d7d8766f9c1f41

[Emoly Liu]

Landed for 2.9.