Skip to content

Seed zero-value quota metric on bucket create / quota enable #6221

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

Open
delthas wants to merge 4 commits into
base: development/9.4
Choose a base branch
from
+514 −320
Open

Seed zero-value quota metric on bucket create / quota enable #6221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ecae692
Prettier reformat files touched by CLDSRV-949
delthas Jul 6, 2026
e52b428
Bump arsenal to include initializeBucketCapacity
delthas Jul 7, 2026
6803d3c
Seed zero-value quota metric on bucket create / quota enable
delthas Jul 6, 2026
7e7a71a
Migrate bucketUpdateQuota to async/await
delthas Jul 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
314 changes: 175 additions & 139 deletions lib/api/apiUtils/bucket/bucketCreation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
const zenkoSeparator = constants.zenkoSeparator;
const userBucketOwner = 'admin';


function addToUsersBucket(canonicalID, bucketName, bucketMD, log, cb) {
// BACKWARD: Simplify once do not have to deal with old
// usersbucket name and old splitter
Expand All @@ -28,8 +27,7 @@
if (err && !err.is.NoSuchBucket && !err.is.BucketAlreadyExists) {
return cb(err);
}
const splitter = usersBucketAttrs ?
constants.splitter : constants.oldSplitter;
const splitter = usersBucketAttrs ? constants.splitter : constants.oldSplitter;
let key = createKeyForUserBucket(canonicalID, splitter, bucketName);
const omVal = {
creationDate: new Date().toJSON(),
Expand All @@ -38,46 +36,44 @@
// If the new format usersbucket does not exist, try to put the
// key in the old usersBucket using the old splitter.
// Otherwise put the key in the new format usersBucket
const usersBucketBeingCalled = usersBucketAttrs ?
usersBucket : oldUsersBucket;
return metadata.putObjectMD(usersBucketBeingCalled, key,
omVal, {}, log, err => {
if (err?.is?.NoSuchBucket) {
// There must be no usersBucket so createBucket
// one using the new format
log.trace('users bucket does not exist, ' +
'creating users bucket');
key = `${canonicalID}${constants.splitter}` +
`${bucketName}`;
const creationDate = new Date().toJSON();
const freshBucket = new BucketInfo(usersBucket,
userBucketOwner, userBucketOwner, creationDate,
BucketInfo.currentModelVersion());
return metadata.createBucket(usersBucket,
freshBucket, log, err => {
// Note: In the event that two
// users' requests try to create the
// usersBucket at the same time,
// this will prevent one of the users
// from getting a BucketAlreadyExists
// error with respect
// to the usersBucket.
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && !err.BucketAlreadyExists) {
log.error('error from metadata', {
error: err,
});
return cb(err);
}
log.trace('Users bucket created');
// Finally put the key in the new format
// usersBucket
return metadata.putObjectMD(usersBucket,
key, omVal, {}, log, cb);
const usersBucketBeingCalled = usersBucketAttrs ? usersBucket : oldUsersBucket;
return metadata.putObjectMD(usersBucketBeingCalled, key, omVal, {}, log, err => {
if (err?.is?.NoSuchBucket) {
// There must be no usersBucket so createBucket
// one using the new format
log.trace('users bucket does not exist, ' + 'creating users bucket');
key = `${canonicalID}${constants.splitter}` + `${bucketName}`;
const creationDate = new Date().toJSON();
const freshBucket = new BucketInfo(
usersBucket,
userBucketOwner,
userBucketOwner,
creationDate,
BucketInfo.currentModelVersion(),
);
return metadata.createBucket(usersBucket, freshBucket, log, err => {
// Note: In the event that two
// users' requests try to create the
// usersBucket at the same time,
// this will prevent one of the users
// from getting a BucketAlreadyExists
// error with respect
// to the usersBucket.
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && !err.BucketAlreadyExists) {
log.error('error from metadata', {
error: err,
});
}
return cb(err);
});
return cb(err);
}
log.trace('Users bucket created');
// Finally put the key in the new format
// usersBucket
return metadata.putObjectMD(usersBucket, key, omVal, {}, log, cb);
});
}
return cb(err);
});
});
}

