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

Clear fabric counters queue/port #26

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

jfeng-arista
Copy link
Contributor

What I did

Raise a PR ( the same as sonic-net/sonic-utilities#2892 ) in the https://github.com/Azure/sonic-utilities.msft/tree/202205

How I did it

How to verify it

tested under https://github.com/Azure/sonic-buildimage-msft/tree/202205

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

@jfeng-arista
Copy link
Contributor Author

@gechiang This the clear fabric counters pr for 202205.

@gechiang
Copy link
Contributor

gechiang commented Sep 1, 2023

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

@jfeng-arista
Copy link
Contributor Author

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,

command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892
here it still use the old way
command = "fabricstat -C -q"

I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.

Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

@gechiang
Copy link
Contributor

gechiang commented Sep 1, 2023

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,

command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892 here it still use the old way command = "fabricstat -C -q"

I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.

Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

Thanks. I will have to hold off on picking up this for now until I get the build team to add the necessary PR tests into the pipeline for the MSFT repo so that we can better identify if a PR is really safe without build issues... last time when I picked it up we did not realize there was issue with it until community member and myself started to build with it... I will try to experiment picking up this change in my local branch and attempt a build there to see if the problem is somehow fixed...
Will update you... Thanks!

@jfeng-arista
Copy link
Contributor Author

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,
command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892 here it still use the old way command = "fabricstat -C -q"
I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.
Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

Thanks. I will have to hold off on picking up this for now until I get the build team to add the necessary PR tests into the pipeline for the MSFT repo so that we can better identify if a PR is really safe without build issues... last time when I picked it up we did not realize there was issue with it until community member and myself started to build with it... I will try to experiment picking up this change in my local branch and attempt a build there to see if the problem is somehow fixed... Will update you... Thanks!

thank you

@jfeng-arista
Copy link
Contributor Author

@jfeng-arista Thanks! BUt this is rather interesting. From this PR it looks like the changes are exactly the same as that we previously cherry-picked to here... Did I missed anything? If there is no change, then how were you able to ensure it is fixed and not causing the build issue that we saw previously?

The major change is the cli. I guess the cli parse infra changes are not here yet,
command = ["fabricstat", "-C", "-q"] is the one in sonic-net/sonic-utilities#2892 here it still use the old way command = "fabricstat -C -q"
I tried to copy all the files in https://github.com/Azure/sonic-buildimage-msft/tree/202205 locally and tried to run the sonic-utilities tests and saw the test failures, and I revised code based on what I see and they passed in my local testing, so I raise the PR based on that test in this repo.
Though I don't know exactly what the errors you saw before ( there is no error log / information for the previous failure you mentioned ). All my changes are based on my local testing.

Thanks. I will have to hold off on picking up this for now until I get the build team to add the necessary PR tests into the pipeline for the MSFT repo so that we can better identify if a PR is really safe without build issues... last time when I picked it up we did not realize there was issue with it until community member and myself started to build with it... I will try to experiment picking up this change in my local branch and attempt a build there to see if the problem is somehow fixed... Will update you... Thanks!

thank you

I was helped with others here during the weekend to try to pull the full source locally, and the build of broadcom ( which is the one close to upstream ) passed. This makes me more confident about this change within the CURRRENT code base. thank you

@gechiang gechiang merged commit 8cf9817 into Azure:202205 Sep 26, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants