Feat gh project 3 heartbeat calculation improvement #89

JaydenLiang · 2021-09-24T22:10:47Z

this PR target the issue #84

resolve #84 #85

enforce a stricter health check record update condition check on seq

record

if in the classic way(without device info), heartbeat with outdated sequence will be discarded

jamie-pate · 2021-10-04T23:31:43Z

core/context-strategy/autoscale-context.ts

@@ -242,6 +251,9 @@ export class ConstantIntervalHeartbeatSyncStrategy implements HeartbeatSyncStrat
            this.result = HealthCheckResult.OnTime;
            // no old next heartbeat time for the first heartbeat, use the current arrival time.
            oldNextHeartbeatTime = heartbeatArriveTime;
+            // no old record for reference, use the seq provided by the device. If the seq is
+            // NaN, it means no sequence provided by the device, then use 0.
+            oldSeq = (isNaN(deviceSyncInfo.sequence) && 0) || deviceSyncInfo.sequence;


(isNaN(NaN) && 0) || undefined -> undefined

you should use a ternary operator instead

isNaN(deviceSyncInfo.sequence) ? 0 : deviceSyncInfo.sequence

jamie-pate · 2021-10-04T23:32:39Z

core/context-strategy/autoscale-context.ts

@@ -252,9 +264,17 @@ export class ConstantIntervalHeartbeatSyncStrategy implements HeartbeatSyncStrat
                nextHeartbeatTime: nextHeartbeatTime,
                syncState: HeartbeatSyncState.InSync,
                syncRecoveryCount: 0, // sync recovery count = 0 means no recovery needed
-                seq: 1, // set to 1 because it is the first heartbeat
+                // use the device sequence if it exists or 1 as the initial sequence
+                seq: (!isNaN(deviceSyncInfo.sequence) && deviceSyncInfo.sequence) || 1,


ternary pls, this one is probably fine but the ternary is clearer.

!isNaN(deviceSyncInfo.sequence) ? deviceSyncInfo.sequence : 1

jamie-pate · 2021-10-04T23:39:33Z

core/context-strategy/autoscale-context.ts

+            if (useDeviceSyncInfo) {
+                delayCalculationMethod = 'by device send time';
+                // check if the sequence is in an incremental order compared to the data in the db
+                // if not, the heartbea should be marked as outdated and to be dropped


jamie-pate · 2021-10-04T23:40:49Z

core/context-strategy/autoscale-context.ts

+                    // in this situation, there are race conditions happening between some
+                    // heartbeat requests. The reason is one autoscale handler is taking much
+                    // longer to process another heartbeat and unable to complete before this
+                    // heartbeat arrives at the handler (by a parallel cloud function thread).


technically: 'parallel cloud function process' (maybe even on a separate machine)

jamie-pate · 2021-10-04T23:43:34Z

core/context-strategy/autoscale-context.ts

+                    // for the other hb (new seq > old seq + 1), the delay cannot be calculated
+                    // thus discarding the delay calculation, and trust it is an on-time hb
+                    else {
+                        delay = -1; // on-time hb must have a negative delay


seems like it would be cleaner to just have null and then handle the null case later. -1ms delay will be shown in the logs otherwise?

To properly account for this situation I guess you would need to store the last 3 heartbeat sequnce numbers in the database so you could retroactively count the missing sequence numbers?

10 -> 7 8 9 (+10): OK
12 -> 8 9 10 (+12): 1 missing...
11 -> 9 10 12 (+11): OK again

This is a simplified diagram for explanation. The final solution would need one record per heartbeat.. Could store the timestamp also so that could be (re)considered when the missing entries appear later.
(afterward delete them if the sequence number is lower than seq - 10 or so?)

This would allow you to calculate the loss count at each heartbeat based on multiple records from the db, rather than storing one record and overwriting each time and having issues with race conditions.

This is a good idea! I believe since we now have the sequence from the sender, we can also make good use of the sequence. Let me think it again.

jamie-pate · 2021-10-04T23:48:14Z

core/context-strategy/autoscale-context.ts

+            // calculate delay using the arrive time (classic method)
+            else {
+                // NOTE:
+                // heartbeatArriveTime: the starting time of the function execution, considerred as


considerred -> considered

jamie-pate · 2021-10-04T23:54:05Z

fortigate-autoscale/azure/azure-platform-adapter.ts

+                snapshot
+            );
+            // NOTE: strictly update the record when the sequence to update is equal to or greater
+            // than the seq in the db ton ensure data not to fall back to old value in race conditions


ton ensure -> to ensure

jamie-pate · 2021-10-04T23:54:48Z

fortigate-autoscale/azure/azure-platform-adapter.ts

+                },
+                snapshot
+            );
+            // NOTE: strictly update the record when the sequence to update is equal to or greater


keeping multiple heartbeat records instead of overwriting them might improve this situation.

good idea! The current heartbeat monitoring has been improved. Will take this idea into consideration next time.

…tings remove caching of loadSettings

Bumping version to 3.3.2

…ovement

clean stale health check records before redo primary election

JaydenLiang · 2021-11-08T18:45:36Z

latest QA/DEV release created: 3.3.3-dev.1

jamie-pate · 2021-11-17T20:20:21Z

core/autoscale-core.ts

+        // the action for updating primary record
+        let action: 'save' | 'delete' | 'noop' = 'noop';
+        let redoElection = false;
+        let reloadPrimarRecord = false;


reloadPrimaryRecord ?

jamie-pate · 2021-11-17T20:29:17Z

core/context-strategy/autoscale-context.ts

