scylla_raid_setup: --online-discard option always enables 'discard'

[vladzcloudius]

Installation details
HEAD: Scylla version (or git commit hash):

Description
scylla_raid_setup will always enable discard mount option even if --online-discard False is given.

Root cause
argparse doesn't support boolean parameter.
To encode such values one should either use action='store_true' (preferred way) or decode the string value using distutils.util.strtobool(value)

Unless action='store_true' is used (which is not the case) the if args.online_discard is going to give an unexpected result - it will be True when the value is set (to any value - which happens to be always the case since the default is provided as well).

While the code that has the condition above assumes that it checks the actual value of the variable.

[vladzcloudius]

cc @roydahan @avikivity
Missing/bogus test alert.

[vladzcloudius]

cc @tarzanek @igorribeiroduarte @ebenzecri @tomer-sandler @noellymedina @AdamStawarz

[mykaul]

I did not read the code, so just asking - if you don't want this enabled, wouldn't it be simpler not to pass '--online-discard' ? (or is the default true, so effectively you can't disable it?)

[vladzcloudius]

I did not read the code, so just asking - if you don't want this enabled, wouldn't it be simpler not to pass '--online-discard' ? (or is the default true, so effectively you can't disable it?)

@mykaul
If you don't pass --online-discard today the idea of the author (@avikivity) was to have the option enabled. And this GH issue is not about changing the current semantics (as much as I hate it and disagree with it).

And that was the only thing that the author tested apparently before sending the patch that added this patch (a19d00e) - the default works as expected ;)

This issue is about the fact that the non-default (when you give --online-discard false) doesn't work. And as far as I can tell - it never worked.

So, let's fix this ASAP and backport to all our currently supported official releases.

The PR with the fix has been sent on Friday (3 days ago).

[roydahan]

I vaguely recall a long discussion about this without reaching agreement on the test / behavior or even the necessity of this option.
IIRC, @soyacz was involved in this. Do you recall that issue?

[soyacz]

@roydahan I don't think it was me or you mixed with scylladb/scylla-machine-image#394

[roydahan]

@roydahan I don't think it was me or you mixed with scylladb/scylla-machine-image#394

Yes, you're right, I mixed it with it.

[DoronArazii]

@scylladb/scylla-maint please consider backporting all the way to 2022.1

[denesb]

Backported to 5.2, 5.1 and 2022.1.