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

Fix DBConfig not initialize issue in pfcwd #2238

Merged
merged 14 commits into from
Jul 4, 2022

Conversation

liuh-80
Copy link
Contributor

@liuh-80 liuh-80 commented Jun 28, 2022

What I did

Fix pfcwd connect DB with exception issue: sonic-net/sonic-buildimage#11269

pfcwd implicit depends on InterfaceAliasConverter() to initialize DB config, however following PR change InterfaceAliasConverter() behavior to lazy initialize, then pfcwd failed when try connect to DB without initialize DB config:

#2183

How I did it

Load DB config in pfcwd.

How to verify it

Pass all UT.

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)

@liuh-80 liuh-80 marked this pull request as ready for review June 28, 2022 07:09
pfcwd/main.py Outdated Show resolved Hide resolved
pfcwd/main.py Outdated
def initialize_db_config():
# Initialize db config here for UT coverage
load_db_config()
return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our code coverage requirement is 80%, so need return here to pass coverage check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer keep code cleaner, and fix the test infra.

Copy link
Contributor

@qiluo-msft qiluo-msft Jun 30, 2022

Choose a reason for hiding this comment

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

I am okay to override coverage check force merge and suppress coverage checker in of this PR as long as there is an issue tracking the coverage issue.

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 improve code and try fix test infra later.

Copy link
Contributor Author

@liuh-80 liuh-80 Jul 1, 2022

Choose a reason for hiding this comment

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

I checked the code coverage of some other script, the @click.group() of those script are all not covered, for example piceutil, so will try cover the cli() and if not work will change coverage rate to 50%

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems a known issue in click: pallets/click#267
Will try use subprocess.check_output to cover code in cli().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I try different solution but all failed, so change code coverage threshold to 50%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Revert the code coverage threshold change for force merge.

@rlhui
Copy link
Contributor

rlhui commented Jul 12, 2022

@qiluo-msft , @SuvarnaMeenakshi - does this one need to be in 202205 branch?

@SuvarnaMeenakshi
Copy link
Contributor

@qiluo-msft , @SuvarnaMeenakshi - does this one need to be in 202205 branch?

Not required in 202205 because the change that caused this issue #2183 is not there in 202205 branch

@ysmanman
Copy link

@qiluo-msft , @SuvarnaMeenakshi - does this one need to be in 202205 branch?

Not required in 202205 because the change that caused this issue #2183 is not there in 202205 branch

Just clarify, we ran into this issue with 202205 image. We filed sonic-net/sonic-buildimage#12062 to track.

@liuh-80
Copy link
Contributor Author

liuh-80 commented Sep 14, 2022

This PR need cherry pick to 202205 branch to fix sonic-net/sonic-buildimage#12062

@liuh-80 liuh-80 deleted the dev/liuh/fix-pfcwd branch September 14, 2022 07:35
@gechiang
Copy link
Contributor

@gechiang check if this can cherry picked into 20205 branch

gechiang pushed a commit to gechiang/sonic-utilities that referenced this pull request Sep 14, 2022
#### What I did
Fix pfcwd connect DB with exception issue: sonic-net/sonic-buildimage#11269

pfcwd implicit depends on InterfaceAliasConverter() to initialize DB config, however following PR change InterfaceAliasConverter() behavior to lazy initialize, then pfcwd failed when try connect to DB without initialize DB config:

sonic-net#2183

#### How I did it
Load  DB config in pfcwd.

#### How to verify it
Pass all UT.
gechiang added a commit that referenced this pull request Sep 15, 2022
#### What I did
Fix pfcwd connect DB with exception issue: sonic-net/sonic-buildimage#11269

pfcwd implicit depends on InterfaceAliasConverter() to initialize DB config, however following PR change InterfaceAliasConverter() behavior to lazy initialize, then pfcwd failed when try connect to DB without initialize DB config:

#2183

#### How I did it
Load  DB config in pfcwd.

#### How to verify it
Pass all UT.

Co-authored-by: Hua Liu <[email protected]>
@gechiang
Copy link
Contributor

gechiang commented Sep 15, 2022

I removed the label request for 20205 branch because I manually created a PR to pick up this same change in 202205 here:
#2372

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.

6 participants