Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unwanted newline character at end of timestamp #3111

Merged

Conversation

arista-nwolfe
Copy link
Contributor

@arista-nwolfe arista-nwolfe commented Apr 10, 2024

In #3052 2 new fields were added the PORT_TABLE in APPL_DB: last_down_time and last_up_time.
But their value was generated with std::ctime which includes a newline character at the end of the character array which causes issues for parsing the output.

Output today:

> sonic-db-cli APPL_DB hgetall PORT_TABLE:Ethernet8
{'admin_status': 'up', 'alias': 'Ethernet3/1', 'asic_port_name': 'Eth8', 'coreId': '0', 'corePortId': '3', 'description': 'Ethernet8-connected-to-@Ethernet5/4/1', 'fec': 'rs', 'index': '3', 'lanes': '4,5', 'mtu': '9100', 'numVoq': '8', 'pfc_asym': 'off', 'role': 'Ext', 'speed': '100000', 'tpid': '0x8100', 'oper_status': 'up', 'flap_count': '1', 'last_up_time': 'Wed Apr 10 00:18:44 2024
', 'system_oper_status': 'up', 'line_oper_status': 'up'}

> config interface shutdown Ethernet8
> sonic-db-cli APPL_DB hgetall PORT_TABLE:Ethernet8
{'admin_status': 'down', 'alias': 'Ethernet3/1', 'asic_port_name': 'Eth8', 'coreId': '0', 'corePortId': '3', 'description': 'Ethernet8-connected-to-@Ethernet5/4/1', 'fec': 'rs', 'index': '3', 'lanes': '4,5', 'mtu': '9100', 'numVoq': '8', 'pfc_asym': 'off', 'role': 'Ext', 'speed': '100000', 'tpid': '0x8100', 'oper_status': 'down', 'flap_count': '2', 'last_up_time': 'Wed Apr 10 00:18:44 2024
', 'system_oper_status': 'down', 'line_oper_status': 'down', 'last_down_time': 'Wed Apr 10 16:35:11 2024
'}

>  config interface startup Ethernet8
> sonic-db-cli APPL_DB hgetall PORT_TABLE:Ethernet8
{'admin_status': 'up', 'alias': 'Ethernet3/1', 'asic_port_name': 'Eth8', 'coreId': '0', 'corePortId': '3', 'description': 'Ethernet8-connected-to-@Ethernet5/4/1', 'fec': 'rs', 'index': '3', 'lanes': '4,5', 'mtu': '9100', 'numVoq': '8', 'pfc_asym': 'off', 'role': 'Ext', 'speed': '100000', 'tpid': '0x8100', 'oper_status': 'up', 'flap_count': '3', 'last_up_time': 'Wed Apr 10 16:35:34 2024
', 'system_oper_status': 'up', 'line_oper_status': 'up', 'last_down_time': 'Wed Apr 10 16:35:11 2024
'}

Output after this change:

> sonic-db-cli APPL_DB hgetall PORT_TABLE:Ethernet8
{'admin_status': 'up', 'alias': 'Ethernet3/1', 'asic_port_name': 'Eth8', 'coreId': '0', 'corePortId': '3', 'description': 'Ethernet8-connected-to-@Ethernet5/4/1', 'fec': 'rs', 'index': '3', 'lanes': '4,5', 'mtu': '9100', 'numVoq': '8', 'pfc_asym': 'off', 'role': 'Ext', 'speed': '100000', 'tpid': '0x8100', 'oper_status': 'up', 'flap_count': '1', 'last_up_time': 'Wed Apr 10 17:01:00 2024', 'system_oper_status': 'up', 'line_oper_status': 'up'}

> config interface shutdown Ethernet8
> sonic-db-cli APPL_DB hgetall PORT_TABLE:Ethernet8
{'admin_status': 'down', 'alias': 'Ethernet3/1', 'asic_port_name': 'Eth8', 'coreId': '0', 'corePortId': '3', 'description': 'Ethernet8-connected-to-@Ethernet5/4/1', 'fec': 'rs', 'index': '3', 'lanes': '4,5', 'mtu': '9100', 'numVoq': '8', 'pfc_asym': 'off', 'role': 'Ext', 'speed': '100000', 'tpid': '0x8100', 'oper_status': 'down', 'flap_count': '2', 'last_up_time': 'Wed Apr 10 17:01:00 2024', 'system_oper_status': 'down', 'line_oper_status': 'down', 'last_down_time': 'Wed Apr 10 18:37:03 2024'}

> config interface startup Ethernet8
> sonic-db-cli APPL_DB hgetall PORT_TABLE:Ethernet8
{'admin_status': 'up', 'alias': 'Ethernet3/1', 'asic_port_name': 'Eth8', 'coreId': '0', 'corePortId': '3', 'description': 'Ethernet8-connected-to-@Ethernet5/4/1', 'fec': 'rs', 'index': '3', 'lanes': '4,5', 'mtu': '9100', 'numVoq': '8', 'pfc_asym': 'off', 'role': 'Ext', 'speed': '100000', 'tpid': '0x8100', 'oper_status': 'up', 'flap_count': '3', 'last_up_time': 'Wed Apr 10 18:37:13 2024', 'system_oper_status': 'up', 'line_oper_status': 'up', 'last_down_time': 'Wed Apr 10 18:37:03 2024'}

@arista-nwolfe arista-nwolfe requested a review from prsunny as a code owner April 10, 2024 19:01
@prsunny prsunny requested a review from prgeor April 10, 2024 19:57
@prsunny
Copy link
Collaborator

prsunny commented Apr 10, 2024

Can this be caught by VS test?

@arista-nwolfe
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@arista-nwolfe arista-nwolfe force-pushed the master-fix-newline-on-timestamp branch from 160c9a4 to 9b68530 Compare April 11, 2024 16:31
arlakshm
arlakshm previously approved these changes Apr 12, 2024
@arista-nwolfe arista-nwolfe force-pushed the master-fix-newline-on-timestamp branch from 0608aba to 740340b Compare April 12, 2024 20:35
FieldValueTuple tuple("last_down_time", std::ctime(&now_c));
char buffer[32];
// Format: Www Mmm dd hh:mm:ss yyyy
std::strftime(buffer, sizeof(buffer), "%a %b %d %H:%M:%S %Y", std::localtime(&now_c));
Copy link
Contributor

Choose a reason for hiding this comment

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

@arista-nwolfe can we keep the time in UTC since the syslog event time is in UTC?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, note that previously it was already in local time with ctime

@arista-nwolfe
Copy link
Contributor Author

arista-nwolfe commented Apr 22, 2024

Can this be caught by VS test?

This PR is the fix for a regression in a feature introduced by someone else in #3052
I've opened #3120 to track adding a test for it.

@arista-nwolfe
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@arista-nwolfe
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@arista-nwolfe arista-nwolfe force-pushed the master-fix-newline-on-timestamp branch from ccb19ba to f2d6606 Compare May 2, 2024 21:28
@arlakshm
Copy link
Contributor

arlakshm commented May 3, 2024

@prsunny, can you help merge this?

@prsunny
Copy link
Collaborator

prsunny commented May 3, 2024

@prgeor , @arlakshm , could you folks signoff?

@prsunny prsunny merged commit 837e467 into sonic-net:master May 7, 2024
17 checks passed
@justin-wong-ce
Copy link

@arista-nwolfe A backport to 202311 is needed to fix a regression caused by a backport of this PR: #3052

Can you add a Request for 202311 Branch label please?

@arista-nwolfe
Copy link
Contributor Author

@arista-nwolfe A backport to 202311 is needed to fix a regression caused by a backport of this PR: #3052

Can you add a Request for 202311 Branch label please?

I don't have label permissions @wenyiz2021 @arlakshm @vmittal-msft would one of you be able to add it.

@StormLiangMS
Copy link
Contributor

hi @arista-nwolfe could you run test with 202311 for the cherrypick?

@StormLiangMS
Copy link
Contributor

@yxieca could you help to cherry pick?

@arista-nwolfe
Copy link
Contributor Author

hi @arista-nwolfe could you run test with 202311 for the cherrypick?

Tested this on 202311

mssonicbld pushed a commit to mssonicbld/sonic-swss that referenced this pull request Aug 8, 2024
* Remove unwanted newline character at end of timestamp
* Cleaner approach to generate timestamp string with no newline character
* Use UTC rather than local time in timestamp
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202311: #3256

mssonicbld pushed a commit that referenced this pull request Aug 8, 2024
* Remove unwanted newline character at end of timestamp
* Cleaner approach to generate timestamp string with no newline character
* Use UTC rather than local time in timestamp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants