{"action":"created","issue":{"url":"https://api.github.com/repos/Azure/sonic-utilities/issues/265","repository_url":"https://api.github.com/repos/Azure/sonic-utilities","labels_url":"https://api.github.com/repos/Azure/sonic-utilities/issues/265/labels{/name}","comments_url":"https://api.github.com/repos/Azure/sonic-utilities/issues/265/comments","events_url":"https://api.github.com/repos/Azure/sonic-utilities/issues/265/events","html_url":"https://github.com/Azure/sonic-utilities/pull/265","id":330069021,"node_id":"MDExOlB1bGxSZXF1ZXN0MTkzMTY5MDEy","number":265,"title":"[scripts/portstat]: Cached counters file per system instead of per user","user":{"login":"praveen-li","id":38769597,"node_id":"MDQ6VXNlcjM4NzY5NTk3","avatar_url":"https://avatars2.githubusercontent.com/u/38769597?v=4","gravatar_id":"","url":"https://api.github.com/users/praveen-li","html_url":"https://github.com/praveen-li","followers_url":"https://api.github.com/users/praveen-li/followers","following_url":"https://api.github.com/users/praveen-li/following{/other_user}","gists_url":"https://api.github.com/users/praveen-li/gists{/gist_id}","starred_url":"https://api.github.com/users/praveen-li/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/praveen-li/subscriptions","organizations_url":"https://api.github.com/users/praveen-li/orgs","repos_url":"https://api.github.com/users/praveen-li/repos","events_url":"https://api.github.com/users/praveen-li/events{/privacy}","received_events_url":"https://api.github.com/users/praveen-li/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2018-06-06T23:39:33Z","updated_at":"2018-09-12T17:51:22Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/Azure/sonic-utilities/pulls/265","html_url":"https://github.com/Azure/sonic-utilities/pull/265","diff_url":"https://github.com/Azure/sonic-utilities/pull/265.diff","patch_url":"https://github.com/Azure/sonic-utilities/pull/265.patch"},"body":"[scripts/portstat]: Cached file is not per system, it is stored per user as of today, which does not allow users to have a global view of counters\r\n\r\nSigned-off-by: pchaudhary@linkedin.com\r\n\r\n<!--\r\nPlease make sure you've read and understood our contributing guidelines:\r\nhttps://github.com/Azure/SONiC/blob/gh-pages/CONTRIBUTING.md\r\n\r\n** Make sure all your commits include a signature generated with `git commit -s` **\r\n\r\nIf this is a bug fix, make sure your description includes \"closes #xxxx\",\r\n\"fixes #xxxx\" or \"resolves #xxxx\" so that GitHub automatically closes the related\r\nissue when the PR is merged\r\n\r\nPlease provide the following information:\r\n-->\r\n\r\n**- What I did**\r\n1.) Made cached counter file per system instead of per user.  Tag files can be used to have different view of stats, if needed.\r\n2.) Allow only root to run \"clear counters\", because it is changing system stats view.\r\n3.) Add synchronization for file writing. Today cached file may get corrupted if 2 portstat processes are writing in file simultaneously. \r\n\r\n**- How I did it**\r\n1.) Create hidden file with system file format name for cached counters. This name format will indicate user, to not change the file, via manual editing.\r\n2.) Allow only root to run \"clear counters\" or \"portstat -c\"\r\n3.) Use flock for synchronization to avoid write by multiple process. Other options considered were a.) linux pidfile, linux lockfile b.) Posix IPCs.\r\n    --flock is less costly system call and also provide stable synchronization.\r\n    Note: ideally we can take shared flock for file read operation as well, so that portstat does not read the file when it is being written. As of now this change is not done, because it is easily recoverable.\r\nFile Modified:\r\n   [scripts/portstat]\r\n\r\n**- How to verify it**\r\n1.) only root should be able to clear counters. \r\n\r\nadmin@lnos-x1-a-asw01:~$ clear counters\r\nPermission Denied\r\n\r\nadmin@lnos-x1-a-asw01:~$ sudo clear counters\r\nCleared counters\r\n\r\n2.) Observe file and time stamp\r\n\r\nadmin@lnos-x1-a-asw01:~$ ls -la /tmp/portstat/\r\ntotal 20\r\ndrwxrwxrwx 2 admin admin  4096 May 16 15:37 .\r\ndrwxrwxrwt 8 root  root   4096 May 16 17:22 ..\r\n-rw-r--r-- 1 root  root  11528 May 16 17:22 .CaChEd_StAtS\r\n\r\nadmin@lnos-x1-a-asw01:~$ date\r\nWed May 16 17:23:01 UTC 2018\r\n\r\n3.) Run command as root user and observe new timestamp\r\n\r\nroot@lnos-x1-a-asw01:/home/admin# clear counters\r\nCleared counters\r\n\r\nroot@lnos-x1-a-asw01:/home/admin# ls -la /tmp/portstat/\r\ntotal 20\r\ndrwxrwxrwx 2 admin admin  4096 May 16 15:37 .\r\ndrwxrwxrwt 8 root  root   4096 May 16 17:23 ..\r\n-rw-r--r-- 1 root  root  11535 May 16 17:23 .CaChEd_StAtS\r\nroot@lnos-x1-a-asw01:/home/admin# date\r\nWed May 16 17:23:36 UTC 2018\r\n\r\n\r\n4. ) Dir permission should be root + exe by all\r\nadmin@lnos-x1-a-asw01:~$ ls -l /tmp/\r\ntotal 64\r\nprw-r--r-- 1 root  root      0 May 16 17:34 479.tmp\r\n-rw-r--r-- 1 root  root      8 May 16 15:14 dhcp_graph_url\r\ndrwxr-xr-x 2 admin admin  4096 May 16 17:33 portstat<<<<<<<<<<<<<<<<<<<<<<<<<<\r\n-rw-r--r-- 1 admin admin 54488 May 16 15:33 python-sonic-utilities_1.2-1_all.deb\r\n\r\n5.) File permission should be rw by root and read by all.\r\n\r\nroot@lnos-x1-a-asw01:/home/admin# ls -la /tmp/portstat/\r\ntotal 20\r\ndrwxrwxrwx 2 admin admin  4096 May 16 15:37 .\r\ndrwxrwxrwt 8 root  root   4096 May 16 17:24 ..\r\n-rw-r--r-- 1 root  root  11535 May 16 17:23 .CaChEd_StAtS\r\n\r\n6.) Try portstat\r\nadmin@lnos-x1-a-asw01:~$ sudo portstat -c\r\nCleared counters\r\n\r\n7.)counters with cached file\r\nadmin@lnos-x1-a-asw01:~$ portstat \r\n\r\nEthernet114        D        0  0.00 B/s      0.00%         0         0         0        0  0.00 B/s      0.00%         0         0         0\r\nEthernet116        U        1  9.13 B/s      0.00%         0         0         0        1  9.24 B/s      0.00%         0         0         0\r\nEthernet118        D        0  0.00 B/s      0.00%         0         0         0        0  0.00 B/s      0.00%         0         0         0\r\nEthernet120        D        0  0.00 B/s      0.00%         0         0         0        0  0.00 B/s      0.00%         0         0         0\r\nEthernet122        D        0  0.00 B/s      0.00%         0         0         0        0  0.00 B/s      0.00%         0         0         0\r\nEthernet124        U        1  9.13 B/s      0.00%         0         0         0        1  9.24 B/s      0.00%         0         0         0\r\nEthernet126        U        1  9.13 B/s      0.00%         0         0         0        1  9.24 B/s      0.00%         0         0         0\r\n\r\n9.) Check synchronisation, run \"portstat -c\" in loop, user root\r\nadmin@lnos-x1-a-asw01:~$ cat portstat_loop.sh \r\nfor i in {1..5000} \r\ndo\r\n    portstat -c\r\n    echo $i\r\ndone\r\n\r\nNoticed file lock:<<<<<\r\nCleared counters\r\n1400\r\nFile is locked, Try again\r\n1401\r\nFile is locked, Try again\r\n1402\r\nFile is locked, Try again\r\n1403\r\nCleared counters\r\n1404\r\n**- Previous command output (if the output of a command-line utility has changed)**\r\nAs per field 3\r\n**- New command output (if the output of a command-line utility has changed)**\r\nAs per field 3\r\n-->\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/Azure/sonic-utilities/issues/comments/420739360","html_url":"https://github.com/Azure/sonic-utilities/pull/265#issuecomment-420739360","issue_url":"https://api.github.com/repos/Azure/sonic-utilities/issues/265","id":420739360,"node_id":"MDEyOklzc3VlQ29tbWVudDQyMDczOTM2MA==","user":{"login":"praveen-li","id":38769597,"node_id":"MDQ6VXNlcjM4NzY5NTk3","avatar_url":"https://avatars2.githubusercontent.com/u/38769597?v=4","gravatar_id":"","url":"https://api.github.com/users/praveen-li","html_url":"https://github.com/praveen-li","followers_url":"https://api.github.com/users/praveen-li/followers","following_url":"https://api.github.com/users/praveen-li/following{/other_user}","gists_url":"https://api.github.com/users/praveen-li/gists{/gist_id}","starred_url":"https://api.github.com/users/praveen-li/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/praveen-li/subscriptions","organizations_url":"https://api.github.com/users/praveen-li/orgs","repos_url":"https://api.github.com/users/praveen-li/repos","events_url":"https://api.github.com/users/praveen-li/events{/privacy}","received_events_url":"https://api.github.com/users/praveen-li/received_events","type":"User","site_admin":false},"created_at":"2018-09-12T17:51:22Z","updated_at":"2018-09-12T17:51:22Z","author_association":"CONTRIBUTOR","body":"> \"it is stored per user as of today, which does not allow users to have a global view of counters\". I do not understand this. the counter is stored per-user so that each user can have it own view. linux is a multi-user environment.\r\n\r\n[PC]: On router/switches from vendors, we have one view of cleared counters, i.e. if one user clears counters, all users see the impact. I think we must follow the same. For different view we are proving tags anyways. Kindly let me know if there is a strong reason for not following same approach as other vendors. \r\n\r\nAlso today, file is not locked properly while writing cleared stats, so if entire dictionary is not written with an atomic operation by OS, then due to process context switch, we may break stats view. This is very easy to repro today, I feel we must fix this part for sure."}}
{"actor":{"display_login":"praveen-li"}}