mon: change osdmap flags set and unset messages #9252

vumrao · 2016-05-22T08:27:02Z

mon : change osdmap flags set and unset message

Fixes: http://tracker.ceph.com/issues/15983

Signed-off-by: Vikhyat Umrao vumrao@redhat.com

vumrao · 2016-05-22T08:31:18Z

I have tested this change with vstart cluster and now results are as follows :
change log 1 : after Kefu suggestion now messages would be : Error EINVAL: noout flag is already set and Error EINVAL: noout flag is not set
change log 2 : after Sage inputs and discussion with Kefu now it is same as earlier before change log 1, we should return true without any error in ceph osd set and unset commands.

$ ./ceph osd set noout
set noout

$./ceph -s
    cluster 23bf9597-e8ae-4d04-9c49-4109f5c4bfcd
     health HEALTH_WARN
            noout,sortbitwise flag(s) set
     monmap e1: 3 mons at {a=192.168.200.47:6789/0,b=192.168.200.47:6790/0,c=192.168.200.47:6791/0}
            election epoch 6, quorum 0,1,2 a,b,c
      fsmap e5: 1/1/1 up {0=b=up:active}, 2 up:standby
     osdmap e12: 3 osds: 3 up, 3 in
            flags noout,sortbitwise
      pgmap v40: 24 pgs, 3 pools, 2068 bytes data, 20 objects
            143 GB used, 156 GB / 299 GB avail
                  24 active+clean

$ ./ceph osd set noout
noout flag is already set

$ ./ceph osd unset noout
unset noout

$ ./ceph -s
    cluster 23bf9597-e8ae-4d04-9c49-4109f5c4bfcd
     health HEALTH_OK
     monmap e1: 3 mons at {a=192.168.200.47:6789/0,b=192.168.200.47:6790/0,c=192.168.200.47:6791/0}
            election epoch 6, quorum 0,1,2 a,b,c
      fsmap e5: 1/1/1 up {0=b=up:active}, 2 up:standby
     osdmap e14: 3 osds: 3 up, 3 in
            flags sortbitwise
      pgmap v108: 24 pgs, 3 pools, 2068 bytes data, 20 objects
            143 GB used, 156 GB / 299 GB avail
                  24 active+clean

$ ./ceph osd unset noout
 noout flag is not set

tchaikov · 2016-05-23T14:44:11Z

@vumrao could you add a simple test in qa/workunits/cephtool/test.sh?

tchaikov · 2016-05-23T14:47:17Z

src/mon/OSDMonitor.cc

-  pending_inc.new_flags |= flag;
-  ss << "set " << OSDMap::get_flag_string(flag);
+  if (osdmap.test_flag(flag)) {
+    ss << OSDMap::get_flag_string(flag) << " flag is already set";


could return false here right away.

After applying your suggestion , I tested $ceph osd set and $ceph osd unset commands just hangs if I try to return false without calling wait_for_finished_proposal() in if condition and if I try to return false with calling wait_for_finished_proposal() then also same behavior.

To me it looks like as $ceph osd set and $ceph osd unset both expect to return true with wait_for_finished_proposal() as I was doing.

As if we are setting a already set flag means condition is true as this is the behavior of $ceph osd set we should return true as flag is already set.

Same goes for unset as in unset also if we unset a flag which is not set then also we should return true as $ceph osd unset behavior is to flag should be unset and which is the case here.

Please let me know if my understanding is wrong.

vumrao · 2016-05-23T14:57:07Z

@tchaikov sure will do it.

vumrao · 2016-05-23T19:15:36Z

@tchaikov I have added a simple test in qa/workunits/cephtool/test.sh. I am not sure if this is how I should write, as this is first test case for me :)

Currently make check is running in my local branch.

vumrao · 2016-05-24T07:12:33Z

Thank you @tchaikov for your help and nice discussion in #ceph-devel. As discussed will make the changes and will send the updated patch.

