Bug 1274464

We are a bit limited by the backend configobj parser, but proposing the following syntax:

[bootloader]
bootcmdline=OPTS1
bootcmdline1=+OPTS2
bootcmdline2=-OPTS3
...

In this case bootcmldine will be '(OPTS1 + OPTS2) \ OPTS3, e.g.:

[bootloader]
bootcmdline=rhgb quiet debug
bootcmdline1=+isolcpus=1
bootcmdline2=-rhgb debug

Result:
bootcmdline='quiet isolcpus=1'

It will allow unlimited 'bootcmdlineSUFFIX' entries, where suffix can be any string. The =+ operator will add entries, the =- will remove the entries.

But it has various consequences due to built-in inheritance:

parent profile:
[bootloader]
bootcmdline=OPTS_PARENT_1
bootcmdline1=+OPTS_PARENT_2

child profile:
[main]
include=parent

[bootloader]
bootcmdline1=+OPTS_CHILD_1

The result will be due to inheritance:
bootcmdline='OPTS_PARENT_1 OPTS_CHILD_1'

This can be resolved by using unique suffix, e.g.:
child profile:
[main]
include=parent

[bootloader]
bootcmdline_child1=+OPTS_CHILD_1

The result will be:
bootcmdline='OPTS_PARENT_1 OPTS_PARENT_2 OPTS_CHILD_1'

Created attachment 1247546 [details]
Patch proposed by Ondra

Ondra I think your patch is quite good and fully fulfill the assignment, but I think there are few things which could be improved (well this is mostly my subjective point of view, so opinion may vary):

- the escaping of '+' and '-' seems to be needlessly complex and I think it could be confusing for users, i.e. in the proposed patch only the first '+' / '-' seems to be taken into account, so you don't need to process escaping for all '+' / '-'. I think it's just enough to say: the first '+' or '-' is modifier and the rest of the string is taken literally, if there is '+' or '-' alone, it's taken literally.

- I think if there is no '+' or '-' modifier in the beginning the '+' should be implicit, e.g.:
cmdline2=quiet
should add 'quiet', this seems more intuitive for me

- I think it should also process following strings:
cmdline2= - quiet
should remove 'quiet'

- I think it shouldn't add empty strings
cmdline2=+
cmdline3=+quiet
could add '+ quiet' or 'quiet', but it shouldn't add ' quiet'

And one objective remark:
- please follow coding style of the project especially regarding tabs/spaces

Created attachment 1247551 [details]
Proposed patch

I propose the following patch which is based on patch from comment 6. I didn't fully test it, so feedback is highly welcome.

Also I think for further implementation of RFEs and related discussion we could move to github pull requests and link the pull request to the appropriate bugzilla to be in sync.

(In reply to Jaroslav Škarvada from comment #7)
> - the escaping of '+' and '-' seems to be needlessly complex and I think it
> could be confusing for users, i.e. in the proposed patch only the first '+'
> / '-' seems to be taken into account, so you don't need to process escaping
> for all '+' / '-'. I think it's just enough to say: the first '+' or '-' is
> modifier and the rest of the string is taken literally, if there is '+' or
> '-' alone, it's taken literally.
> 
> - I think if there is no '+' or '-' modifier in the beginning the '+' should
> be implicit, e.g.:
> cmdline2=quiet
> should add 'quiet', this seems more intuitive for me

Yes, your solution is simpler. But is doesn't provide a way for you to completely override the cmdline in a child profile. You would have to remove the options using the cmdline=- syntax. Which also implies that when parent changes in the cmdline option, you have to change the child too.

> - I think it should also process following strings:
> cmdline2= - quiet
> should remove 'quiet'
> 
> - I think it shouldn't add empty strings
> cmdline2=+
> cmdline3=+quiet
> could add '+ quiet' or 'quiet', but it shouldn't add ' quiet'

Agreed.

> And one objective remark:
> - please follow coding style of the project especially regarding tabs/spaces

Sorry about that. My editor gets confused when it sees a python file :).

