Validation for cluster.brick-multiplex cluster option. #1373

vpandey-RH · 2018-12-06T18:58:25Z

Updates #1367

Add validation to setting cluster.brick-multiplex cluster option.
Discard all garbage strings and allow only "on", "yes", "true", "enable", "1" OR
"off", "no", "false", "disable", "0"

Signed-off-by: Vishal Pandey [email protected]

centos-ci · 2018-12-06T18:58:27Z

Can one of the admins verify this patch?

aravindavk · 2018-12-07T03:24:39Z

add to whitelist

Madhu-1

please add e2e for this fix.

atinmu

We already have the validator function in ClusterOptMap which defines the respective validation function per cluster option. We need to check why the validation isn't being happening through that infra. I don't think we should add validation like this for individual options instead of a generic framework.

vpandey-RH · 2018-12-07T05:36:15Z

@atinmu The validations are happening through that infra. Only thing is that we call RegisterClusterOpValidationFunc() to register these validation functions.
We can add validations directly in clusterOptMap. But, since we do have a method to register the validation function, so I am thinking of leaving the framework as generalised as possible and just use the functions already made available to register the options validator. Even if we add validation functions in clusterOptMap, those validation functions structure is bound to be same to what's happening now.

vpandey-RH · 2018-12-07T08:37:49Z

@atinmu As per our discussion in the morning, I tried using ValidateBool () but its not possible as the validation function for cluster options expects a function with string arguments and the ValidateBool() function has different types of arguments to what is expected for cluster option validation function.

atinmu

Also CI seems to have failed from a shd test, please have a look.

atinmu · 2018-12-10T04:31:43Z

