Skip to content

Improve ConsumerStateTable: del after consumed, and let consumer writes the final table#204

Merged
qiluo-msft merged 5 commits intosonic-net:masterfrom
qiluo-msft:qiluo/statequeuefix
Jul 27, 2018
Merged

Improve ConsumerStateTable: del after consumed, and let consumer writes the final table#204
qiluo-msft merged 5 commits intosonic-net:masterfrom
qiluo-msft:qiluo/statequeuefix

Conversation

@qiluo-msft
Copy link
Copy Markdown
Contributor

@qiluo-msft qiluo-msft commented May 30, 2018 •
edited
Loading

Original implementation will have 2 issues:

  1. The consumer side will accumulate all the state into redis hashes, and each time a new field/value, the consumer will pop it, and return all the filed/value accumulated.
  2. The producer side modify the redis hashes directly. To behavior the same way as Producer/ConsumerTable, let the consumer side modify the redis hashes.

@kcudnik
Copy link
Copy Markdown
Contributor

kcudnik commented May 30, 2018 •
edited by qiluo-msft
Loading

whats the motivation behind this pr? #Resolved

@qiluo-msft
Copy link
Copy Markdown
Contributor Author

qiluo-msft commented May 30, 2018 •
edited
Loading

@kcudnik I modified the first comment to answer your question. #Closed

jipanyang
jipanyang reviewed May 30, 2018
View reviewed changes
common/consumer_state_table_pops.lua
for f, v in pairs(fieldvalues) do
redis.call('HSET', tablename..key, f, v)
end
redis.call('DEL', stateprefix..tablename..key)
Copy link
Copy Markdown
Contributor

@jipanyang jipanyang May 30, 2018 •
edited by qiluo-msft
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the extra HSET and DEL processing, it looks the number of redis operations has been doubled. Is the performance impact a concern here?

Is it possible to compress all the data in sadd argument and process them after SPOP? #Resolved

Copy link
Copy Markdown
Contributor Author

@qiluo-msft qiluo-msft May 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the requirement changes, performance is not the first concern here. I am open to any optimization suggestion.

Regarding compress, the SADD is used to keep keys, so we have chance to merge the operations on the same key. Compressing key/filed/value into SADD doesn't help.

In reply to: 191872386 [](ancestors = 191872386)

@jipanyang
Copy link
Copy Markdown
Contributor

jipanyang commented Jul 7, 2018 •
edited by qiluo-msft
Loading

  • Please ignore this message. I found it was caused by outdated swss-common library on host test environment. The library inside vs docker is up to date but pytest uses library on host *

It looks vs test_FDBAddedAfterMemberCreated will fail with this change?

sudo pytest -v --dvsname=vs --notempview -k 'test_FDBAddedAfterMemberCreated'
============================================================================== test session starts ==============================================================================
platform linux2 -- Python 2.7.15rc1, pytest-3.3.0, py-1.5.3, pluggy-0.6.0 -- /usr/bin/python
cachedir: .cache
rootdir: /home/jipan/warm_reboot/sonic-buildimage/src/sonic-swss/tests, inifile:
collected 47 items                                                                                                                                                              

test_fdb.py::test_FDBAddedAfterMemberCreated FAILED                                                                                                                       [100%]

=================================================================================== FAILURES ====================================================================================
________________________________________________________________________ test_FDBAddedAfterMemberCreated ________________________________________________________________________

