Allow set readonly attributes in unittest mode for easier testing (sonic-net#273)

Author: kcudnik

meta/sai_meta.cpp

@@ -14,6 +14,86 @@
 #include <map>
 #include <iterator>
 
+static volatile bool unittests_enabled = false;
+
+static std::set<std::string> meta_unittests_set_readonly_set;
+
+void meta_unittests_enable(
+        _In_ bool enable)
+{
+    SWSS_LOG_ENTER();
+
+    unittests_enabled = enable;
+}
+
+bool meta_unittests_enabled()
+{
+    SWSS_LOG_ENTER();
+
+    return unittests_enabled;
+}
+
+sai_status_t meta_unittests_allow_readonly_set_once(
+        _In_ sai_object_type_t object_type,
+        _In_ int32_t attr_id)
+{
+    SWSS_LOG_ENTER();
+
+    if (!unittests_enabled)
+    {
+        SWSS_LOG_NOTICE("unittests are not enabled");
+        return SAI_STATUS_FAILURE;
+    }
+
+    auto *md = sai_metadata_get_attr_metadata(object_type, attr_id);
+
+    if (md == NULL)
+    {
+        SWSS_LOG_ERROR("failed to get metadata for object type %d and attr id %d", object_type, attr_id);
+        return SAI_STATUS_FAILURE;
+    }
+
+    if (!HAS_FLAG_READ_ONLY(md->flags))
+    {
+        SWSS_LOG_ERROR("attribute %s is not marked as READ_ONLY", md->attridname);
+        return SAI_STATUS_FAILURE;
+    }
+
+    meta_unittests_set_readonly_set.insert(md->attridname);
+
+    SWSS_LOG_INFO("enabling SET for readonly attribute: %s", md->attridname);
+
+    return SAI_STATUS_SUCCESS;
+}
+
+static bool meta_unittests_get_and_erase_set_readonly_flag(
+        _In_ const sai_attr_metadata_t& md)
+{
+    SWSS_LOG_ENTER();
+
+    if (!unittests_enabled)
+    {
+        // explicityly  to not produce false alarms
+        SWSS_LOG_NOTICE("unittests are not enabled");
+        return false;
+    }
+
+    const auto &it = meta_unittests_set_readonly_set.find(md.attridname);
+
+    if (it == meta_unittests_set_readonly_set.end())
+    {
+        SWSS_LOG_ERROR("%s is not present in readonly set", md.attridname);
+        return false;
+    }
+
+    SWSS_LOG_INFO("%s is present in readonly set, erasing", md.attridname);
+
+    meta_unittests_set_readonly_set.erase(it);
+
+    return true;
+}
+
+
 // TODO move to metadata utils
 bool is_ipv6_mask_valid(
         _In_ const uint8_t* mask)
@@ -1647,9 +1727,16 @@ sai_status_t meta_generic_validation_set(
 
     if (HAS_FLAG_READ_ONLY(md.flags))
     {
-        META_LOG_ERROR(md, "attr is read only and cannot be modified");
+        if (meta_unittests_get_and_erase_set_readonly_flag(md))
+        {
+            META_LOG_NOTICE(md, "readonly attribute is allowd to be set (unittests enabled)");
+        }
+        else
+        {
+            META_LOG_ERROR(md, "attr is read only and cannot be modified");
 
-        return SAI_STATUS_INVALID_PARAMETER;
+            return SAI_STATUS_INVALID_PARAMETER;
+        }
     }
 
     if (HAS_FLAG_CREATE_ONLY(md.flags))

meta/sai_meta.h

@@ -158,4 +158,62 @@ extern void meta_sai_on_fdb_event(
         _In_ uint32_t count,
         _In_ sai_fdb_event_notification_data_t *data);
 
+// UNIT TESTS HELPERS
+
+/**
+ * @brief Enable unittest globally.
+ *
+ * @param[in] enable If set to true unittests are enabled.
+ */
+void meta_unittests_enable(
+        _In_ bool enable);
+
+/**
+ * @brief Indicates whethre unittests are enabled;
+ */
+bool meta_unittests_enabled();
+
+/**
+ * @bried Allow to perform SET operation on READ_ONLY attribue only once.
+ *
+ * This function relaxes metadata checking on SET operation, it allows to
+ * perform SET api on READ_ONLY attribute only once on specific object type and
+ * specific attribue.
+ *
+ * Once means that SET operation is only relaxed for the very next SET call on
+ * that specific object type and attrirbute id.
+ *
+ * Function is explicitly named ONCE, since it will force test developer to not
+ * forget that SET check is relaxed, and not forget for future unittests.
+ *
+ * Function is provided for more flexible testing using virtual switch.  Since
+ * some of the read only attributes maybe very complex to simulate (for example
+ * resources used by actual asic when adding next hop or next hop group), then
+ * it's easier to write such unittest:
+ *
+ * TestCase:
+ * 1. meta_unittests_allow_readonly_set_once(x,y);
+ * 2. object_x_api->set_attribyte(object_id, attr, foo); // attr.id == y
+ * 3. object_x_api->get_attribute(object_id, 1, attr); // attr.id == y
+ * 4. check if get result is equal to set result.
+ *
+ * On real ASIC, even after allowing SET on read only attribute, actual SET
+ * should fail.
+ *
+ * It can be dangerous to set any readonly attribute to different values since
+ * internal metadata logic maybe using that value and in some cases metadata
+ * database may get out of sync and cause unexpected results in api calls up to
+ * application carash.
+ *
+ * This function is not thread safe.
+ *
+ * @param[in] object_type Object type on which SET will be possible.
+ * @param[in] attr_id Attribute ID on which SET will be possible.
+ *
+ * @return #SAI_STATUS_SUCCESS on success Failure status code on error
+ */
+sai_status_t meta_unittests_allow_readonly_set_once(
+        _In_ sai_object_type_t object_type,
+        _In_ int32_t attr_id);
+
 #endif // __SAI_META_H__

vslib/src/sai_vs_switch_BCM56850.cpp

@@ -1143,6 +1143,13 @@ sai_status_t refresh_read_only_BCM56850(
         return refresh_vlan_member_list(meta, object_id, switch_id);
     }
 
+    if (meta_unittests_enabled())
+    {
+        SWSS_LOG_NOTICE("unittests enabled, SET could be performed on %s, not recalculating", meta->attridname);
+
+        return SAI_STATUS_SUCCESS;
+    }
+
     SWSS_LOG_WARN("need to recalculate RO: %s", meta->attridname);
 
     return SAI_STATUS_NOT_IMPLEMENTED;

vslib/src/sai_vs_switch_MLNX2700.cpp

@@ -1122,6 +1122,13 @@ sai_status_t refresh_read_only_MLNX2700(
         return refresh_vlan_member_list(meta, object_id, switch_id);
     }
 
+    if (meta_unittests_enabled())
+    {
+        SWSS_LOG_NOTICE("unittests enabled, SET could be performed on %s, not recalculating", meta->attridname);
+
+        return SAI_STATUS_SUCCESS;
+    }
+
     SWSS_LOG_WARN("need to recalculate RO: %s", meta->attridname);
 
     return SAI_STATUS_NOT_IMPLEMENTED;

vslib/src/tests.cpp

@@ -137,6 +137,43 @@ void test_ports()
     ASSERT_TRUE(attr.value.objlist.count == expected_ports);
 }
 
+void test_set_readonly_attribute()
+{
+    SWSS_LOG_ENTER();
+
+    sai_attribute_t attr;
+
+    sai_object_id_t switch_id;
+
+    attr.id = SAI_SWITCH_ATTR_INIT_SWITCH;
+    attr.value.booldata = true;
+
+    SUCCESS(sai_metadata_sai_switch_api->create_switch(&switch_id, 1, &attr));
+
+    attr.id = SAI_SWITCH_ATTR_PORT_MAX_MTU;
+    attr.value.u32 = 42;
+
+    ASSERT_TRUE(sai_metadata_sai_switch_api->set_switch_attribute(switch_id, &attr) != SAI_STATUS_SUCCESS);
+
+    meta_unittests_enable(true);
+
+    // allow set on readonly attribute
+    SUCCESS(meta_unittests_allow_readonly_set_once(SAI_OBJECT_TYPE_SWITCH, SAI_SWITCH_ATTR_PORT_MAX_MTU));
+
+    // set on readonly attribute should pass
+    SUCCESS(sai_metadata_sai_switch_api->set_switch_attribute(switch_id, &attr));
+
+    // just scramble value to make sure that GET will succeed
+    attr.value.u32 = 1;
+
+    SUCCESS(sai_metadata_sai_switch_api->get_switch_attribute(switch_id, 1, &attr));
+
+    ASSERT_TRUE(attr.value.u32 == 42);
+
+    // second SET should fail
+    ASSERT_TRUE(sai_metadata_sai_switch_api->set_switch_attribute(switch_id, &attr) != SAI_STATUS_SUCCESS);
+}
+
 int main()
 {
     swss::Logger::getInstance().setMinPrio(swss::Logger::SWSS_DEBUG);
@@ -153,5 +190,7 @@ int main()
 
     test_ports();
 
+    test_set_readonly_attribute();
+
     return 0;
 }