vumrao · 2016-05-24T09:34:46Z

Need to look into sortbitwise flag test as this flag is by default set :

- qa/workunits/cephtool/test.sh:1096: test_mon_osd:  for f in noup nodown noin noout noscrub
   nodeep-scrub nobackfill norebalance norecover notieragent full sortbitwise
- qa/workunits/cephtool/test.sh:1098: test_mon_osd:  ceph osd set sortbitwise
   Error EINVAL: sortbitwise flag is already set

Need to look into full flag test as this flag gets unset if any OSD in cluster is not full and hence:

 - qa/workunits/cephtool/test.sh:1096: test_mon_osd:  for f in noup nodown noin noout noscrub
    nodeep-scrub nobackfill norebalance norecover notieragent full sortbitwise
 - qa/workunits/cephtool/test.sh:1098: test_mon_osd:  ceph osd set full
    set full

-  As it got unset in between as there was no OSD in cluster which was full 

 -  qa/workunits/cephtool/test.sh:1099: test_mon_osd:  ceph osd unset full
    Error EINVAL: full flag is not set

vumrao · 2016-05-24T14:45:34Z

I have tested this current commit locally and now it is not failing in code change and it has one fail test in chain_xattr which is not related to code change in this commit :

[       OK ] chain_xattr.chunk_aligned (2272 ms)
[ RUN      ] chain_xattr.listxattr
[       OK ] chain_xattr.listxattr (85 ms)
[ RUN      ] chain_xattr.fskip_chain_cleanup_and_ensure_single_attr
test/objectstore/chain_xattr.cc:371: Failure
Value of: 1UL
  Actual: 1
Expected: get_xattrs(fd).size()
Which is: 2
[  FAILED  ] chain_xattr.fskip_chain_cleanup_and_ensure_single_attr (0 ms)
[ RUN      ] chain_xattr.skip_chain_cleanup_and_ensure_single_attr
test/objectstore/chain_xattr.cc:415: Failure

tchaikov · 2016-05-24T15:57:35Z

@vumrao are you using selinux could you check the files in your testdir to see if it has some xattr already?

vumrao · 2016-05-24T16:11:00Z

@tchaikov yes you are right Kefu, I have selinux enabled in my local box and also xattr file :

 $ getenforce 
Enforcing

 $ ll chain_xattr 
-rwx------. 1 vumrao vumrao 0 May 24 19:43 chain_xattr

$ getfattr -m . -d -e hex chain_xattr 
# file: chain_xattr
security.selinux=0x756e636f6e66696e65645f753a6f626a6563745f723a757365725f686f6d655f743a733000
user.foo=0xb8b8b8b8b8b8b8b8b8b8b8b8b 
[....]

tchaikov · 2016-05-24T16:37:20Z

lgtm.

@vumrao i think we can either fix this or leave as it is atm. 5abc95d fixed chain_xattr.listxattr last time.

liewegas · 2016-05-24T20:05:30Z

This isn't right. The mon commands must be idempotent. There is a debug option somewhere in fact that sends every command twice and asserts that if the first succeeds, teh second succeeds too.

vumrao · 2016-05-25T05:40:59Z

Thanks Sage for your inputs. I had discussion with Kefu will modify the patch.

vumrao · 2016-05-25T06:59:23Z

make[3]: *** [libec_jerasure.la] Error 1
../libtool: line 9822: .libs/libceph_example.lai: No space left on device
ln: failed to create symbolic link 'libceph_example.la': No space left on device
make[3]: *** [libceph_example.la] Error 1

vumrao · 2016-05-25T06:59:44Z

test this please.

tchaikov · 2016-05-25T09:34:32Z

@vumrao thanks filed http://tracker.ceph.com/issues/16014

vumrao · 2016-05-27T10:43:55Z

test this please.

tchaikov · 2016-05-27T11:58:20Z

test this please.

tchaikov · 2016-06-05T06:23:09Z

