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

Etcd cleanup #5064

Closed
wants to merge 9 commits into from
Closed

Etcd cleanup #5064

wants to merge 9 commits into from

Conversation

ManishaKumari295
Copy link

@ManishaKumari295 ManishaKumari295 commented Sep 12, 2024

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

  • Do sensuctl configure.
  • Check respective entry in etcd ar /sensu.io/user-sessions/admin/xxyyzz[session_id]
  • this entry will be deleted automatically after 11 minutes if the backend keep .
  • I have the video of default behaviour ( which caused the issue) and the video of fixed behaviour with TTL=2mins , unfortunately cannot be uploaded due to big size.

Signed-off-by: manisha kumari <[email protected]>
@elfranne
Copy link

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]>
@ManishaKumari295
Copy link
Author

Thanks for the fix @ManishaKumari295 but it seems there is a issue with the branch...

Hi @elfranne , the branch seems to be fine now. Please verify the commited changes.
Thanks

@ManishaKumari295 ManishaKumari295 marked this pull request as ready for review September 17, 2024 09:13
@ManishaKumari295 ManishaKumari295 changed the base branch from main to develop/6 September 17, 2024 09:25
CHANGELOG-6.md Outdated
## [6.11.1] - 2024-09-12

### Changed
- Added TTl to each entry in user-session within etcd
Copy link
Contributor

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.

@@ -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
Copy link
Contributor

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.

@fguimond
Copy link
Contributor

Great approach @ManishaKumari295. Just a couple of minor things and it should be good to go!

@elfranne
Copy link

I have limited knowledge in the inner workings of the Sensu, please take my findings with a grain of salt.
After some research I can see that Etcd has a feature to create a lease grant:

LeaseGrant creates a lease which expires if the server does not receive a keepAlive within a given time to live period. All keys attached to the lease will be expired and deleted if the lease expires. Each expired key generates a delete event in the event history.

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?

(token expiration * 1.5)

Signed-off-by: manisha kumari <[email protected]>
Signed-off-by: manisha kumari <[email protected]>
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.

etcd slowly increase in size, sessions cleanup missing
3 participants