(In reply to Ondřej Lysoněk from comment #10)
> (In reply to Jaroslav Škarvada from comment #7)
> > - the escaping of '+' and '-' seems to be needlessly complex and I think it
> > could be confusing for users, i.e. in the proposed patch only the first '+'
> > / '-' seems to be taken into account, so you don't need to process escaping
> > for all '+' / '-'. I think it's just enough to say: the first '+' or '-' is
> > modifier and the rest of the string is taken literally, if there is '+' or
> > '-' alone, it's taken literally.
> > 
> > - I think if there is no '+' or '-' modifier in the beginning the '+' should
> > be implicit, e.g.:
> > cmdline2=quiet
> > should add 'quiet', this seems more intuitive for me
> 
> Yes, your solution is simpler. But is doesn't provide a way for you to
> completely override the cmdline in a child profile. You would have to remove
> the options using the cmdline=- syntax. Which also implies that when parent
> changes in the cmdline option, you have to change the child too.
> 
I think this is handled by the replace keyword e.g.:

parent:
[bootloader]
cmdline=opt1 opt2
cmdline2=+opt3
cmdline3=+opt4

child:
[bootloader]
replace=true
cmdline=opt5
cmdline2=+opt6

resulting cmdline:
opt5 opt6

I think this is more proper regarding the inheritance logic. Because current default handling strategy of child profiles is 'merge', the replace keyword changes it to the 'replace' strategy.

The replace option is used to replace the _whole_ plugin configuration. It's not for overriding individual options.

IMHO your patch breaks the inheritance. And it definitely breaks current behaviour (so does mine of course, but less :)). If someone had this configuration:

parent:
[bootloader]
cmdline = something

child:
[bootloader]
cmdline = something_else

then suddenly, they would have the effective cmdline equal to 'something something_else', which is probably not what they expect.

I designed my patch so that it breaks current behaviour as little as reasonably possible. That's also why I do the escaping only at the beginning of the value - I figured it would break less of people's current configurations. (Also, I think it's more likely that someone has a configuration similar to the one outlined above, than that someone has a plus sign in the very beginning of the cmdline option.)

Actually I just realized I was wrong about the example in c#12. The cmdline is overriden as expected in this case, because of the merge.

Alright, I suppose I'm OK with your solution. The need to use 'replace' keyword doesn't matter all that much, as the bootloader plugin currently recognizes only one other option.

(In reply to Ondřej Lysoněk from comment #12)
> The replace option is used to replace the _whole_ plugin configuration. It's
> not for overriding individual options.
>
AFAIK it's to clear all options and define only those which are explicitly specified. I.e. with replace=true user have to set everything right and not depend on inheritance. In such case command line will be set exactly to the specified value. IIRC there is only grub2_cfg_file option which is rarely used. But if user needs it to set to anything different from the default he has to explicitly specify it. I think it's OK.

> IMHO your patch breaks the inheritance.
>
Well, the bootloader plugin is quite specific regarding the inheritance and we are trying to figure out the most optimal way how to do it.

> And it definitely breaks current
> behaviour (so does mine of course, but less :)). If someone had this
> configuration:
> 
> parent:
> [bootloader]
> cmdline = something
> 
> child:
> [bootloader]
> cmdline = something_else
> 
> then suddenly, they would have the effective cmdline equal to 'something
> something_else', which is probably not what they expect.
>
The current inheritance handling in Tuned is not the best (that's why we introduced the cmdline* approach), but the described behavior is according to the docs and how it behaved previously, i.e. by default it merges all options and redefine conflicting options with the last specified option. I.e. before the patch is applied the example above should result in "something_else" and so it resulted after the patch is applied.

> I designed my patch so that it breaks current behaviour as little as
> reasonably possible. That's also why I do the escaping only at the beginning
> of the value - I figured it would break less of people's current
> configurations. (Also, I think it's more likely that someone has a
> configuration similar to the one outlined above, than that someone has a
> plus sign in the very beginning of the cmdline option.)
>
Escaping on the beginning is OK, but the results could be a bit surprising when it came to counting the number of "pluses" and comparison with the previous behaviour.

(In reply to Ondřej Lysoněk from comment #13)
> Actually I just realized I was wrong about the example in c#12. The cmdline
> is overriden as expected in this case, because of the merge.

Yup, IIRC it's handled by merger.py.

(In reply to Ondřej Lysoněk from comment #14)
> Alright, I suppose I'm OK with your solution. The need to use 'replace'
> keyword doesn't matter all that much, as the bootloader plugin currently
> recognizes only one other option.

I think the solution proposed by patch in comment 8 is low breaking, logical, and very simple change while still being quite effective. Well, it's probably not the best solution, but feel free to rewrite it or propose anything different. We are just brainstorming - that's the whole Tuned project about - proof of concepts and result of brainstorming ideas.

(In reply to Jaroslav Škarvada from comment #17)
> We are just brainstorming - that's the whole Tuned
> project about - proof of concepts and result of brainstorming ideas.

That's how I take it :).

Ondra thanks for help, committing upstream, we can still change it upstream.

Upstream commit adding the feature:
https://github.com/redhat-performance/tuned/commit/66d550b94d6e7198043ea0c8379fa330da947a81

Now it supports the following extended syntax:

parent profile:
[bootloader]
cmdline=opt1 opt2

child profile:
[bootloader]
cmdline1=+opt3
cmdline2=opt4
cmdline3=-opt2

Resulting cmdline:
opt1 opt3 opt4

It recognizes "cmdline*" and postprocess it to extend the inheritance.
It recognizes '+' and '-' on the very beginning of the option and adds
or removes the string from the command line. When using
'+' or '-' it takes the string after the sign till end of the line. To
add option starting with e.g. '+' you need to use '++opt'. If '+' is
omitted in succesive command lines, it's taken implicitly (e.g. opt4).

If you need to replace the whole command line in child profile, use:

[bootloader]
replace=true
cmdline=opt1 opt2

This will drop all 'cmdline*' setting from the parent profile(s).

New feature was verified in tuned-2.8.0-4.el7.noarch
No major bugs found --> VERIFIED

One small problem found when using 'replace=false'. See new bug BZ#1461279.

Automated test case were created:
/CoreOS/tuned/Regression/add-bootloader-plugin-for-heredity-of-cmdline-options

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHBA-2017:2102