Expand All @@ -89,6 +85,18 @@
return metadata.updateBucket(bucketName, bucket, log, callback);
}

function seedBucketQuotaCapacity(bucket, log, done) {

@DarkIsDude DarkIsDude Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This function is a new one and must use async/await. We agreed to don't do it for prettier one, but this one is a new one 🙏

if (!config.isQuotaEnabled()) {
return done();
}
return metadata.initializeBucketCapacity(bucket.getName(), bucket.getCreationDate(), log, err => {
if (err) {
log.error('error seeding bucket quota capacity metric', { error: err });

@DarkIsDude DarkIsDude Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why we just log and don't propagate it ?

}
return done();
});
}

Check notice

Code scanning / CodeQL

Callback-style function (async migration) Note

This function uses a callback parameter ('done'). Refactor to async/await.
Comment on lines +88 to +98

function freshStartCreateBucket(bucket, canonicalID, log, callback) {
const bucketName = bucket.getName();
metadata.createBucket(bucketName, bucket, log, err => {
Expand All @@ -101,7 +109,12 @@
if (err) {
return callback(err);
}
return removeTransientOrDeletedLabel(bucket, log, callback);
return removeTransientOrDeletedLabel(bucket, log, labelErr => {

@DarkIsDude DarkIsDude Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here, you touch the function, it's not only a formatting 🙏

if (labelErr) {
return callback(labelErr);
}
return seedBucketQuotaCapacity(bucket, log, () => callback(null));

@DarkIsDude DarkIsDude Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why callback(null) ? Why not callback() ?

});
});
});
}
Expand All @@ -124,7 +137,12 @@
if (err) {
return callback(err);
}
return removeTransientOrDeletedLabel(bucketMD, log, callback);
return removeTransientOrDeletedLabel(bucketMD, log, labelErr => {
if (labelErr) {
return callback(labelErr);
}
return seedBucketQuotaCapacity(bucketMD, log, () => callback(null));

@benzekrimaha benzekrimaha Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

seeding also fires on the transient/deleted-flag cleanup path (re-creating a bucket left in transient state), which the PR description doesn't mention and no test covers, codecov flags 5 missed lines in this file. Is the creationDate used here guaranteed to be the new incarnation's metastore date that _evaluateQuotas will query? can we add a test for this path (or explain why it can't be hit with quotas enabled).

});
});
}

Expand All @@ -139,16 +157,15 @@
* @callback called with (err, sseInfo: object)
*/
function bucketLevelServerSideEncryption(bucket, headers, log, cb) {
kms.bucketLevelEncryption(
bucket, headers, log, (err, sseInfo) => {
if (err) {
log.debug('error getting bucket encryption info', {
error: err,
});
return cb(err);
}
return cb(null, sseInfo);
});
kms.bucketLevelEncryption(bucket, headers, log, (err, sseInfo) => {
if (err) {
log.debug('error getting bucket encryption info', {
error: err,
});
return cb(err);
}
return cb(null, sseInfo);
});
}

