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.

Sign up for GitHub

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

Jump to bottom

tests: catch exception during switch shutdown #15903

Merged
merged 1 commit into from
Oct 8, 2024
Merged

tests: catch exception during switch shutdown #15903

merged 1 commit into from
Oct 8, 2024

Conversation

y-bharath14
Copy link
Contributor

@y-bharath14 y-bharath14 commented May 2, 2024 •
edited
Loading

Handling exception at topogen library
OSError exception is raised when mininet tries to stop switch though switch is stopped once but mininet tries to stop same switch again, where it ended up with exception

@frrbot frrbot bot added the tests Topotests, make check, etc label May 2, 2024
@ton31337
Copy link
Member

ton31337 commented May 2, 2024

What does it solve here, and why only ospfX?

@y-bharath14
Copy link
Contributor Author

What does it solve here, and why only ospfX?

Hi Donatas Abraitis,

Thanks for the review !!

During the session cleanup, I mean at stop_topology(), there are cases where mininet tries to stop the switch eventhough switch is stopped at once.
When mininet tries to stop the same switch again and again, it ended with the exception. So, we are handling this as part of OSError.

Initially , I have started with ospfX module. Going forward, I will do the same change across all the modules.
For ospfX module alone, I could see change is needed in 46 files. If I do change in all modules, file count will be more.

So, I felt doing module by module changes would be a wise option.

Thanks & Regards,
Bharath

Mahesh-Pature
Mahesh-Pature approved these changes May 2, 2024
View reviewed changes
Copy link

@Mahesh-Pature Mahesh-Pature left a comment

Choose a reason for hiding this comment

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

Bharath thanks for the efforts.
Looks good for Me.
Keep continue for other modules as well.

@ton31337
Copy link
Member

ton31337 commented May 3, 2024

But... maybe this should be handled globally inside the libs (stop_topology()), and not per-test?

@y-bharath14 y-bharath14 force-pushed the srib-topotests-v1 branch from ed126f7 to 7d0be2a Compare May 3, 2024 11:45
@github-actions github-actions bot added size/XS and removed size/XL labels May 3, 2024
@y-bharath14
Copy link
Contributor Author

Hi Donatas Abraitis,

Hi Donatas Abraitis,

Addressed review comment.
Thank you.

Regards,
Bharath

@y-bharath14 y-bharath14 force-pushed the srib-topotests-v1 branch 3 times, most recently from c597145 to 060fd05 Compare May 6, 2024 10:44
@y-bharath14 y-bharath14 closed this May 7, 2024
@y-bharath14 y-bharath14 force-pushed the srib-topotests-v1 branch from 060fd05 to 5fe0c59 Compare May 7, 2024 14:30
@y-bharath14 y-bharath14 reopened this May 7, 2024
@github-actions github-actions bot added size/S and removed size/XS labels May 7, 2024
@y-bharath14 y-bharath14 requested a review from Mahesh-Pature May 7, 2024 15:11
@y-bharath14 y-bharath14 force-pushed the srib-topotests-v1 branch from 63de896 to 88a8bc5 Compare May 8, 2024 14:07
ton31337
ton31337 reviewed May 13, 2024
View reviewed changes
tests/topotests/lib/topogen.py
except OSError:
# OSError exception is raised when mininet tries to stop switch
# though switch is stopped once but mininet tries to stop same
# switch again, where it ended up with exception
Copy link
Member

Choose a reason for hiding this comment

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

Can we log something also in such a case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we log something also in such a case?

Hi Donatas Abraitis,

Addressed review comment.
Thank you.

Regards,
Bharath

ton31337
ton31337 previously requested changes May 15, 2024
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Squash two commits into a single one (they are a single change here).

@y-bharath14
Copy link
Contributor Author

Squash two commits into a single one (they are a single change here).

Hi Donatas Abraitis,

Squashed two commits into a single one.
Thank you.

Regards,
Bharath

@y-bharath14 y-bharath14 requested a review from ton31337 May 20, 2024 07:56
riw777
riw777 approved these changes May 20, 2024
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good

@riw777
Copy link
Member

riw777 commented May 28, 2024 •
edited
Loading

looks relevant

      Error: [DUT: FRR] OSPF GR Helper: activeRestarterCnt
   assert '[DUT: FRR] OSPF GR Helper: activeRestarterCnt' is True```

@riw777
Copy link
Member

riw777 commented Aug 14, 2024

@ton31337 ping ... :-)

@riw777
Copy link
Member

riw777 commented Sep 24, 2024

ci started 27 days ago ... seems hung?

ci:rerun

@y-bharath14
Copy link
Contributor Author

ci started 27 days ago ... seems hung?

ci:rerun

@riw777,
I did rebase of branch.
Now CI results seems to be fine.

Thanks,
Bharath

mjstapp
mjstapp reviewed Sep 25, 2024
View reviewed changes
Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

The PR title and description don't match the actual changes. Could you please update that? Something like "catch exception during switch shutdown", and note that it's a library change, not changes to any specific test suites.

tests/topotests/lib/topogen.py Outdated
logger.info(error)
logger.info(
"Exception is raised when mininet tries to stop switch"
"though switch is stopped once but mininet tries to stop same switch again."
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be simpler - how about: "Exception ignored: switch is already stopped"

Copy link
Contributor Author

@y-bharath14 y-bharath14 Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this could be simpler - how about: "Exception ignored: switch is already stopped"

Hi Mark Stapp,

Modified the title of the PR and log description.

Thanks & Regards,
Bharath

@y-bharath14 y-bharath14 changed the title tests: exception handling at ospf testcases tests: Exception ignored: switch is already stopped Sep 25, 2024
@y-bharath14 y-bharath14 changed the title tests: Exception ignored: switch is already stopped tests: catch exception during switch shutdown Sep 25, 2024
mjstapp
mjstapp approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me

@riw777 riw777 merged commit 382e4e9 into FRRouting:master Oct 8, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mahesh-Pature Mahesh-Pature Mahesh-Pature approved these changes

@mjstapp mjstapp mjstapp approved these changes

@riw777 riw777 riw777 approved these changes

@ton31337 ton31337 Awaiting requested review from ton31337

Assignees
No one assigned
Labels
master rebase PR needs rebase size/S tests Topotests, make check, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@y-bharath14 @ton31337 @riw777 @mjstapp @Mahesh-Pature