dvs = <conftest.DockerVirtualSwitch object at 0x7f30b822bb50>

    def test_FDBAddedAfterMemberCreated(dvs):
        appl_db = swsscommon.DBConnector(swsscommon.APPL_DB, dvs.redis_sock, 0)
        asic_db = swsscommon.DBConnector(swsscommon.ASIC_DB, dvs.redis_sock, 0)
        conf_db = swsscommon.DBConnector(swsscommon.CONFIG_DB, dvs.redis_sock, 0)
    
        # create a FDB entry in Application DB
        create_entry_pst(
            appl_db,
            "FDB_TABLE", "Vlan2:52-54-00-25-06-E9",
            [
                ("port", "Ethernet0"),
                ("type", "dynamic"),
            ]
        )
    
        # check that the FDB entry wasn't inserted into ASIC DB
        assert how_many_entries_exist(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY") == 0, "The fdb entry leaked to ASIC"
    
        # create vlan
        create_entry_tbl(
            conf_db,
            "VLAN", "Vlan2",
            [
                ("vlanid", "2"),
            ]
        )
    
        # create vlan member entry in application db
        create_entry_tbl(
            conf_db,
            "VLAN_MEMBER", "Vlan2|Ethernet0",
             [
                ("tagging_mode", "untagged"),
             ]
        )
    
        # check that the vlan information was propagated
        assert how_many_entries_exist(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_VLAN") == 2, "The 2 vlan wasn't created"
        assert how_many_entries_exist(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_BRIDGE_PORT") == 1, "The bridge port wasn't created"
        assert how_many_entries_exist(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_VLAN_MEMBER") == 1, "The vlan member wasn't added"
    
        # Get mapping between interface name and its bridge port_id
        iface_2_bridge_port_id = get_map_iface_bridge_port_id(asic_db, dvs)
    
        # check that the FDB entry was inserted into ASIC DB
>       assert how_many_entries_exist(asic_db, "ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY") == 1, "The fdb entry wasn't inserted to ASIC"
E       AssertionError: The fdb entry wasn't inserted to ASIC
E       assert 0 == 1
E        +  where 0 = how_many_entries_exist(<swsscommon.swsscommon.DBConnector; proxy of <Swig Object of type 'swss::DBConnector *' at 0x7f30b8295ea0> >, 'ASIC_STATE:SAI_OBJECT_TYPE_FDB_ENTRY')

test_fdb.py:123: AssertionError
------------------------------------------------------------------------------ Captured log setup -------------------------------------------------------------------------------
auth.py                    234 DEBUG    Trying paths: ['/home/jipan/.docker/config.json', '/home/jipan/.dockercfg']
auth.py                    241 DEBUG    No config file found
connectionpool.py          396 DEBUG    http://localhost:None "GET /v1.30/containers/json?all=0&limit=-1&trunc_cmd=0&size=0 HTTP/1.1" 200 1501
connectionpool.py          396 DEBUG    http://localhost:None "GET /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/json HTTP/1.1" 200 None
connectionpool.py          396 DEBUG    http://localhost:None "GET /v1.30/containers/9652a812b62dc6da224da2eb10afb9c133da50c1cc84d88cc7a67c4492987521/json HTTP/1.1" 200 None
connectionpool.py          396 DEBUG    http://localhost:None "GET /v1.30/containers/json?all=0&limit=-1&trunc_cmd=0&size=0 HTTP/1.1" 200 1501
connectionpool.py          396 DEBUG    http://localhost:None "GET /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/json HTTP/1.1" 200 None
connectionpool.py          396 DEBUG    http://localhost:None "GET /v1.30/containers/9652a812b62dc6da224da2eb10afb9c133da50c1cc84d88cc7a67c4492987521/json HTTP/1.1" 200 None
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/restart?t=10 HTTP/1.1" 204 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/e6b68214102ba58e961059ff73b8e97025ad20468b932ec2d758021dc5e4be36/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/60e69f5817d3254d18ffbc6dbfbd52c22eec478919092a17f99a16a2cc0253ef/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/139a4db9cdaf2f28148fc17b6751a8d567c8dbe27e124ecdfe4eb5aa5b136869/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/fb58dd2e6afd48d036454e64db4d0e57e34e7d07614eabdc8de24db744a6e9a8/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/4f09fb8e00b3bf05d76757baf8fc4728f2bdf3a84c5c68a57449d9ae910265c4/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/56aa247091281d554d3475b2b7f4175f686a63d7046ec430dbe3299fbbb7d83f/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/ef8e578e56e4c90882fb928c1defdb22c9057043f1a4b30c269987f0560ca10d/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/19ee2162af3d5bdd1586617fe391b5d7f3babf1d6b5235276c2bdfda782aceb0/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/98f9d62ea809f081d290c92b240e6bf3f79c5317045630cb714fdd57f3e7ea44/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/5a6b72f5e3b8dbe6f1122fb09f62c284e0399edf3d16ab2ba7cb2099894a634c/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/20e0320ae4551100fc47865049eb5e8922cc98ffa813cbb26b9ff22a0b0676a8/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/4e8d3bcdce068f7b60e66d49b7df04478573b0759416308374336dfb5422ac3c/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/e6c1dc710a8e55d3568a7fe8833955d0bb28544e99d45f01251b6c8baea0b920/start HTTP/1.1" 101 0
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/containers/85b80d419bbaa3ebefc2ce354db2da0724fe4e435c2200ef73b9c86664928b0e/exec HTTP/1.1" 201 74
connectionpool.py          396 DEBUG    http://localhost:None "POST /v1.30/exec/90f58175ff0c76033baca36e7d37782e562b9900c666dd8885bf2b3ca8d10e9c/start HTTP/1.1" 101 0
============================================================================== 46 tests deselected ==============================================================================
=================================================================== 1 failed, 46 deselected in 22.13 seconds ====================================================================

``` #Closed

jipanyang
jipanyang reviewed Jul 11, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jipanyang jipanyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though it passed swss common unit test, the data left in DB table is wrong: Sequence number is taken as field while both the original field/value are set as value.

root@80fe1d6fbd69:/sonic/src/sonic-sairedis/tests# redis-cli -n 0
127.0.0.1:6379> hget UT_REDIS_THREAD_0:key 818 1
(error) ERR wrong number of arguments for 'hget' command
127.0.0.1:6379> hget "UT_REDIS_THREAD_0:key 818" 1
"field 0"
127.0.0.1:6379> hget "UT_REDIS_THREAD_0:key 818" 2
""
127.0.0.1:6379> hget "UT_REDIS_THREAD_0:key 818" 3
"field 1"
127.0.0.1:6379> hget "UT_REDIS_THREAD_0:key 818" 4
"value 1"

127.0.0.1:6379> hgetall "UT_REDIS_THREAD_0:key 818"

  1. "1"
  2. "field 0"
  3. "2"
  4. ""
  5. "3"
  6. "field 1"
  7. "4"
  8. "value 1"
  9. "5"
  10. "field 2"
  11. "6"
  12. "value 2"
  13. "7"
  14. "field 3"
  15. "8"
  16. "value 3"
  17. "9"
  18. "field 4"
  19. "10"
  20. "value 4"
  21. "11"
  22. "field 5"
  23. "12"
  24. "value 5"
  25. "13"
  26. "field 6"
  27. "14"
  28. "value 6"
  29. "15"
  30. "field 7"
  31. "16"
  32. "value 7"
  33. "17"
  34. "field 8"
  35. "18"
  36. "value 8"
  37. "19"
  38. "field 9"
  39. "20"
  40. "value 9"
  41. "21"
  42. "field 10"
  43. "22"
  44. "value 10"
  45. "23"
  46. "field 11"
  47. "24"
  48. "value 11"
  49. "25"
  50. "field 12"
  51. "26"
  52. "value 12"
  53. "27"
  54. "field 13"
  55. "28"
  56. "value 13"
  57. "29"
  58. "field 14"
  59. "30"
  60. "value 14"
  61. "31"
  62. "field 15"
  63. "32"
  64. "value 15"
  65. "33"
  66. "field 16"
  67. "34"
  68. "value 16"
  69. "35"
  70. "field 17"
  71. "36"
  72. "value 17"
  73. "37"
  74. "field 18"
  75. "38"
  76. "value 18"
  77. "39"
  78. "field 19"
  79. "40"
  80. "value 19"
  81. "41"
  82. "field 20"
  83. "42"
  84. "value 20"
  85. "43"
  86. "field 21"
  87. "44"
  88. "value 21"
  89. "45"
  90. "field 22"
  91. "46"
  92. "value 22"
  93. "47"
  94. "field 23"
  95. "48"
  96. "value 23"
  97. "49"
  98. "field 24"
  99. "50"
  100. "value 24"
  101. "51"
  102. "field 25"
  103. "52"
  104. "value 25"
  105. "53"
  106. "field 26"
  107. "54"
  108. "value 26"
  109. "55"
  110. "field 27"
  111. "56"
  112. "value 27"
    127.0.0.1:6379>

@qiluo-msft qiluo-msft force-pushed the qiluo/statequeuefix branch from 552152d to b073815 Compare July 14, 2018 01:37
@qiluo-msft
Fix DEL, add tests
2d9759f 
Signed-off-by: Qi Luo <[email protected]>
@jipanyang
Copy link
Copy Markdown
Contributor

jipanyang commented Jul 15, 2018

Correct data written to DB table after the fieldvalue set fix. thanks!

@qiluo-msft qiluo-msft merged commit 3edb017 into sonic-net:master Jul 27, 2018
@qiluo-msft qiluo-msft deleted the qiluo/statequeuefix branch July 27, 2018 22:40
@qiluo-msft qiluo-msft mentioned this pull request Aug 8, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kcudnik kcudnik kcudnik approved these changes

@pavel-shirshov pavel-shirshov Awaiting requested review from pavel-shirshov

@lguohan lguohan Awaiting requested review from lguohan

@stcheng stcheng Awaiting requested review from stcheng

+1 more reviewer

@jipanyang jipanyang jipanyang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@qiluo-msft @kcudnik @jipanyang