-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Delete session after key expiration #11487
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
Conversation
DaanHoogland
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #11487 +/- ##
============================================
- Coverage 16.17% 16.17% -0.01%
- Complexity 13285 13287 +2
============================================
Files 5656 5656
Lines 498000 498043 +43
Branches 60401 60409 +8
============================================
+ Hits 80539 80543 +4
- Misses 408496 408538 +42
+ Partials 8965 8962 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@harikrishna-patnala, @sureshanaparti could you please review the PR? @DaanHoogland is there anything we can do to help speed up the PR review and merge process? |
shwstppr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code lgtm. Specific to Primera plugin so not everyone can test
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
@shrikantjoshi-hpe it seems the PR is fixing an issue. Should this be targeted for the 4.20 branch? |
|
Yes, we can target it for the 4.20 branch |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 14863 |
|
@shrikantjoshi-hpe you will have to rebase your branch to 4.20 for targeting it for 4.20/4.20.2 |
|
@shwstppr Does it mean we need to raise the new PR post-rebase targeting the 4.20 branch? |
|
@shrikantjoshi-hpe either way would work. |
|
Rebase the branch to 4.20, change the base branch to 4.20 near the top of the PR |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@shrikantjoshi-hpe your branch is not rebased correctly. If 6c8fe7a is the only change then you can do something like, |
6c8fe7a to
3dbb587
Compare
|
@shwstppr, Done as you asked. Thanks |
|
Thanks @shrikantjoshi-hpe @blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 14890 |
|
@shrikantjoshi-hpe |
|
This implementation is scoped specifically to the primera storage plugin. Extensive testing has been completed with no regression issues identified. We can merge it safely. |
@shrikantjoshi-hpe great, |
Description
This PR addresses a session leak issue in the Primera storage adapter by updating the disconnect method to ensure sessions are properly terminated. Additionally, a new logout method has been introduced to explicitly handle session cleanup. These changes improve resource management and prevent lingering sessions that could impact system stability.
The disconnect method now calls the new logout method to ensure sessions are closed.
The logout method encapsulates the logic for terminating sessions with the storage system.
This is a bug fix aimed at improving reliability and resource usage.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
N/A
How Has This Been Tested?
How did you try to break this feature and the system with this change?