Skip to content

Fix triggering replication when multiple destinations are set #331

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

Merged
bert-e merged 3 commits into from
Jan 14, 2025
Merged

Fix triggering replication when multiple destinations are set #331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6cbfd22
fix triggering replication when multpile destinations are set
Dec 20, 2024
b66c7f4
setup oras in build workflow
Jan 13, 2025
e82f3ea
install libssl1.1 in ci
Jan 14, 2025
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
7 changes: 7 additions & 0 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ jobs:
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Install Oras
run: |
curl -L https://github.com/oras-project/oras/releases/download/v${ORAS_VERSION}/oras_${ORAS_VERSION}_linux_amd64.tar.gz | \
tar -xz -C /usr/local/bin oras
env:
ORAS_VERSION: 1.2.2

- name: Login to Registry
uses: docker/login-action@v3
with:
Expand Down
6 changes: 6 additions & 0 deletions .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,12 @@ jobs:
node-version: '16'
cache: 'yarn'
- run: sudo apt-get update -q
# Ubuntu 24.04 doesn't have libssl1.1 which is needed by mongo-memory-server
- name: install libssl1.1
run: |
mkdir -p ~/ssl && cd ~/ssl
wget -nc https://ftp.debian.org/debian/pool/main/o/openssl/libssl1.1_1.1.1w-0+deb11u1_amd64.deb
sudo dpkg -i libssl1.1_1.1.1w-0+deb11u1_amd64.deb
- run: yarn --network-concurrency 1
- run: yarn --silent lint -- --max-warnings 0
- run: yarn --silent test:unit
Expand Down
56 changes: 40 additions & 16 deletions CRR/ReplicationStatusUpdater.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,16 +114,19 @@ class ReplicationStatusUpdater {
* Determines if an object should be updated based on its replication metadata properties.
* @private
* @param {ObjectMD} objMD - The metadata of the object.
* @param {string} site - The destination site name.
* @returns {boolean} True if the object should be updated.
*/
_objectShouldBeUpdated(objMD) {
_objectShouldBeUpdated(objMD, site) {
return this.replicationStatusToProcess.some(filter => {
if (filter === 'NEW') {
// Either site specific replication info is missing
// or are initialized with empty fields.
return (!objMD.getReplicationInfo()
|| objMD.getReplicationInfo().status === '');
|| !objMD.getReplicationSiteStatus(site));
}
return (objMD.getReplicationInfo()
&& objMD.getReplicationInfo().status === filter);
&& objMD.getReplicationSiteStatus(site) === filter);
Comment on lines +126 to +129
Copy link
Contributor

@francoisferrand francoisferrand Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

is this true as well for CRR (in s3c) ?

esp. we should check that there is no "legacy" data format where we would not have the per-site status, and where we should rely only on the global state...

(e.g. maybe if CRR sometimes has objet which are marked with replicationInfo.status === 'COMPLETE' and site listed in storageClass, but no site status...)

Copy link
Contributor

@francoisferrand francoisferrand Jan 8, 2025

Choose a reason for hiding this comment

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

@nicolas2bert @jonathan-gramain what do you think?

Copy link
Author

@welansari welansari Jan 13, 2025

Choose a reason for hiding this comment

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

Replication metadata are managed the same in both S3C and Zenko so yes this should work fine.
I looked back to the initial format of the metadata, there was always a global status.

});
}

