Merge pull request #4006 from telefonicaid/hardening/3989_remove_fail…

…ed_status REMOVE status failed in csubs
telefonicaid · Nov 23, 2021 · f0f5bdf · f0f5bdf
2 parents 5ff2bb7 + 91a60df
commit f0f5bdf
Show file tree

Hide file tree

Showing 29 changed files with 241 additions and 239 deletions.
diff --git a/CHANGES_NEXT_RELEASE b/CHANGES_NEXT_RELEASE
@@ -8,5 +8,6 @@
 - Fix: crash when updating attribute with empty JSON object or array (#3995)
 - Fix: csubs cache logic has to ignore fiware-sevice information if -multiservice is not used
 - Fix: subscription throttling was not working with value 1 second
+- Remove: status "failed" in subscriptions (use failsCounter greater than 0 instead)
 - Hardening: refactor PATCH /v2/subscriptions/{subId} logic avoiding over-quering MongoDB and possible (although very rare) crash situations during csub cache (#3701)
 - Upgrade Dockerfile base image from centos8.3.2011 to centos8.4.2105
diff --git a/doc/manuals/admin/database_model.md b/doc/manuals/admin/database_model.md
@@ -306,7 +306,7 @@ Fields:
 -   **format**: the format to use to send notification, possible values are **JSON**
     (meaning JSON notifications in NGSIv1 legacy format), **normalized**, **keyValues** and **values** (the last three used in NGSIv2 format).
 -   **status**: either `active` (for active subscriptions), `inactive` (for inactive subscriptions) or
-    `oneshot` (for [oneshot subscriptions](../user/oneshot_subscription.md)). Note that NGSIv2 API consider additional states (e.g. `failed` or `expired`)
+    `oneshot` (for [oneshot subscriptions](../user/oneshot_subscription.md)). Note that NGSIv2 API consider additional states (e.g. `expired`)
     but they never hit the DB (they are managed by Orion).
 -   **statusLastChange**: last time status was updated (as decimal number, meaning seconds and fractions of seconds).
     This is mainly to be used by the subscription cache update logic (so status in updated in DB from cache only if it is newer).

diff --git a/doc/manuals/user/mqtt_notifications.md b/doc/manuals/user/mqtt_notifications.md
@@ -42,9 +42,7 @@ fields:
   corresponding MQTT broker. Thus, there is no need of providing extra detail.
 
 However, note that `lastSuccess` and `lastFailure` fields (which specify the timestamp of the last
-success/failure) are supported in MQTT subscriptions in the same way than in HTTP subscriptions, along
-with correct management of status field (e.g. if a MQTT subscrition is failing, `status` would be
-`failed`).
+success/failure) are supported in MQTT subscriptions in the same way than in HTTP subscriptions.
 
 ## Custom notifications
 

diff --git a/doc/manuals/user/ngsiv2_implementation_notes.md b/doc/manuals/user/ngsiv2_implementation_notes.md
@@ -22,6 +22,7 @@
 * [`timeout` subscriptions option](#timeout-subscriptions-option)
 * [`lastFailureReason` and `lastSuccessCode` subscriptions fields](#lastfailurereason-and-lastsuccesscode-subscriptions-fields)
 * [`failsCounter` and `maxFailsLimit` subscriptions fields](#failscounter-and-maxfailslimit-subscriptions-fields)
+* [Ambiguous subscription status `failed` not used](#ambiguous-subscription-status-failed-not-used)
 * [`forcedUpdate` option](#forcedupdate-option)
 * [`flowControl` option](#flowcontrol-option)
 * [Registrations](#registrations)
@@ -444,6 +445,31 @@ The following requests can use the flowControl URI param option:
 
 [Top](#top)
 
+## Ambiguous subscription status `failed` not used
+
+NGSIv2 specification describes `failed` value for `status` field in subscriptions:
+
+> `status`: [...] Also, for subscriptions experiencing problems with notifications, the status
+> is set to `failed`. As soon as the notifications start working again, the status is changed back to `active`.
+
+Status `failed` was removed in Orion 3.4.0 due to it is ambiguous:
+
+* `failed` may refer to an active subscription (i.e. a subscription that will trigger notifications
+  upon entity updates) which last notification sent was failed
+* `failed` may refer to an inactive subscription (i.e. a subscription that will not trigger notifications
+  upon entity update) which was active in the past and which last notification sent in the time it was
+  active was failed
+
+In other words, looking to status `failed` is not possible to know if the subscription is currently
+active or inactive.
+
+Thus, `failed` is not used by Orion Context Broker and the status of the subscription always clearly specifies
+if the subscription is `active` (including the variant [`oneshot`](#oneshot-subscriptions)) or
+`inactive` (including the variant `expired`). You can check the value of `failsCounter` in order to know if
+the subscription failed in its last notification or not (i.e. checking that `failsCounter` is greater than 0).
+
+[Top](#top)
+
 ## `forcedUpdate` option
 As extra URI param option to the ones included in the NGSIv2 specification, Orion implements forcedUpdate, 
 than can be used to specify that an update operation have to trigger any matching subscription (and send 

diff --git a/src/lib/apiTypesV2/Subscription.cpp b/src/lib/apiTypesV2/Subscription.cpp
@@ -80,24 +80,7 @@ std::string Subscription::toJson(void)
     jh.addDate("expires", this->expires);
   }
 
-  // Status inactive takes precedence over failed
-  //
-  // FIXME #3989: the if-clause check is somehow artificial. Some alternatives described in the issue
-  //
-  // Moreover, this->notification.lastFailure > this->notification.lastSuccess condition is weak
-  // as two notifications coud be sent in the same second (first one successfu, second one failing)
-  // and the status will end in "active" instead of "failed"
-  //
-  // Remove the sleep 1s in maxfailslimit_full_lifecycle.test once this gets fixed
-  //
-  if ((this->status != "inactive") && (this->notification.lastFailure > 0) && (this->notification.lastFailure > this->notification.lastSuccess))
-  {
-    jh.addString("status", "failed");
-  }
-  else
-  {
-    jh.addString("status", this->status);
-  }
+  jh.addString("status", this->status);
 
   jh.addRaw("subject", this->subject.toJson());
   jh.addRaw("notification", this->notification.toJson(renderFormatToString(this->attrsFormat, true, true)));

diff --git a/...alTest/cases/1048_subscription_cache/get_subscription_before_and_after_cache_refresh.test b/...alTest/cases/1048_subscription_cache/get_subscription_before_and_after_cache_refresh.test
@@ -177,7 +177,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 10
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/...ubscription_cache/get_subscription_before_and_after_cache_refresh_status_in_db_older.test b/...ubscription_cache/get_subscription_before_and_after_cache_refresh_status_in_db_older.test
@@ -162,7 +162,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 10
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/test/functionalTest/cases/1048_subscription_cache/refresh_cache_wins_notif_fail.test b/test/functionalTest/cases/1048_subscription_cache/refresh_cache_wins_notif_fail.test
@@ -171,7 +171,7 @@ Date: REGEX(.*)
             "lastNotification": "2019-01-01T10:10:10.000Z",
             "onlyChangedAttrs": false
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -221,7 +221,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -298,7 +298,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/...ases/1048_subscription_cache/refresh_cache_wins_notif_fail_with_update_in_the_middle.test b/...ases/1048_subscription_cache/refresh_cache_wins_notif_fail_with_update_in_the_middle.test
@@ -190,7 +190,7 @@ Date: REGEX(.*)
             "lastNotification": "2019-01-01T10:10:10.000Z",
             "onlyChangedAttrs": false
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -240,7 +240,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -364,7 +364,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/test/functionalTest/cases/1048_subscription_cache/refresh_db_wins_notif_disabled.test b/test/functionalTest/cases/1048_subscription_cache/refresh_db_wins_notif_disabled.test
@@ -119,7 +119,7 @@ echo
 echo
 
 
-echo "04. GET /v2/subscriptions and see status failed (as cache data is older), timesSent 2, failsCounter 2"
+echo "04. GET /v2/subscriptions and see status active (as cache data is older), timesSent 2, failsCounter 2"
 echo "====================================================================================================="
 orionCurl --url /v2/subscriptions
 echo
@@ -214,7 +214,7 @@ Date: REGEX(.*)
 
 
 
-04. GET /v2/subscriptions and see status failed (as cache data is older), timesSent 2, failsCounter 2
+04. GET /v2/subscriptions and see status active (as cache data is older), timesSent 2, failsCounter 2
 =====================================================================================================
 HTTP/1.1 200 OK
 Content-Length: 436
@@ -239,7 +239,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 2
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -315,7 +315,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 2
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/...ses/1048_subscription_cache/refresh_db_wins_notif_disabled_with_update_in_the_middle.test b/...ses/1048_subscription_cache/refresh_db_wins_notif_disabled_with_update_in_the_middle.test
@@ -121,7 +121,7 @@ echo
 echo
 
 
-echo "04. GET /v2/subscriptions and see status failed (as cache data is older), timesSent 2, failsCounter 2"
+echo "04. GET /v2/subscriptions and see status active (as cache data is older), timesSent 2, failsCounter 2"
 echo "====================================================================================================="
 orionCurl --url /v2/subscriptions
 echo
@@ -233,7 +233,7 @@ Date: REGEX(.*)
 
 
 
-04. GET /v2/subscriptions and see status failed (as cache data is older), timesSent 2, failsCounter 2
+04. GET /v2/subscriptions and see status active (as cache data is older), timesSent 2, failsCounter 2
 =====================================================================================================
 HTTP/1.1 200 OK
 Content-Length: 436
@@ -258,7 +258,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 2
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -379,7 +379,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 2
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/test/functionalTest/cases/1048_subscription_cache/refresh_db_wins_notif_fail.test b/test/functionalTest/cases/1048_subscription_cache/refresh_db_wins_notif_fail.test
@@ -168,7 +168,7 @@ Date: REGEX(.*)
             "lastNotification": "2050-01-01T10:00:00.000Z",
             "onlyChangedAttrs": false
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -218,7 +218,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -295,7 +295,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/...t/cases/1048_subscription_cache/refresh_db_wins_notif_fail_with_update_in_the_middle.test b/...t/cases/1048_subscription_cache/refresh_db_wins_notif_fail_with_update_in_the_middle.test
@@ -187,7 +187,7 @@ Date: REGEX(.*)
             "lastNotification": "2050-01-01T10:00:00.000Z",
             "onlyChangedAttrs": false
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -237,7 +237,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []
@@ -361,7 +361,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": []

diff --git a/test/functionalTest/cases/1126_GET_v2_subscriptions/GET_v2_all_subscriptions.test b/test/functionalTest/cases/1126_GET_v2_subscriptions/GET_v2_all_subscriptions.test
@@ -319,7 +319,7 @@ Date: REGEX(.*)
             "onlyChangedAttrs": false,
             "timesSent": 1
         },
-        "status": "failed",
+        "status": "active",
         "subject": {
             "condition": {
                 "attrs": [