+            points += this.weightMethod1(rec);
+            points += this.weightMethod2(rec, allHealthCheckRecords);
+            points += this.weightMethod3(rec, allHealthCheckRecords);
+            points += this.weightMethod4(rec);
+            points += this.weightMethod5(rec, allHealthCheckRecords);
+            points += this.weightMethod6(rec, allHealthCheckRecords);


I think the weight methods should have descriptive names instead of 1 2 3 4 5 6....

haveChecksumWeight() + sharedChecksumWeight() + groupedChecksumWeight() + ... etc

Some weigh methods have mixed and complicated conditions which are hard to have a self-descriptive name so I gave up naming them in that way. I prefer to keep their naming well-formatted and with a better jsdoc. It also does the job. Any good IDE could display the information properly.

@jamie-pate Let me know whether the self-descriptive naming is still strongly desired to apply here in this case or not?

jamie-pate · 2021-12-07T22:01:45Z

core/context-strategy/autoscale-context.ts

+     * @returns {number} point
+     */
+    weightMethod1(rec: HealthCheckRecord): number {
+        return (rec.deviceChecksum !== null && 1) || 0;


Prefer rec.deviceChecksum !== null ? 1 : 0

misusing boolean operators instead is confusing.

JaydenLiang added 8 commits September 24, 2021 11:04

add a platform adapter function to retrieve device info from vm

ff06530

implement getReqDeviceSyncInfo() for AWS and Azure

306dadd

save/read new properties with db

400a8d3

(AWS) add support for using sequence provided by the device, and

c691cf9

enforce a stricter health check record update condition check on seq

(Azure) update saveDbItem function to take a checker function

d25c422

(Azure) add strict condition on the sequence when update healthcheck

302793c

record

minor changes in the type of new property from the device info

17f948a

improve heartbeat calculation with device provided information,

5f97226

if in the classic way(without device info), heartbeat with outdated sequence will be discarded

JaydenLiang requested review from jamie-pate and Joel-Cripps September 24, 2021 22:10

JaydenLiang added 2 commits September 24, 2021 15:14

update dependency axios@0.21.2

2c45748

update primary election handling and add a workflow diagrams

3c0e2d4

jamie-pate requested changes Oct 4, 2021

View reviewed changes

JaydenLiang and others added 13 commits October 12, 2021 13:09

remove caching of loadSettings

7d2b94a

rewrite the loadSetting method to run completely without caching

191d62d

primary election enhancement

6546845

code review related changes

23a905d

Merge pull request #93 from fortinet/patch_remove_caching_of_load_set…

b3ca979

…tings remove caching of loadSettings

release: 3.3.2

fa784e4

Merge pull request #94 from fortinet/rel_3.3.2_verbra

1d6c6a9

Bumping version to 3.3.2

Merge branch 'main' into feat_gh_project_3_heartbeat_calculation_impr…

d725a6c

…ovement

primary election bugfixes and

bd34d55

clean stale health check records before redo primary election

resolve a few heartbeat calculation and primary election issues

ddc997d

fix license assignment issue

a6aaaaf

heartbeat calculation bugfix and log output and notification improvement

96c2672

remove unused debugging log messages

64695fa

JaydenLiang requested a review from jamie-pate November 4, 2021 21:05

fixed AWS platformadapter a condition expression error on update

9b203de

JaydenLiang added the minor feature update label Nov 8, 2021

JaydenLiang added this to In progress in heartbeat calculation improvement via automation Nov 8, 2021

This was linked to issues Nov 8, 2021

heartbeat calculation improvement based on heartbeat request sequence and timestamp #84

Closed

improve primary election facilitating the device sync info #85

Closed

Joel-Cripps approved these changes Nov 17, 2021

View reviewed changes

jamie-pate requested changes Nov 17, 2021

View reviewed changes

jamie-pate requested changes Dec 7, 2021

View reviewed changes

JaydenLiang requested a review from jamie-pate December 7, 2021 22:25

fix some typos and rename primary election weighting functions

971b741

JaydenLiang force-pushed the feat_gh_project_3_heartbeat_calculation_improvement branch from 432a578 to 971b741 Compare December 7, 2021 22:28

jamie-pate approved these changes Dec 7, 2021

View reviewed changes

JaydenLiang merged commit 207e7ff into staging_project_3_heartbeat_calculation_improvement Dec 7, 2021

heartbeat calculation improvement automation moved this from In progress to Done Dec 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat gh project 3 heartbeat calculation improvement #89

Feat gh project 3 heartbeat calculation improvement #89

JaydenLiang commented Sep 24, 2021 •

edited

Loading

jamie-pate Oct 4, 2021

JaydenLiang Oct 22, 2021

jamie-pate Oct 4, 2021

JaydenLiang Oct 22, 2021

jamie-pate Oct 4, 2021

JaydenLiang Oct 22, 2021

jamie-pate Oct 4, 2021

JaydenLiang Oct 22, 2021

jamie-pate Oct 4, 2021

JaydenLiang Oct 22, 2021

jamie-pate Oct 4, 2021

JaydenLiang Oct 22, 2021

jamie-pate Oct 4, 2021

JaydenLiang Oct 22, 2021

jamie-pate Oct 4, 2021

JaydenLiang Nov 4, 2021

JaydenLiang commented Nov 8, 2021

jamie-pate Nov 17, 2021

JaydenLiang Dec 7, 2021

jamie-pate Nov 17, 2021

JaydenLiang Dec 7, 2021

JaydenLiang Dec 7, 2021

jamie-pate Dec 7, 2021

JaydenLiang Dec 7, 2021

Feat gh project 3 heartbeat calculation improvement #89

Feat gh project 3 heartbeat calculation improvement #89

Conversation

JaydenLiang commented Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydenLiang commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JaydenLiang commented Sep 24, 2021 •

edited

Loading