Expand Down Expand Up @@ -172,36 +175,54 @@ class ReplicationStatusUpdater {
// codebase easier to maintain and upgrade, as opposed to having multiple branches or versions of
// the code for different schema versions.
objMD = new ObjectMD(JSON.parse(mdRes.Body));
if (!this._objectShouldBeUpdated(objMD)) {
if (!this._objectShouldBeUpdated(objMD, storageClass)) {
skip = true;
return process.nextTick(next);
}
// Initialize replication info, if missing
// This is particularly important if the object was created before
// enabling replication on the bucket.
if (!objMD.getReplicationInfo()
|| !objMD.getReplicationSiteStatus(storageClass)) {
let replicationInfo = objMD.getReplicationInfo();
if (!replicationInfo || !replicationInfo.status) {
const { Rules, Role } = repConfig;
const destination = Rules[0].Destination.Bucket;
Copy link
Contributor

@francoisferrand francoisferrand Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this may not be correct, if there is more than 1 desitination....

Copy link
Author

@welansari welansari Dec 23, 2024

Choose a reason for hiding this comment

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

crrExistingObjects doesn't currently support triggering replication for multiple sites at once. So this would work as it will initialize the replication info when triggering the first site and then append the other sites' info to the fields when the other ones are triggered.

Copy link
Contributor

@francoisferrand francoisferrand Dec 26, 2024

Choose a reason for hiding this comment

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

even without triggering multiple sites at once : if a user requests replicatoin to the second "site", this will initialize with the 1st destination (--> trigger replication!), then we will add the second one...

When there is no replicationInfo (i.e. typically when it is empty), should we not just initialize an empty replicationInfo, and let the next block ("Update replication info with site specific info") fill the details for the requested destination?

Copy link
Contributor

@francoisferrand francoisferrand Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

in cloudserver, it seems this is initialized to bucketMD.replicationConfig.destination : should we do the same?

Copy link
Author

@welansari welansari Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a weird one... So, we currently only support having a single destination bucket for all replication rules of a bucket. When creating the rules/workflows via UI it's even worse, the destination bucket becomes the name of the bucket we are replicating from.
The value of this field is not used in Zenko. When creating a location, Cloudserver initializes a client class for the respective backend (aws/azure/...) that keeps the name of the destination bucket in memory, that's the value we use when replicating and not what's in the replication rule (only the storageClass is used to know which client to use).

// set replication properties
const ops = objMD.getContentLength() === 0 ? ['METADATA']
: ['METADATA', 'DATA'];
const backends = [{
site: storageClass,
status: 'PENDING',
dataStoreVersionId: '',
}];
const replicationInfo = {
replicationInfo = {
status: 'PENDING',
backends,
content: ops,
backends: [],
destination,
storageClass,
storageClass: '',
role: Role,
storageType: this.storageType,
storageType: '',
Comment on lines +197 to +199
Copy link
Contributor

@francoisferrand francoisferrand Dec 20, 2024

Choose a reason for hiding this comment

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

what are these 2 fields (storageClass and storageType) used for?

Copy link
Author

@welansari welansari Dec 23, 2024

Choose a reason for hiding this comment

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

storageClass is what each location's replication queue processor uses to check if it should replicate an object or not.
storageType is used by Cloudserver in Backbeat routes to do some pre-checks (check if versioning is supported on the backend and that the location is valid)

Copy link
Contributor

@francoisferrand francoisferrand Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

that is how these fields are used... my question is really what information do they actually store, what these fields represent (for example, the storageClass of the object is already known, and stored in .location[] and dataStoreName ; and this cannot be the storageClass of the 'remote' object, since there may be multiple destinations...)

same for destination btw, I don't understand this field initialize with a single bucket name :-/ Or maybe this is a left-over from the first replication (not supporting multi-targets)?

Copy link
Contributor

@francoisferrand francoisferrand Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the code which initializes these in cloudserver:

function _getReplicationInfo(rule, replicationConfig, content, operationType,
    objectMD, bucketMD) {
    const storageTypes = [];
    const backends = [];
    const storageClasses = _getStorageClasses(rule);
    if (!storageClasses) {
        return undefined;
    }
    storageClasses.forEach(storageClass => {
        const storageClassName =
              storageClass.endsWith(':preferred_read') ?
              storageClass.split(':')[0] : storageClass;
        const location = s3config.locationConstraints[storageClassName];
        if (location && replicationBackends[location.type]) {
            storageTypes.push(location.type);
        }
        backends.push(_getBackend(objectMD, storageClassName));
    });
    if (storageTypes.length > 0 && operationType) {
        content.push(operationType);
    }
    return {
        status: 'PENDING',
        backends,
        content,
        destination: replicationConfig.destination,
        storageClass: storageClasses.join(','),
        role: replicationConfig.role,
        storageType: storageTypes.join(','),
        isNFS: bucketMD.isNFS(),
    };
}

--> it seems these fields (along with role) are not multi-destination/rule aware?
--> these should be mostly useless for Zenko's multibackend replication (otherwise we probably have bugs), but may still be needed for CRR (and may cause bugs if multiple rules/destinations were used in that case)

Copy link
Author

@welansari welansari Dec 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

storageClass and storageType are multi-destination aware (although their name suggests otherwise), they contain the list of all storage classes and storage types we are replicating to.
Example:

storageClass: "aws-location,azure-blob"
storageType: "aws_s3,azure"

In Zenko, these fields are duplicates of information we already have, as backbeat/cloudserver have a list of all location information, we could just use the storage class stored in the rules to get the info we want. I think these are more of a relic from S3C that we can't really remove right now as S3C uses them.

In CRR, the role is also a list. I don't think this works in Zenko tho.
The destination field is a weird one, i explained how it works in the previous comment

Copy link
Contributor

@francoisferrand francoisferrand Jan 8, 2025

Choose a reason for hiding this comment

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

these are more of a relic from S3C that we can't really remove right now as S3C uses them
In CRR, the role is also a list. I don't think this works in Zenko tho.

"soon", we will have a single arsenal, cloudserver & backbeat : so we can start planning to clean this up, possibly removing (deprecating) redudant fields and aligning the way CRR and multi-backend replication store/manage the state?

can you please create a tech debt ticket, to track this (future) work and document the problem/situation/state of the analysis?

};
objMD.setReplicationInfo(replicationInfo);
}
// Update replication info with site specific info
if (!objMD.getReplicationSiteStatus(storageClass)) {
// When replicating to multiple destinations,
// the storageClass and storageType properties
// become comma-separated lists of the storage
// classes and types of the replication destinations.
const storageClasses = objMD.getReplicationStorageClass()
? `${objMD.getReplicationStorageClass()},${storageClass}` : storageClass;
objMD.setReplicationStorageClass(storageClasses);
if (this.storageType) {
const storageTypes = objMD.getReplicationStorageType()
? `${objMD.getReplicationStorageType()},${this.storageType}` : this.storageType;
objMD.setReplicationStorageType(storageTypes);
}
// Add site to the list of replication backends
const backends = objMD.getReplicationBackends();
backends.push({
site: storageClass,
status: 'PENDING',
dataStoreVersionId: '',
});
objMD.setReplicationBackends(backends);
}

objMD.setReplicationSiteStatus(storageClass, 'PENDING');
objMD.setReplicationStatus('PENDING');
Expand Down Expand Up @@ -273,13 +294,16 @@ class ReplicationStatusUpdater {
}),
(repConfig, next) => {
const { Rules } = repConfig;
const storageClass = Rules[0].Destination.StorageClass || this.siteName;
const storageClass = this.siteName || Rules[0].Destination.StorageClass;
if (!storageClass) {
const errMsg = 'missing SITE_NAME environment variable, must be set to'
+ ' the value of "site" property in the CRR configuration';
this.log.error(errMsg);
return next(new Error(errMsg));
}
if (!this.siteName) {
this.log.warn(`missing SITE_NAME environment variable, triggering replication to the ${storageClass} storage class`);
}
return eachLimit(versions, this.workers, (i, apply) => {
const { Key, VersionId } = i;
this._markObjectPending(bucket, Key, VersionId, storageClass, repConfig, apply);
Expand Down
Loading
Loading