qa/workunits/cephtool/test.sh

+  ceph osd set noout >& $TMPFILE || return 1
+  check_response "set noout"
+  ceph osd set noout >& $TMPFILE || return 1
+  expect_failure $TMPDIR "noout flag is already set"


@vumrao this should not fail. as per sage

The mon commands must be idempotent

@kchai ahh sorry , right it should be check_response() instead of expect_failure().
Right, as sage pointed earlier:

The mon commands must be idempotent

I think it will apply to all the new tests for new messages ? will change it or do you think any other change is needed.

yeah. just the new messages.

Thanks Kefu, sure will make the changes and test it locally and then will send the updated commit.

tchaikov · 2016-06-13T06:42:52Z

lgtm.

vumrao · 2016-06-29T18:30:23Z

This branch has conflicts. Doing rebase.

tchaikov · 2016-07-04T14:55:50Z

missed it in last qa batch. will include it in next run.

vumrao · 2016-07-04T15:01:42Z

@tchaikov thank you !

liewegas · 2016-07-04T15:23:20Z

This PR is still wrong. At any time the MonClient may resend a command (e.g., due ot a transient network disconnect, or a mon quorum change). Making messages like "set foo" and "foo already set" will be incorrect in some cases. This is why we try to use messages like "flag foo is (or was already) set".

vumrao · 2016-07-05T15:10:33Z

Thank you Sage for your inputs. I will change the messages and will modify the test cases.

Fixes: http://tracker.ceph.com/issues/15983 Signed-off-by: Vikhyat Umrao <vumrao@redhat.com>

tchaikov · 2016-07-07T06:33:11Z

lgtm. @liewegas what do you think?

liewegas · 2016-07-07T20:37:04Z

looks good, thanks!

tchaikov · 2016-07-11T03:51:08Z

tested in http://pulpito.ceph.com/kchai-2016-07-10_07:48:29-rados-wip-kefu-testing---basic-mira/

the non-environmental failures are addressed by #10234 .

vumrao changed the title ~~mon : add osdmap flags set and unset message when~~ mon : add osdmap flags set and unset message when osdmap flags are already set and unset May 22, 2016

tchaikov added feature core labels May 22, 2016

tchaikov reviewed May 23, 2016
View reviewed changes

vumrao force-pushed the wip-vumrao-15983 branch 3 times, most recently from e2952af to 16db7e0 Compare May 23, 2016 19:05

vumrao force-pushed the wip-vumrao-15983 branch 3 times, most recently from c606f49 to fb1d3a5 Compare May 24, 2016 07:51

vumrao force-pushed the wip-vumrao-15983 branch from fb1d3a5 to 072eea9 Compare May 24, 2016 14:43

vumrao force-pushed the wip-vumrao-15983 branch from 072eea9 to a30e587 Compare May 24, 2016 14:47

tchaikov added the needs-qa label May 24, 2016

liewegas removed the needs-qa label May 24, 2016

vumrao force-pushed the wip-vumrao-15983 branch from a30e587 to 9729bba Compare May 25, 2016 06:25

tchaikov added needs-qa wip-kefu-testing labels Jun 2, 2016

tchaikov reviewed Jun 5, 2016
View reviewed changes

tchaikov removed needs-qa wip-kefu-testing labels Jun 5, 2016

vumrao force-pushed the wip-vumrao-15983 branch from 9729bba to 15a9ff8 Compare June 5, 2016 11:19

tchaikov added the needs-qa label Jun 13, 2016

vumrao force-pushed the wip-vumrao-15983 branch from 15a9ff8 to 1decb71 Compare June 29, 2016 18:31

tchaikov self-assigned this Jul 4, 2016

tchaikov removed the needs-qa label Jul 4, 2016

mon : change osdmap flags set and unset message

28c35fb

Fixes: http://tracker.ceph.com/issues/15983 Signed-off-by: Vikhyat Umrao <vumrao@redhat.com>

