Improve BackbeatAPI pause/resume logic #2644

benzekrimaha · 2025-04-25T07:37:01Z

bert-e · 2025-04-25T07:37:35Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-04-25T07:37:43Z

Incorrect fix version

The Fix Version/s in issue BB-658 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.0.7
9.1.0

Please check the Fix Version/s of BB-658, or the target
branch of this pull request.

lib/util/LocationStatusManager.js

codecov · 2025-05-07T11:05:25Z

Codecov Report

Attention: Patch coverage is 84.39024% with 32 lines in your changes missing coverage. Please review.

Project coverage is 74.25%. Comparing base (487dbb6) to head (91a075a).
Report is 5 commits behind head on development/9.1.

Files with missing lines	Patch %	Lines
lib/util/LocationStatusManager.js	84.39%	32 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/util/LocationStatusManager.js	`83.38% <84.39%> (+0.99%)`	⬆️

... and 3 files with indirect coverage changes

Components	Coverage Δ
Bucket Notification	`75.40% <ø> (ø)`
Core Library	`80.52% <84.39%> (+0.67%)`	⬆️
Ingestion	`70.26% <ø> (ø)`
Lifecycle	`77.94% <ø> (ø)`
Oplog Populator	`85.06% <ø> (ø)`
Replication	`61.15% <ø> (ø)`
Bucket Scanner	`85.60% <ø> (ø)`

@@                 Coverage Diff                 @@
##           development/9.1    #2644      +/-   ##
===================================================
+ Coverage            73.91%   74.25%   +0.34%     
===================================================
  Files                  201      201              
  Lines                13446    13591     +145     
===================================================
+ Hits                  9938    10092     +154     
+ Misses                3498     3489       -9     
  Partials                10       10