e2e/brickmux_test.go

 optReq := api.ClusterOptionReq{
- Options: map[string]string{"cluster.brick-multiplex": "on"},
+ Options: map[string]string{"cluster.brick-multiplex": "invalidValue"},


so we don't have a test where we set brick multiplexing to off now?

No, we don't have a test for this as of now but I will add it.

atinmu · 2018-12-11T08:31:22Z

@vpandey-RH Can you please address the comment?

vpandey-RH · 2018-12-11T08:36:25Z

@atinmu The shd test is failing because max-bricks-per-process is still not merged and in all shd tests I am killing one of the bricks from volume, so if brick-multiplexing is on, killing one brick will result in all bricks going in offline state and that's why the CI fails, saying transport endpoint is not connected.

atinmu · 2018-12-11T11:20:23Z

@vpandey-RH you should be testing shd on a multi node cluster like environment, running a replica config on the same node isn’t ideal where such behavior can’t be tested. Agree?

atinmu · 2018-12-15T05:58:00Z

@vpandey-RH CI still fails.

rishubhjain · 2018-12-15T06:01:13Z

retest this please

Madhu-1 · 2018-12-15T06:11:15Z

11:40:11 --- FAIL: TestVolume/Statedump (1.03s)
11:40:11 require.go:157:
11:40:11 Error Trace: volume_ops_test.go:530
11:40:11 Error: Not equal:
11:40:11 expected: 2
11:40:11 actual : 8
11:40:11 Test: TestVolume/Statedump
11:40:11 --- PASS: TestVolume/Stop (0.66s)
11:40:11 --- PASS: TestVolume/List (0.04s)
11:40:11 --- PASS: TestVolume/Info (0.01s)
11:40:11 --- PASS: TestVolume/Edit (0.05s)
11:40:11 --- PASS: TestVolume/VolumeFlags (0.71s)
11:40:11 --- PASS: TestVolume/Delete (0.02s)
11:40:11 --- PASS: TestVolume/Disperse (0.47s)
11:40:11 --- PASS: TestVolume/DisperseMount (0.14s)
11:40:11 --- PASS: TestVolume/DisperseDelete (0.26s)
11:40:11 --- PASS: TestVolume/testShdOnVolumeStartAndStop (1.90s)
11:40:11 --- PASS: TestVolume/testArbiterVolumeCreate (0.88s)
11:40:11 --- FAIL: TestVolume/SelfHeal (0.62s)
11:40:11 require.go:765:
11:40:11 Error Trace: glustershd_test.go:94
11:40:11 utils_test.go:33
11:40:11 Error: Expected nil, but got: &os.PathError{Op:"open", Path:"/tmp/gd2_func_test/TestVolume/SelfHeal/mnt020307401/file1.txt", Err:0x6b}
11:40:11 Test: TestVolume/SelfHeal
11:40:11 Messages: failed to open file: open /tmp/gd2_func_test/TestVolume/SelfHeal/mnt020307401/file1.txt: transport endpoint is not connected
11:40:11 --- FAIL: TestVolume/GranularEntryHeal (0.01s)
11:40:11 require.go:765:
11:40:11 Error Trace: glustershd_test.go:195
11:40:11 utils_test.go:33
11:40:11 Error: Expected nil, but got: &errors.errorString{s:"volume already exists"}
11:40:11 Test: TestVolume/GranularEntryHeal
11:40:11 --- FAIL: TestVolume/SelfHeal#01 (0.01s)
11:40:11 require.go:765:
11:40:11 Error Trace: glustershd_test.go:53
11:40:11 utils_test.go:33
11:40:11 Error: Expected nil, but got: &errors.errorString{s:"volume already exists"}
11:40:11 Test: TestVolume/SelfHeal#01
11:40:11 --- FAIL: TestVolume/SplitBrainOperations (0.49s)
11:40:11 require.go:347:
11:40:11 Error Trace: glustershd_test.go:309
11:40:11 utils_test.go:33
11:40:11 Error: Should be false
11:40:11 Test: TestVolume/SplitBrainOperations
11:40:11 Messages: glustershd is still running
11:40:11 --- PASS: TestVolume/VolumeProfile (1.04s)
11:40:11 === RUN TestVolumeOptions

@vpandey-RH CI failure PTAL

Discard all garbage strings and allow only "on", "yes", "true", "enable", "1" OR "off", "no", "false", "disable", "0" Signed-off-by: Vishal Pandey <[email protected]>

atinmu · 2018-12-24T13:52:08Z

@vpandey-RH any update on this PR?

vpandey-RH · 2018-12-26T05:54:52Z

@atinmu Still working on this. Stuck in an issue regarding local mount by glfsheal binary because of which the self heal tests are failing.

atinmu · 2019-01-07T05:26:39Z

@atinmu Still working on this. Stuck in an issue regarding local mount by glfsheal binary because of which the self heal tests are failing.

@vpandey-RH Did we manage to figure out the root cause?

vpandey-RH · 2019-01-07T05:29:56Z

@atinmu No. I have asked @aravindavk for some help.

ghost assigned vpandey-RH Dec 6, 2018

ghost added the in progress label Dec 6, 2018

rishubhjain approved these changes Dec 6, 2018

View reviewed changes

ghost assigned Madhu-1 Dec 7, 2018

Madhu-1 suggested changes Dec 7, 2018

View reviewed changes

atinmu suggested changes Dec 7, 2018

View reviewed changes

vpandey-RH force-pushed the add-validations-to-brickmux-option branch from 79c0ecd to fca2699 Compare December 7, 2018 08:57

atinmu suggested changes Dec 10, 2018

View reviewed changes

ghost assigned atinmu Dec 11, 2018

Add validation to setting cluster.brick-multiplex cluster option.

ebb281e

Discard all garbage strings and allow only "on", "yes", "true", "enable", "1" OR "off", "no", "false", "disable", "0" Signed-off-by: Vishal Pandey <[email protected]>

vpandey-RH force-pushed the add-validations-to-brickmux-option branch from 60e5aa2 to ebb281e Compare December 17, 2018 09:20

Merge branch 'master' into add-validations-to-brickmux-option

2c9eb06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validation for cluster.brick-multiplex cluster option. #1373

Validation for cluster.brick-multiplex cluster option. #1373

vpandey-RH commented Dec 6, 2018

centos-ci commented Dec 6, 2018

aravindavk commented Dec 7, 2018

Madhu-1 left a comment

atinmu left a comment

vpandey-RH commented Dec 7, 2018

vpandey-RH commented Dec 7, 2018

atinmu left a comment

atinmu Dec 10, 2018

vpandey-RH Dec 10, 2018

atinmu commented Dec 11, 2018

vpandey-RH commented Dec 11, 2018

atinmu commented Dec 11, 2018

atinmu commented Dec 15, 2018

rishubhjain commented Dec 15, 2018

Madhu-1 commented Dec 15, 2018

atinmu commented Dec 24, 2018

vpandey-RH commented Dec 26, 2018

atinmu commented Jan 7, 2019

vpandey-RH commented Jan 7, 2019

Validation for cluster.brick-multiplex cluster option. #1373

Are you sure you want to change the base?

Validation for cluster.brick-multiplex cluster option. #1373

Conversation

vpandey-RH commented Dec 6, 2018

centos-ci commented Dec 6, 2018

aravindavk commented Dec 7, 2018

Madhu-1 left a comment

Choose a reason for hiding this comment

atinmu left a comment

Choose a reason for hiding this comment

vpandey-RH commented Dec 7, 2018

vpandey-RH commented Dec 7, 2018

atinmu left a comment

Choose a reason for hiding this comment

atinmu Dec 10, 2018

Choose a reason for hiding this comment

vpandey-RH Dec 10, 2018

Choose a reason for hiding this comment

atinmu commented Dec 11, 2018

vpandey-RH commented Dec 11, 2018

atinmu commented Dec 11, 2018

atinmu commented Dec 15, 2018

rishubhjain commented Dec 15, 2018

Madhu-1 commented Dec 15, 2018

atinmu commented Dec 24, 2018

vpandey-RH commented Dec 26, 2018

atinmu commented Jan 7, 2019

vpandey-RH commented Jan 7, 2019