vumrao force-pushed the wip-vumrao-15983 branch from 1decb71 to 28c35fb Compare July 7, 2016 06:25

vumrao changed the title ~~mon : add osdmap flags set and unset message when osdmap flags are already set and unset~~ mon : change osdmap flags set and unset message Jul 7, 2016

vumrao changed the title ~~mon : change osdmap flags set and unset message~~ mon : change osdmap flags set and unset messages Jul 7, 2016

liewegas changed the title ~~mon : change osdmap flags set and unset messages~~ mon: change osdmap flags set and unset messages Jul 7, 2016

tchaikov added needs-qa wip-kefu-testing labels Jul 8, 2016

tchaikov merged commit 35ce55d into ceph:master Jul 11, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mon: change osdmap flags set and unset messages #9252

mon: change osdmap flags set and unset messages #9252

vumrao commented May 22, 2016 •

edited

vumrao commented May 22, 2016 •

edited

tchaikov commented May 23, 2016

tchaikov May 23, 2016

vumrao May 23, 2016

vumrao May 23, 2016 •

edited

vumrao commented May 23, 2016

vumrao commented May 23, 2016

vumrao commented May 24, 2016

vumrao commented May 24, 2016

vumrao commented May 24, 2016

tchaikov commented May 24, 2016

vumrao commented May 24, 2016

tchaikov commented May 24, 2016

liewegas commented May 24, 2016

vumrao commented May 25, 2016

vumrao commented May 25, 2016

vumrao commented May 25, 2016

tchaikov commented May 25, 2016

vumrao commented May 27, 2016

tchaikov commented May 27, 2016

tchaikov Jun 5, 2016

vumrao Jun 5, 2016

tchaikov Jun 5, 2016

vumrao Jun 5, 2016

tchaikov commented Jun 13, 2016

vumrao commented Jun 29, 2016

tchaikov commented Jul 4, 2016

vumrao commented Jul 4, 2016

liewegas commented Jul 4, 2016

vumrao commented Jul 5, 2016

tchaikov commented Jul 7, 2016

liewegas commented Jul 7, 2016

tchaikov commented Jul 11, 2016

mon: change osdmap flags set and unset messages #9252

mon: change osdmap flags set and unset messages #9252

Conversation

vumrao commented May 22, 2016 • edited

vumrao commented May 22, 2016 • edited

tchaikov commented May 23, 2016

tchaikov May 23, 2016

Choose a reason for hiding this comment

vumrao May 23, 2016

Choose a reason for hiding this comment

vumrao May 23, 2016 • edited

Choose a reason for hiding this comment

vumrao commented May 23, 2016

vumrao commented May 23, 2016

vumrao commented May 24, 2016

vumrao commented May 24, 2016

vumrao commented May 24, 2016

tchaikov commented May 24, 2016

vumrao commented May 24, 2016

tchaikov commented May 24, 2016

liewegas commented May 24, 2016

vumrao commented May 25, 2016

vumrao commented May 25, 2016

vumrao commented May 25, 2016

tchaikov commented May 25, 2016

vumrao commented May 27, 2016

tchaikov commented May 27, 2016

tchaikov Jun 5, 2016

Choose a reason for hiding this comment

vumrao Jun 5, 2016

Choose a reason for hiding this comment

tchaikov Jun 5, 2016

Choose a reason for hiding this comment

vumrao Jun 5, 2016

Choose a reason for hiding this comment

tchaikov commented Jun 13, 2016

vumrao commented Jun 29, 2016

tchaikov commented Jul 4, 2016

vumrao commented Jul 4, 2016

liewegas commented Jul 4, 2016

vumrao commented Jul 5, 2016

tchaikov commented Jul 7, 2016

liewegas commented Jul 7, 2016

tchaikov commented Jul 11, 2016

vumrao commented May 22, 2016 •

edited

vumrao commented May 22, 2016 •

edited

vumrao May 23, 2016 •

edited