Flag	Coverage Δ
bucket-scanner	`85.60% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

bert-e · 2025-05-07T11:05:30Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

bert-e · 2025-05-12T13:10:52Z

Incorrect fix version

The Fix Version/s in issue BB-658 contains:

9.1.0

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.0.7
9.1.0

Please check the Fix Version/s of BB-658, or the target
branch of this pull request.

francoisferrand · 2025-05-12T13:59:45Z

lib/util/LocationStatusManager.js

@@ -438,7 +434,9 @@ class LocationStatusManager {
                    date.setMinutes(date.getMinutes() + 1);
                    this._scheduledResumeJobs[service][location] = schedule.scheduleJob(date,
                        triggerResume.bind(this));
+                    return;


should add a test for this case

francoisferrand · 2025-05-12T14:08:41Z

lib/util/LocationStatusManager.js

-                this._deleteResumeJob(location, service);
-                this._locationStatusStore[location].setServiceResumeSchedule(service, null);
-                return this._updateServiceStatusForLocation(location, next);
+                this._deleteResumeJob(location, service);                


here _deleteResumeJob() will be applied even if we fail to update the state in mongo : leading to inconsistent state...

francoisferrand · 2025-05-12T16:20:10Z

lib/util/LocationStatusManager.js

                }
+                this._locationStatusStore[location].resumeLocation(service);


setting the "in-memory" value afterwards allows to handle errors, but does not actually solve everything: there are still many race conditions here...

First and foremost, since we set the value afterwards, changing the value quickly will not "toggle" but keep trying to do the same transition

In addition, the state is changed asynchronously: so we may make multiple changes to mongo in parallel, and the results may not be processed in the same order...and so the (in-mem) state may not match the state in mongo (i.e. send "Pause" to mongo ; send "Resume" to mongo ; process the end of the "Resume" call ; process of the "Suspend" call → in mongo we may have state Resume, but store "suspend" in memory)

→ the way to handle this is to actually have a proper automaton, so we can detect the "conflicting" operations and deal with them (reject, retry, ignore/wait for completion...)

francoisferrand · 2025-05-12T16:20:46Z

lib/util/LocationStatusManager.js

+                this._deleteResumeJob(location, service);                
+                return this._updateServiceStatusForLocation(location, err => {
+                    if (err) {
+                        this._logger.debug('failed to delete scheduled resume', {


francoisferrand · 2025-05-12T16:21:26Z

lib/util/LocationStatusManager.js

+            if (!paused) {
+                this._updateServiceStatusForLocation(location, err => {
+                    if (err) {
+                        this._logger.debug('failed to pause service', {


maybe this log should be moved to _updateServiceStatusForLocation, may be less redundant than having it after each call...

bert-e · 2025-05-13T07:54:05Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

This commit fixes the callback error format in the LocationStatusManager class. It also ensures that the status is stored in memory only if it has been updated in MongoDB successfully, ensuring thus consistensy between both. Issue: BB-658

williamlardier

I tried to review everything but due to some changes I discuss, some of my comments will likely end up irrelevant, I suggest reading the whole review and work on the "big part"s first, so you don't waste time addressing some of my comments I wrote with the current code

williamlardier · 2025-06-27T12:35:50Z

lib/util/LocationStatusManager.js


-    /**
+   /**


Suggested change

/**

/**

williamlardier · 2025-06-27T12:36:04Z

lib/util/LocationStatusManager.js

@@ -343,7 +924,7 @@ class LocationStatusManager {
            return cb(null, statuses);
        });
    }
-
+    


Suggested change

williamlardier · 2025-07-01T10:18:13Z

lib/util/LocationStatusManager.js

@@ -12,20 +12,156 @@ const actions = {
    resume: 'resumeService',
 };

+// Operation types for the state machine
+const OPERATION_TYPES = {


Suggested change

const OPERATION_TYPES = {

const OPERATION_TYPES = Object.freeze({

if we want to use enum-like values without TS I suggest using this to ensure it will never be changed in any way

williamlardier · 2025-07-01T10:20:11Z

lib/util/LocationStatusManager.js

+    }
+
+    canTransitionTo(newState) {
+        const validTransitions = {


Suggested change

const validTransitions = {

const validTransitions = Object.freeze({

williamlardier · 2025-07-01T10:23:24Z

lib/util/LocationStatusManager.js

+/**
+ * Represents a queued operation
+ */
+class QueuedOperation {


This is a generic class name that should:

Have a more specific name if we can only handle specific operations, here it seems it's related to location, so it should be in the name (maybe ServiceOperation?)

Be fully unit tested

Be in a dedicated file, as we tend to avoid having multiple classes in the same file

williamlardier · 2025-07-01T10:49:45Z

lib/util/LocationStatusManager.js

+            if (operation.retryCount < operation.maxRetries) {
+                setTimeout(() => {
+                    this._operationQueue.push(operation);
+                }, 1000 * Math.pow(2, operation.retryCount));


Do we have support for exponentional backoff in arsenal?
We should add a comment maybe stating the periods it gives given the number of retries we have right now, maybe it will be too big or too small

williamlardier · 2025-07-01T10:50:24Z

lib/util/LocationStatusManager.js

+                        { key, oldState: stateMachineInstance.state, newState: newSmState, 
+                          oldScheduledResume: stateMachineInstance.scheduledResumeDate, 
+                          newScheduledResume: newScheduledResumeDate,
+                          zkDataUsed: currentZkData });


Suggested change

{ key, oldState: stateMachineInstance.state, newState: newSmState,

oldScheduledResume: stateMachineInstance.scheduledResumeDate,

newScheduledResume: newScheduledResumeDate,

zkDataUsed: currentZkData });

{

key, oldState: stateMachineInstance.state,

newState: newSmState,

oldScheduledResume: stateMachineInstance.scheduledResumeDate,

newScheduledResume: newScheduledResumeDate,

zkDataUsed: currentZkData,

});

williamlardier · 2025-07-01T10:51:32Z

lib/util/LocationStatusManager.js

+                    (stateMachineInstance.scheduledResumeDate ? 
+                        stateMachineInstance.scheduledResumeDate.toISOString() : null) !== 


Suggested change

(stateMachineInstance.scheduledResumeDate ?

stateMachineInstance.scheduledResumeDate.toISOString() : null) !==

(stateMachineInstance.scheduledResumeDate?.toISOString?.()

Also what if we get two null (or undefined with my suggestion) -> the condition will pass, is that what we want?

williamlardier · 2025-07-01T10:54:06Z

lib/util/LocationStatusManager.js

+            if (this._serviceConfig[service] && !this._serviceConfig[service].isMongo) {
+                this._logger.debug('Initializing state machine for ZK-based service', { key, service, location });
+                try {
+                    sourceData = await this._getZkDataForLocation(service, location);


This calls ZK every time we process an operation, is that OK? I saw we increased the timeouts in tests from 1000ms to 2000ms, is this change fine if we pause/resume at high rate?

williamlardier · 2025-07-01T10:54:41Z

lib/util/LocationStatusManager.js

+        if (transition) {
+            transition();
+        }


Suggested change

if (transition) {

transition();

}

transition?.();

A log in case there is no transition?

Kerkesni reviewed Apr 25, 2025

View reviewed changes

lib/util/LocationStatusManager.js Outdated Show resolved Hide resolved

Kerkesni reviewed Apr 25, 2025

View reviewed changes

lib/util/LocationStatusManager.js Outdated Show resolved Hide resolved

benzekrimaha force-pushed the improvement/BB-658 branch 3 times, most recently from 5c752cc to 8bdad47 Compare May 7, 2025 11:04

scality deleted a comment from codecov bot May 7, 2025

benzekrimaha marked this pull request as ready for review May 7, 2025 11:05

benzekrimaha changed the title ~~Improvement/bb 658~~ Improve BackbeatAPI pause/resume logic May 12, 2025

francoisferrand reviewed May 12, 2025

View reviewed changes

benzekrimaha changed the base branch from development/9.0 to development/9.1 May 13, 2025 07:53

benzekrimaha force-pushed the improvement/BB-658 branch from c0d3dcc to ff585d7 Compare June 12, 2025 07:27

benzekrimaha force-pushed the improvement/BB-658 branch from ff585d7 to c054a62 Compare June 26, 2025 07:47

benzekrimaha added 3 commits June 26, 2025 09:51

introduction of state machine logic

a948817

tests update to match the new logic

a22cfb0

benzekrimaha force-pushed the improvement/BB-658 branch from c054a62 to a22cfb0 Compare June 26, 2025 07:51

benzekrimaha requested review from francoisferrand and Kerkesni June 27, 2025 14:16

williamlardier reviewed Jul 1, 2025

View reviewed changes

benzekrimaha added 4 commits July 15, 2025 17:25

fixup post reviews

2a5e3f9

fixup post reviews

7be2664

fixup post reviews

f809c48

fixup post reviews

4516f30

fixup post reviews

91a075a

		}
		this._locationStatusStore[location].resumeLocation(service);

	const OPERATION_TYPES = {
	const OPERATION_TYPES = Object.freeze({

	const validTransitions = {
	const validTransitions = Object.freeze({

		(stateMachineInstance.scheduledResumeDate ?
		stateMachineInstance.scheduledResumeDate.toISOString() : null) !==

Improve BackbeatAPI pause/resume logic #2644

Are you sure you want to change the base?

Improve BackbeatAPI pause/resume logic #2644

Uh oh!

Conversation

benzekrimaha commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Apr 25, 2025

Hello benzekrimaha,

Uh oh!

bert-e commented Apr 25, 2025

Incorrect fix version

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bert-e commented May 7, 2025

Request integration branches

Uh oh!

bert-e commented May 12, 2025

Incorrect fix version

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bert-e commented May 13, 2025

Waiting for approval

Uh oh!

williamlardier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benzekrimaha commented Apr 25, 2025 •

edited

Loading

codecov bot commented May 7, 2025 •

edited

Loading