Scheduled Confiugurations by amazor · Pull Request #1 · Marvell-switching/sonic-swss

amazor · 2024-03-24T21:32:32Z

Phase two first implementation is complete, but needs debugging since ACL table is not being configured as expected.

This phase implements the ability to add to the CONFIG_DB a TIME_RANGE table. Once this configuration is sent, the schedulermgrd will pick up the changes, and call schedulermgr. This process will parse the interval, and create cron files under /etc/cron.d/ within the swss container. It will also create a default TIME_RANGE_STATUS_TABLE entry with the time range name, and give it the default field-value of "status": disabled.

To configure you may use this template json file:
{ "TIME_RANGE": { "rangeTest": { "start": "*/2 * * * *", "end": "1-59/2 * * * *" }, "runOnceTest": { "start": "20 * * * *", "end": "21 * * * *", "runOnce": "true" } } }
Then call this command on the json file: sonic-cfggen -j <json-file-name> --write-to-db.
This will trigger the code found in this PR.
Note this PR also needs changes from two other PRs in other repos. Since this is just phase 1, i decided to wait until both phases are complete before creating all the other PRs

created schedulermgrd and schedulermgr files

Fixed minor and return task_process_status Added systemd infrastructure functions to reload and update Removed options for schedulermgrd application

- Fixed bug where endtime was used for enable cronjob - Fixed bug where EOL was not added to crontab file, causing cron to not be able to read the file - Removed extra c in writeContabFile() funcname - Removed unnecessary scheduledConfigurationTable in schedulermgr - Changed type of m_stateTimeRangeStatusTable from ProducerStateTable to Table in order for set command to work correctly - Formatted code

Forgot an ending quote when adding a runOnce==true boolean

Switching to ";" allows the configuration to be deleted after a "runOnce" whether or not the configuration changes succeed or not.

matiAlfaro · 2024-04-16T11:56:29Z

+        {
+            // Removing quotes from primitive types to match the expected format
+            string value = item.value().dump();
+            if (value.front() == '"' && value.back() == '"')


extract to utility function, reuse in join

matiAlfaro · 2024-04-16T12:34:01Z

same as schedulermgr.cpp, which one do you want to review?

timerangemgr.cpp is the current name. Need to remove schedulermgr.cpp.

matiAlfaro · 2024-04-16T12:41:27Z

+    }
+
+    // Create systemd files for time range and enable them
+    // TODO sanitize inputs


yes, verify time ranges validity.

Need to strictly validate and sanitize time range since this can be a security vulnerability since we are running cron commands as root (albeit only in swss container).

matiAlfaro · 2024-04-16T12:43:29Z

+    // TODO sanitize inputs
+    if (task_process_status::task_need_retry == createCronjobs(rangeName, start, end, (runOnce == "true")))
+    {
+        return task_process_status::task_need_retry;


what if succeeded in creating one file but failed on the second?

Will implement a rollback mechanism in case of OS and possibly a validation errors.

matiAlfaro · 2024-04-16T12:51:42Z

+using namespace swss;
+
+/* SELECT() function timeout retry time, in millisecond */
+#define SELECT_TIMEOUT 1000


why do we need to "wake up" every second?

The timeout will end up calling the doTask() function again with all the consumables still in the queue. This is usually used for "retrying" tasks that did not succeed the first time, but select will not return because no changes have been made to the DB to trigger it again.

matiAlfaro · 2024-04-16T12:54:43Z

I was expecting somewhere in this file to "receive" events from cron, if not - how does enable/disable (active/inactive) status is updated? by polling?

timerangemgrd does not receive events from cron, it writes the cron files. It receives the time ranges from the user, and cron will activate/deactivate the ranges by writing to the STATE_DB. Scheduledconfigmgrd then receives an event from redis that a change has occured.

Also clean up code, and began validation of cron syntax

- Added 3 new files to help with cron validation & time checking. Leaning to use croncpp.h and removing cronhelper.cpp ands cronhelper.h - Added check that scheduled configuration does not already exist when adding a scheduled configuration - Automatically apply scheduled configuration if time range is currently active - Save scheduled configurations in "unboundConfiguration" hashmap when time range is deleted - Check if scheduled configurations saved in "unboundConfiguration" hashmap should be bound to newly created time range, and then add it to that new time range if applicable - Added if statement to not process time range/scheduled configuration if an invalid field is found in the asssociated table - Implemented DEL operation processing for scheduled configuration and time range deletion - New function "isTimeRangeActive()" in scheduledconfigmgr.cpp to check TIME_RANGE_STATUS table if the time range is active according to the field - New function "is_time_in_range()" that uses the croncpp.h file in order to test if the local time is within the scheduled time range interval -- NOT TESTED - Implemented DEL operation for when time range key is deleted from CONFIG_DB

matiAlfaro · 2024-05-16T14:18:15Z

Wouldn't it be better to have the implementations in .cpp file

matiAlfaro · 2024-05-16T14:27:27Z

+      static const cron_int CRON_MAX_YEARS_DIFF = 4;
+
+#ifdef CRONCPP_IS_CPP17
+      static const inline std::vector<std::string> DAYS = { "SUN", "MON", "TUE", "WED", "THU", "FRI", "SAT" };


Wouldn't it be easier (maybe better) to define DAYS and MONTHS as enums have have replace_ordinals be a bit smarter? What is the benefit of doing it this way?
also, these (and a few more) types/functions can be defined in a "more global" scope, so you won't have to define them twice

matiAlfaro · 2024-05-16T14:29:49Z

+      inline std::tm to_tm(CRONCPP_STRING_VIEW time)
+      {
+         std::tm result;
+#if __cplusplus > 201103L


What is the purpose of these compiler version checks? isn't it simpler to write to the currently supported compiler

matiAlfaro · 2024-05-16T14:40:55Z

+bool is_time_in_range(const cronexpr& startExpr, const cronexpr& endExpr, const std::tm& currentTM) {
+    std::time_t currentTime = mktime(const_cast<tm*>(&currentTM)); // Convert currentTM to time_t
+    time_t previousMinute = currentTime - 60; // Go back one minute to ensure full coverage
+


Doesn't this add risk of getting an "event" for something that you already activate here? do you handle such case?

- Added license to croncpp.h - Removed static qualifier from replace_ordinals() in croncpp.h - Done in order to remove warning saying this function isn't used - change "enabled" and "disabled" to "active" and "inactive" respectively

- Changed DB to use a nested unordered_map instead of a vector of pairs - Surrounded replace_ordinals() in croncpp.h to remove unused fxn warning - Surrounded make_cron() usage with try-catch

Will now correctly add and remove configurations when time range becomes active/inactive. Time range status will be updated to active immediately if the current time is within the newly set time range. Added internal database to keep track of active/inactive scheduled configurations in case of delete.

amazor added 5 commits March 18, 2024 14:19

First Commit

9beea35

created schedulermgrd and schedulermgr files

Fixed bugs and Added Schedulermgr(d) to Makefile

e776f30

Fixed minor and return task_process_status Added systemd infrastructure functions to reload and update Removed options for schedulermgrd application

Remove systemd and use cron instead

175468c

Missed ending single quote for runOnce field

7b860c1

Forgot an ending quote when adding a runOnce==true boolean

amazor requested review from DanielaMurin, VladimirKuk, aviramd, matiAlfaro and shiraez March 24, 2024 21:32

amazor changed the title ~~Time based configurations~~ Time based configurations Phase 1 Mar 25, 2024

aviramd reviewed Mar 25, 2024

View reviewed changes

Comment thread cfgmgr/schedulermgr.cpp Outdated

matiAlfaro reviewed Mar 25, 2024

View reviewed changes

Comment thread cfgmgr/schedulermgrd.cpp

Comment thread cfgmgr/schedulermgr.cpp Outdated

Comment thread cfgmgr/schedulermgr.cpp Outdated

Comment thread cfgmgr/schedulermgrd.cpp

amazor added 2 commits March 26, 2024 13:44

Replace "&&" to ";" for cronjobs

4e26a4f

Switching to ";" allows the configuration to be deleted after a "runOnce" whether or not the configuration changes succeed or not.

Create timebasedconfigmgrd and rename schedulermgrd to timerangemgrd

5f37ccc

amazor changed the title ~~Time based configurations Phase 1~~ Time based configurations Apr 3, 2024

amazor changed the title ~~Time based configurations~~ Scheduled Confiugurations Apr 16, 2024

amazor added the enhancement New feature or request label Apr 16, 2024

Rename timebasedconfigmgrd to scheduledconfigmgrd

4d56f59

matiAlfaro reviewed Apr 16, 2024

View reviewed changes

amazor added 2 commits May 7, 2024 15:45

Add support for removing configuration when inactive

b1a6470

Also clean up code, and began validation of cron syntax

matiAlfaro reviewed May 16, 2024

View reviewed changes

amazor added 6 commits May 19, 2024 15:05

Implemented isTimeInRange() and removed changes added to croncpp.h

6ac24b5

Fixed Most Compiler Errors (Still Does Not Compile)

9314657

- Added license to croncpp.h - Removed static qualifier from replace_ordinals() in croncpp.h - Done in order to remove warning saying this function isn't used - change "enabled" and "disabled" to "active" and "inactive" respectively

Changed Data Structure ScheduledConfig DB (Compiles - not tested)

74f2937

- Changed DB to use a nested unordered_map instead of a vector of pairs - Surrounded replace_ordinals() in croncpp.h to remove unused fxn warning - Surrounded make_cron() usage with try-catch

Implemnted Year Functionality and Removed Seconds from croncpp.h

a17d233

Remove Unecessary Files

92414aa

Uh oh!

Conversation

amazor commented Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

amazor commented Mar 24, 2024 •

edited

Loading