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

NC | Accounts ID cache addition #8304

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

romayalon
Copy link
Contributor

@romayalon romayalon commented Aug 22, 2024

Explain the changes

  1. A follow-up PR to NC | Bucket Owner Removal #8289.
  2. Added in accountspaceFS accounts id cache (mapped to account config)
  3. Changed the calls from get_account_by_idnetity() to use the cache.

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

  • Doc added/updated
  • Tests added

src/sdk/accountspace_fs.js Outdated Show resolved Hide resolved
@shirady
Copy link
Contributor

shirady commented Aug 26, 2024

@romayalon The main open points I have in this PR are:

  • The account_id_cache is based on account_cache with 2 differences: (1) expiry by 3 minutes (instead of 10 minutes by default) (2) use get_identity_by_id (read account by ID instead of read_account_by_access_key read account by access key). I remembered that @guymguym had in mind that were aspects that bucket_namespace_cache was better than account_cache.
  • More general about the cache we use (both of the cache we have) - the cache is saved in memory per process, so changes won't be reflected in other processes - for example, any change to an account that will be made by the CLI will not be reflected in endpoint and they will still read the account with the previous changes - can be with S3 API and IAM API. As I understood creating this communication was discussed in the past (it was thought it was complex).

@romayalon
Copy link
Contributor Author

romayalon commented Aug 29, 2024

@shirady @guymguym I suggest discussing it next week

  1. Bucket cache is better because we invalidate the items of the cache when doing changes on the bucket, while we don't do that on the accounts cache.
  2. Yes, this is a change @guymguym and I discussed in the past for master keys, we can schedule a meeting for considering it again but this is out of scope for this PR.

Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

A few small comments

src/sdk/accountspace_fs.js Outdated Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_cli_utils.js Outdated Show resolved Hide resolved
src/manage_nsfs/manage_nsfs_validations.js Outdated Show resolved Hide resolved
src/sdk/accountspace_fs.js Show resolved Hide resolved
Copy link
Contributor

@shirady shirady left a comment

Choose a reason for hiding this comment

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

LGTM

@romayalon romayalon merged commit b7a2cae into noobaa:master Oct 15, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants