-
Notifications
You must be signed in to change notification settings - Fork 175
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
Etcd cleanup #5064
Etcd cleanup #5064
Conversation
Signed-off-by: manisha kumari <[email protected]>
Signed-off-by: manisha kumari <[email protected]>
Thanks for the fix @ManishaKumari295 but it seems there is a issue with the branch... |
Signed-off-by: manisha kumari <[email protected]>
Signed-off-by: manisha kumari <[email protected]>
Hi @elfranne , the branch seems to be fine now. Please verify the commited changes. |
Signed-off-by: manisha kumari <[email protected]>
Signed-off-by: manisha kumari <[email protected]>
Signed-off-by: manisha kumari <[email protected]>
CHANGELOG-6.md
Outdated
## [6.11.1] - 2024-09-12 | ||
|
||
### Changed | ||
- Added TTl to each entry in user-session within etcd |
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.
I would rephrase this to something like this and have just one entry:
### Fixed
- ADD TTL to each user session in the etcd data store to prevent leak.
backend/store/etcd/session.go
Outdated
@@ -30,12 +30,15 @@ func (s *Store) GetSession(ctx context.Context, username, sessionID string) (str | |||
} | |||
|
|||
// UpdateSession applies the supplied state to the session uniquely identified | |||
// by the given username and session ID. | |||
// by the given username and session ID and TTL of 6 minutes added considering access token expires in 5 minutes |
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.
Instead of hard-coding the value to 6 minutes I would calculate it based on the token expiration. I think something like (token expiration * 1.5) would be ok.
Great approach @ManishaKumari295. Just a couple of minor things and it should be good to go! |
I have limited knowledge in the inner workings of the Sensu, please take my findings with a grain of salt.
But it seems we are now creating grants based on username and sessionID and usuing new grants at every UpdateSession. Will this not result of a similar situation where we now have grants filling up etcd? Another approach would be to create a lease grant per user at the token creation and do a LeaseKeepAlive. There might also be necessary to create a cleanup of lease grants for deleted users. @fguimond, out of curiosity, why would you keep expired token?
|
Signed-off-by: manisha kumari <[email protected]>
Signed-off-by: manisha kumari <[email protected]>
Closed #5063
Description
The user sessions gets piled up in etcd occupying upto 2bg space .
Change in behavior
Deleting the etcd user_sesssions them after 6 minutes of creation ( Access token of session gets expired in 5 mnts) This change will make sure the memory occupancy uptil 1.8 GB will be eradicated
Added
Added TTL time of 6 minutes in session.go file for each entry regarding user-sessions.
Fixed
This fixes the etcd space occupancy issue .
Change verification