/**
Expand All @@ -163,135 +180,154 @@
* @param {function} cb - callback to bucketPut
* @return {undefined}
*/
function createBucket(authInfo, bucketName, headers,
locationConstraint, log, cb) {
function createBucket(authInfo, bucketName, headers, locationConstraint, log, cb) {
// Prefer using undefined instead of null for unused properties so they are not present in metadata payload
log.trace('Creating bucket');
assert.strictEqual(typeof bucketName, 'string');
const canonicalID = authInfo.getCanonicalID();
const ownerDisplayName =
authInfo.getAccountDisplayName();
const ownerDisplayName = authInfo.getAccountDisplayName();
const creationDate = new Date().toJSON();
const isNFSEnabled = headers['x-scal-nfs-enabled'] === 'true' || undefined;
const headerObjectLock = headers['x-amz-bucket-object-lock-enabled'];
const objectLockEnabled
= headerObjectLock && headerObjectLock.toLowerCase() === 'true';
const bucket = new BucketInfo(bucketName, canonicalID, ownerDisplayName,
creationDate, BucketInfo.currentModelVersion(), undefined, undefined, undefined, undefined,
undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, isNFSEnabled,
undefined, undefined, objectLockEnabled);
const objectLockEnabled = headerObjectLock && headerObjectLock.toLowerCase() === 'true';
const bucket = new BucketInfo(

@DarkIsDude DarkIsDude Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

😂 this is where you know that a class is too large 😂

@DarkIsDude DarkIsDude Jul 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(nothing to do)

bucketName,
canonicalID,
ownerDisplayName,
creationDate,
BucketInfo.currentModelVersion(),
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
undefined,
isNFSEnabled,
undefined,
undefined,
objectLockEnabled,
);
let locationConstraintVal = null;

if (locationConstraint) {
const [locationConstraintStr, ingestion] =
locationConstraint.split(zenkoSeparator);
const [locationConstraintStr, ingestion] = locationConstraint.split(zenkoSeparator);
if (locationConstraintStr) {
locationConstraintVal = locationConstraintStr;
bucket.setLocationConstraint(locationConstraintStr);
}
if (ingestion === 'ingest') {
bucket.enableIngestion();
//automatically enable versioning for ingestion buckets
bucket.setVersioningConfiguration({ Status: 'Enabled' });
}
}
if (objectLockEnabled) {
// default versioning configuration AWS sets
// when a bucket is created with object lock
const versioningConfiguration = {
Status: 'Enabled',
MfaDelete: 'Disabled',
};
bucket.setVersioningConfiguration(versioningConfiguration);
}
const parseAclParams = {
headers,
resourceType: 'bucket',
acl: bucket.acl,
log,
};
async.parallel({
prepareNewBucketMD: function prepareNewBucketMD(callback) {
acl.parseAclFromHeaders(parseAclParams, (err, parsedACL) => {
if (err) {
log.debug('error parsing acl from headers', {
error: err,
});
return callback(err);
}
bucket.setFullAcl(parsedACL);
return callback(null, bucket);
});
},
getAnyExistingBucketInfo: function getAnyExistingBucketInfo(callback) {
metadata.getBucket(bucketName, log, (err, data) => {
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && err.NoSuchBucket) {
return callback(null, 'NoBucketYet');
}
if (err) {
return callback(err);
}
return callback(null, data);
});
async.parallel(
{
prepareNewBucketMD: function prepareNewBucketMD(callback) {
acl.parseAclFromHeaders(parseAclParams, (err, parsedACL) => {
if (err) {
log.debug('error parsing acl from headers', {
error: err,
});
return callback(err);
}
bucket.setFullAcl(parsedACL);
return callback(null, bucket);
});
},

Check notice

Code scanning / CodeQL

Callback-style function (async migration) Note

This function uses a callback parameter ('callback'). Refactor to async/await.
getAnyExistingBucketInfo: function getAnyExistingBucketInfo(callback) {
metadata.getBucket(bucketName, log, (err, data) => {
// TODO: move to `.is` once BKTCLT-9 is done and bumped in Cloudserver
if (err && err.NoSuchBucket) {
return callback(null, 'NoBucketYet');
}
if (err) {
return callback(err);
}
return callback(null, data);
});
},

Check notice

Code scanning / CodeQL

Callback-style function (async migration) Note

This function uses a callback parameter ('callback'). Refactor to async/await.
},
},
// Function to run upon finishing both parallel requests
(err, results) => {
if (err) {
return cb(err);
}
const existingBucketMD = results.getAnyExistingBucketInfo;
if (existingBucketMD instanceof BucketInfo &&
existingBucketMD.getOwner() !== canonicalID &&
!isServiceAccount(canonicalID)) {
// return existingBucketMD to collect cors headers
return cb(errors.BucketAlreadyExists, existingBucketMD);
}
const newBucketMD = results.prepareNewBucketMD;
if (existingBucketMD === 'NoBucketYet') {
const bucketSseConfig = parseBucketEncryptionHeaders(headers);
// Function to run upon finishing both parallel requests
(err, results) => {
if (err) {
return cb(err);
}
const existingBucketMD = results.getAnyExistingBucketInfo;
if (
existingBucketMD instanceof BucketInfo &&
existingBucketMD.getOwner() !== canonicalID &&
!isServiceAccount(canonicalID)
) {
// return existingBucketMD to collect cors headers
return cb(errors.BucketAlreadyExists, existingBucketMD);
}
const newBucketMD = results.prepareNewBucketMD;
if (existingBucketMD === 'NoBucketYet') {
const bucketSseConfig = parseBucketEncryptionHeaders(headers);

// Apply global SSE configuration when global encryption is enabled
// and no SSE settings were specified during bucket creation.
// Bucket-specific SSE headers override the default encryption.
const sseConfig = config.globalEncryptionEnabled && !bucketSseConfig.algorithm
? {
algorithm: 'AES256',
mandatory: true,
} : bucketSseConfig;
// Apply global SSE configuration when global encryption is enabled
// and no SSE settings were specified during bucket creation.
// Bucket-specific SSE headers override the default encryption.
const sseConfig =
config.globalEncryptionEnabled && !bucketSseConfig.algorithm
? {
algorithm: 'AES256',
mandatory: true,
}
: bucketSseConfig;

return bucketLevelServerSideEncryption(
bucket, sseConfig, log,
(err, sseInfo) => {
return bucketLevelServerSideEncryption(bucket, sseConfig, log, (err, sseInfo) => {
if (err) {
return cb(err);
}
newBucketMD.setServerSideEncryption(sseInfo);
log.trace(
'new bucket without flags; adding transient label');
log.trace('new bucket without flags; adding transient label');
newBucketMD.addTransientFlag();
return freshStartCreateBucket(newBucketMD, canonicalID,
log, cb);
return freshStartCreateBucket(newBucketMD, canonicalID, log, cb);
});
}
if (existingBucketMD.hasTransientFlag() ||
existingBucketMD.hasDeletedFlag()) {
log.trace('bucket has transient flag or deleted flag. cleaning up');
return cleanUpBucket(newBucketMD, canonicalID, log, cb);
}
// If bucket already exists in non-transient and non-deleted
// state and owned by requester, then return BucketAlreadyOwnedByYou
// error unless old AWS behavior (us-east-1)
// Existing locationConstraint must have legacyAwsBehavior === true
// New locationConstraint should have legacyAwsBehavior === true
if (isLegacyAWSBehavior(locationConstraintVal) &&
isLegacyAWSBehavior(existingBucketMD.getLocationConstraint())) {
log.trace('returning 200 instead of 409 to mirror us-east-1');
return cb(null, existingBucketMD);
}
return cb(errors.BucketAlreadyOwnedByYou, existingBucketMD);
});
}
if (existingBucketMD.hasTransientFlag() || existingBucketMD.hasDeletedFlag()) {
log.trace('bucket has transient flag or deleted flag. cleaning up');
return cleanUpBucket(newBucketMD, canonicalID, log, cb);
}
// If bucket already exists in non-transient and non-deleted
// state and owned by requester, then return BucketAlreadyOwnedByYou
// error unless old AWS behavior (us-east-1)
// Existing locationConstraint must have legacyAwsBehavior === true
// New locationConstraint should have legacyAwsBehavior === true
if (
isLegacyAWSBehavior(locationConstraintVal) &&
isLegacyAWSBehavior(existingBucketMD.getLocationConstraint())
) {
log.trace('returning 200 instead of 409 to mirror us-east-1');
return cb(null, existingBucketMD);
}
return cb(errors.BucketAlreadyOwnedByYou, existingBucketMD);
},
);
}

Check notice

Code scanning / CodeQL

Callback-style function (async migration) Note

This function uses a callback parameter ('cb'). Refactor to async/await.

module.exports = {
Expand Down
Loading
Loading