scylla_raid_setup: make --online-discard argument useful

[vladzcloudius]

This argument was dead since its introduction and 'discard' was always configured regardless of its value.
This patch allows actually configuring things using this argument.

Fixes #14963

[scylladb-promoter]

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2906/

[tarzanek]

@syuu1228 can you review?

[vladzcloudius]

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2906/

I couldn't find the output of the failed test.
Can I get help with this, please, @roydahan?

[mykaul]

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2906/

I couldn't find the output of the failed test. Can I get help with this, please, @roydahan?

@roydahan is on PTO - I'm seeing https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2906/testReport/junit/update_cluster_layout_tests/TestUpdateClusterLayout/Tests___Sanity_Tests___test_add_node_with_large_partition4/ as a failure, which may be https://github.com/scylladb/scylla-dtest/issues/3384

[vladzcloudius]

CI state FAILURE - https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2906/

I couldn't find the output of the failed test. Can I get help with this, please, @roydahan?

@roydahan is on PTO - I'm seeing https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/2906/testReport/junit/update_cluster_layout_tests/TestUpdateClusterLayout/Tests___Sanity_Tests___test_add_node_with_large_partition4/ as a failure, which may be scylladb/scylla-dtest#3384

Got it. Thanks.
Then it's probably not related to this seemingly harmless patch (again, it doesn't change the current semantics of the argument) - the test above doesn't call scylla_raid_setup explicitly AFAICT.

[mykaul]

This PR has no tests (is that intentional? we may want not to repeat past mistakes and test both true and false values?)

[vladzcloudius]

This PR has no tests (is that intentional? we may want not to repeat past mistakes and test both true and false values?)

This PR doesn't add a new feature, @mykaul.
But I agree with you, our QA should do a home-check and verify how such basic breakage passed under their radars.

[mykaul]

This PR has no tests (is that intentional? we may want not to repeat past mistakes and test both true and false values?)

This PR doesn't add a new feature, @mykaul. But I agree with you, our QA should do a home-check and verify how such basic breakage passed under their radars.

Actually I'd expect more tests from Engineering, not QA, for such basic functionality. Especially if you fix something which lacked tests in the past, it makes sense to add them now (random example I've just seen - https://github.com/scylladb/scylla-enterprise/pull/3252 )

[vladzcloudius]

This PR has no tests (is that intentional? we may want not to repeat past mistakes and test both true and false values?)

This PR doesn't add a new feature, @mykaul. But I agree with you, our QA should do a home-check and verify how such basic breakage passed under their radars.

Actually I'd expect more tests from Engineering, not QA, for such basic functionality. Especially if you fix something which lacked tests in the past, it makes sense to add them now (random example I've just seen - scylladb/scylla-enterprise#3252 )

I agree, Engineering should definitely own that.
Unfortunately, I don't have bandwidth to help you on that.
Nor I know whether there is (and it's broken) or isn't a test for this specific API.

[tarzanek]

@vladzcloudius what about master and
acc408c ?
seems Takuya has it fixed

[vladzcloudius]

@vladzcloudius what about master and acc408c ? seems Takuya has it fixed

@tarzanek you are confusing the scylla_setup (patched by Takuya's patch) and scylla_raid_setup (which is the matter of this PR).

[vladzcloudius]

@mykaul @syuu1228 can we have this reviewed/merged, please? It's urgent and we urgently need this in the field.

[mykaul]

@syuu1228 - can you please review this?

[syuu1228]

How about to change the option for disabling discard, using action='store_true':

    parser.add_argument('--disable-online-discard', action='store_true',
                        help='Disable XFS online discard (trim SSD cells after file deletion)')

It will work like this:

$ scylla_raid_setup --disks /dev/sdb # enable discard
$ scylla_raid_setup --disks /dev/sdb --disable-online-discard # disable discard

Or, if we can't change option name due to keep compatibility, how about to use type=int:

    parser.add_argument('--online-discard', default=1, type=int,
                        help='Disable XFS online discard (trim SSD cells after file deletion)')

It will work like this:

$ scylla_raid_setup --disks /dev/sdb # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard 1 # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard 0 # disable discard

[vladzcloudius]

How about to change the option for disabling discard, using action='store_true':

    parser.add_argument('--disable-online-discard', action='store_true',
                        help='Disable XFS online discard (trim SSD cells after file deletion)')

It will work like this:

$ scylla_raid_setup --disks /dev/sdb # enable discard
$ scylla_raid_setup --disks /dev/sdb --disable-online-discard # disable discard

Or, if we can't change option name due to keep compatibility,

Exactly. We can't.

how about to use type=int:

    parser.add_argument('--online-discard', default=1, type=int,
                        help='Disable XFS online discard (trim SSD cells after file deletion)')

It will work like this:

$ scylla_raid_setup --disks /dev/sdb # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard 1 # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard 0 # disable discard

My PR supersedes your type=int proposal.

It allows all following usages:

$ scylla_raid_setup --disks /dev/sdb # enable discard

$ scylla_raid_setup --disks /dev/sdb --online-discard 1 # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard 0 # disable discard

$ scylla_raid_setup --disks /dev/sdb --online-discard true # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard false # disable discard

$ scylla_raid_setup --disks /dev/sdb --online-discard True # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard False # disable discard

[syuu1228]

My PR supersedes your type=int proposal.

It allows all following usages:

$ scylla_raid_setup --disks /dev/sdb # enable discard

$ scylla_raid_setup --disks /dev/sdb --online-discard 1 # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard 0 # disable discard

$ scylla_raid_setup --disks /dev/sdb --online-discard true # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard false # disable discard

$ scylla_raid_setup --disks /dev/sdb --online-discard True # enable discard
$ scylla_raid_setup --disks /dev/sdb --online-discard False # disable discard

Okay, I agree your PR is more flexible than type=int.

[syuu1228]

LGTM