Merge ~dashmage/charm-logrotated:bug-2019990/random-time-cronjob into charm-logrotated:master

[Ashley James]

Implement random time functionality for daily cronjob.

This branch contains changes to fix the random time setting for the `update-cron-daily-schedule`. It does this by calculating the total minutes between the start and end time and generating a random number in that range. This is then added to the start time to obtain a random time within the provided time ranges.

It also fixes the unit tests that have been written assuming that the start, end times were directly added to the crontab file.

[Ashley James (dashmage)]

I found out that the existing logic [1] to validate the random time is also incorrect.

For example, "update-cron-daily-schedule=random,08:40,09:20" would be treated as an invalid entry but it is actually correct since end_time > start_time.

I will be fixing that and pushing it along with this MR since it's also related to the random time functionality.

[1]: https://git.launchpad.net/charm-logrotated/tree/src/lib/lib_cron.py#n198

[Eric Chen (eric-chen)]

As we discovered last time, cronhelper.install_cronjob and update_cron_daily_schedule will be triggered daily. Will it change the random time every day? I am thinking we should avoid this situation. What do you think?

review: Needs Information

[Eric Chen (eric-chen)]

About test_random_time. Sorry that I do not have specific answer here but just leave some questions.

1. Should we verify "randominess" here?
2. If yes, should we execute it over 100000+ times to approve it? or how to approve it?
3. If no, mock the randInt function?

Please share your finding and conclusion. thanks!

review: Needs Information

[JamesLin (jneo8)]

Please see my inline comments.

review: Needs Fixing

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:57f55f50274e4760cc11b9e6085ef075ffc8b8f2
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/45/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1030/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/321/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/45//rebuild

review: Approve (continuous-integration)

[Ashley James (dashmage)]

> As we discovered last time, cronhelper.install_cronjob and update_cron_daily_schedule will be triggered daily. Will it change the random time every day? I am thinking we should avoid this situation. What do you think?

- Edited response -
The charm's cronjob will be triggered according to the schedule provided in `logrotate-cronjob-frequency` config option (hourly, daily, weekly, monthly). This cronjob would be triggering CronHelper.install_cronjob and update_cron_daily_schedule. And each time update_cron_daily_schedule is run, this will generate a new random time in case `update-cron-daily-schedule` config option is set with the "random".

In order to resolve this, we could remove the `install_cronjob` function call from the `main` function in lib_cron.py so that the charm's cronjob wouldn't set another random time except for install/config changed hook event or running juju action. As per what I understand, the charm's cronjob would only need to override the logrotate configs at a regular time, and it wouldn't need to install the cronjob as well.

But otherwise as well, even if we leave things as it is, the cron.daily jobs would still run only once a day (at a random time which might change) and I feel it's not a big problem.

---
> About test_random_time. Sorry that I do not have specific answer here but just leave some questions.

1. Should we verify "randominess" here?
2. If yes, should we execute it over 100000+ times to approve it? or how to approve it?
3. If no, mock the randInt function?

So in the unit test for the `get_random_time` method, I'm not actually verifying the "randomness" because we can safely assume that `random.randint` will generate a random number for us. Instead I'm testing whether the random time generated with a variety of inputs is actually within the time range (between start and end time).

For the other unit test `test_cron_daily_schedule` I'm mocking `random.randint`.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:fc8d83446a003d1fd127499f7bc50b1dcfb7452d
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/46/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1031/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/322/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/46//rebuild

review: Approve (continuous-integration)

[JamesLin (jneo8)]

Please see my inline comments.

review: Needs Fixing

[Eric Chen (eric-chen)]

> ---
> > About test_random_time. Sorry that I do not have specific answer here but
> just leave some questions.
>
> 1. Should we verify "randominess" here?
> 2. If yes, should we execute it over 100000+ times to approve it? or how to
> approve it?
> 3. If no, mock the randInt function?
>
>
> So in the unit test for the `get_random_time` method, I'm not actually
> verifying the "randomness" because we can safely assume that `random.randint`
> will generate a random number for us. Instead I'm testing whether the random
> time generated with a variety of inputs is actually within the time range
> (between start and end time).

From my prospective, it's testing the "randomness".
Or May I ask another question from another direction:
After the unit test pass, can we 100% confirm the return value is actually within the time range? I don't think it was been proofed. We only test the "get_random_time" less than 10 times.

[Eric Chen (eric-chen)]

I prefer to remove "install_cronjob" in the charm's cronjob if it doesn't voliate the intention in this issue.
https://bugs.launchpad.net/charm-logrotated/+bug/1832296

Please review the code and confirm the impact again.
Thanks!

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:e2d3b0582663aa800f5b3c7529951dc9603461f9
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/47/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1032/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/323/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/47//rebuild

review: Approve (continuous-integration)

[Ashley James (dashmage)]

> From my prospective, it's testing the "randomness".
Or May I ask another question from another direction:
After the unit test pass, can we 100% confirm the return value is actually within the time range? I don't think it was been proofed. We only test the "get_random_time" less than 10 times.

In the logic that I've written, the random number is generated between 0 and the total number of minutes within the provided time range and added to the start time. We can trust that random.randint will only return a number between 0 and the total minutes within the range. This will 100% return a random time between the start and end time.

But if you're asking about how "random" is the random.randint function that ships with Python -- I believe we wouldn't need to test that and we can assume that the randomness provided by the random module in Python is good enough for our purposes.

I was only testing it with a handful of test cases just for sanity. I can add few more test cases if you'd like but I'm not sure how else we can test the `get_random_time` method. I'm open to any suggestions for this.

---
> I prefer to remove "install_cronjob" in the charm's cronjob if it doesn't voliate the intention in this issue.
https://bugs.launchpad.net/charm-logrotated/+bug/1832296

Thanks for sharing the bug report. After taking some time to review it and thinking about the overall intention for the charm, I agree in removing the "install_cronjob". I've pushed another commit for this and provided ample reasoning as part of the commit message.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:f6611640754c9bc8f7a22362f05fc80c888761ba
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/48/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1033/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/324/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/48//rebuild

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

> In the logic that I've written, the random number is generated between 0 and the
> total number of minutes within the provided time range and added to the start
> time. We can trust that random.randint will only return a number between 0 and
> the total minutes within the range. This will 100% return a random time between
> the start and end time.

When imeplemnting unit test, we should not have assumption how will the function been implemented. Because the implementation may be changed one day but the unit test should still work.

For example, I can pass current unit test very easily by this implementation

+ def get_random_time(self, time_range):
+ # Convert start and end times to integers
+ start_hour, start_minute, end_hour, end_minute = [int(t) for t in time_range]
+ return str(start_hour), str(start_minute)+1

Therefore, I don't agree the unit test of "get_random_time" proof the function work here.

review: Needs Fixing

[Ashley James (dashmage)]

In the latest commit, I've modified the unit test for "get_random_time" so that it executes the function 5 times and captures the output each time in a set. Each output is checked for whether it lies within the provided time range and after that, the set is checked for whether there is atleast 2 distinct elements so we can make sure the "get_random_time" function isn't returning some static output.

True, we can have a better random check with all outputs of the function but that would not be fruitful since we aren't trying to test `random.randint` here but only:
a. The function outputs a time within the provided range
b. The time is random enough for the logrotate charm's purposes

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:cf6dd13f12f5e67e8d09aa542bcb7c813492be03
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/49/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1035/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/325/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/49//rebuild

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

Please check my inline comment.

I also put my thought in summary:

1. unit test of a function should cover the corner case, no matter it is possible or impossible under current program logic. We should clear define the expected behavior under normal situation and abnormal situation.

2. prove "all the possibility values could be generated from the functions" is valuable and the implementation is not too complicated compare to current version

review: Needs Fixing

[Ashley James (dashmage)]

> 1. unit test of a function should cover the corner case, no matter it is possible or impossible under current program logic. We should clear define the expected behavior under normal situation and abnormal situation.

> I suggest to add corner cases. What is the expected behavior under error parameters in this function?

1) start_time = end_time
2) start_time < end_time
3) any wrong format of time_range

All the three mentioned "corner cases" or invalid inputs are addressed in the `validate_cron_daily_schedule_conf` method. If you firmly believe the `get_random_time` function should also check these invalid inputs, then I can add a call to the same validate method at the beginning.

But considering the code logic, this validation is already done beforehand and only then does the code execution reach the `get_random_time` method. So I strongly feel that the unit test for `get_random_time` does not need to have test cases for invalid inputs and there are already other unit tests (test_{valid,invalid}_cron_daily_schedule) which test this validation of the input.

The unit tests for `CronHelper.write_to_crontab` and `CronHelper.update_cron_daily_schedule` also does not have invalid inputs as part of its test cases for the similar reason that the validation happens beforehand in `CronHelper.install_cronjob` and these invalid inputs will be caught already.

---
> 2. prove "all the possibility values could be generated from the functions" is valuable and the implementation is not too complicated compare to current version

I have implemented this.

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:187f902d59bde670459a6b2adbbd3dcc0c0c78df
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/50/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1036/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/326/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/50//rebuild

review: Approve (continuous-integration)

[Eric Chen (eric-chen)]

1.
_check_time_in_range is no longer needed in test_get_random_time. Because the validation of the full set already implies all the values are correct.

2.
How long will test_get_random_time run? If it took too much time, we can reduce the range between start_time and end_time. But if the range is too small, then I prefer to separate it into two unit cases for different purpose.

- validate the return value is valid under different start_time / end_time.
- validate all possible datetime would be generated from the fucntion.

3. The corner case is mandatory for each function. We never know how the code logic would be implemented. In ideal situation, the unit test should be finished before writing the function. I mentioned this concept several times. Therefore, we should avoid to decide the unit test case scope based on the logic in real code.

4.
In get_random_time , it’s okay to skip the parameter validation and error handling. (Even though I think it’s better to have error handling). But we still need to know the expected behavior under invalid parameters and add unit tests.
https://github.com/python/cpython/blob/9a68ff12c3e647a4f8dd935919ae296593770a6b/Lib/test/test_random.py#L502

5.
Thanks for the reminder. Yes, we need the corner case for write_to_crontab and update_cron_daily_schedule too. That is a technical debt that is not covered in this MR.

review: Needs Fixing

[Ashley James (dashmage)]

1. I've removed _check_time_in_range. Since we're already generating and validating the entire set of minutes within the range, it makes sense that the check wouldn't be needed.

2. `test_get_random_time` does not take too much extra time to run. Maybe another extra second or 2. So I feel like the existing test case is OK.

3. Alright, I understand the concept. Will implement this.

4. For `get_random_time`, I've added another unit test to check for the invalid parameters as well now.

[Eric Chen (eric-chen)]

LGTM for the code.

But I just realize these three functions can be staticmethod.

get_random_time
_validate_random_schedule
_valid_timestamp

review: Needs Fixing

[Ashley James (dashmage)]

>
> LGTM for the code.
>
> But I just realize these three functions can be staticmethod.
>
> get_random_time
> _validate_random_schedule
> _valid_timestamp

For some reason, making the methods as staticmethod was breaking many of the unit tests and I wasn't able to find a way to fix it for pytest. I've used classmethods instead. So "self.<method>" is replaced with "self.__class__.<method>" in the regular instance methods when they are calling the class methods. The unit test also does not work if I try to refer to them as "CronHelper.<method>".

[Eric Chen (eric-chen)]

There is no sufficient reason to use classmethod.

review: Needs Fixing

[JamesLin (jneo8)]

> For some reason, making the methods as staticmethod was breaking many of the unit tests and I wasn't able to find a way to fix it for pytest. I've used classmethods instead. So "self.<method>" is replaced with "self.__class__.<method>" in the regular instance methods when they are calling the class methods. The unit test also does not work if I try to refer to them as "CronHelper.<method>".

I believe the major use case of classmethod is not for access the other methods in the same class. I mean it's workable but not that suitable. The purpose of get_random_time is very simple and it's make sense to become a static like method. You can try to find a way to resolve this function dependency issue and fix the unit tests.

review: Needs Fixing

[Ashley James (dashmage)]

I've replaced the class methods with static methods now.

The unit tests were breaking because of the monkeypatching of CronHelper done in conftest.py. Instead of monkeypatching the Class, I'm now having it return the Class itself and this will allow for accessing the static methods of the class. The monkeypatching is unnecessary since we want to call the actual methods of the class anyway and if we want to refer to the static methods through the class name, the monkeypatch will not work.

The static methods were breaking while monkeypatching CronHelper because any calls to the static method, for example,`CronHelper._validate_random_schedule()` in the method being tested returned an error saying "AttributeError: 'function' object has no attribute '_validate_random_schedule'".

[Eric Chen (eric-chen)]

Please check my inline comment for the validation function

review: Needs Fixing

[Ashley James (dashmage)]

Latest changes:

- Modified the arguments being used by `_validate_random_schedule`, `get_random_time`, `_validate_set_schedule` to be more coherent. They all now accept either a single timestamp or two timestamps (for the methods dealing with random) in the format "08:30".

- The `_validate_random_schedule` is not being called again in the `get_random_time` method. Instead, the test for the invalid timestamps for get_random_time are just compared with their expected output which might not be correct according to the code logic. For example, if start_time = end_time say 07:30, according to the code logic it will raise a ValueError. But the test_get_random_time checks only the functionality of the get_random_time method and checks if the output is the expected 07:30 (since there is no other time within the range).

[Eric Chen (eric-chen)]

Overall it's good. Two small things in inline comment

review: Needs Fixing

[Ashley James (dashmage)]

Addressed both inline comments!

[JamesLin (jneo8)]

LGTM now.

review: Approve

[Eric Chen (eric-chen)]

LGTM

review: Approve

[🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack)]

PASSED: Continuous integration, rev:af917de4e101a494ca13550fbe29e9f882cde377
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/51/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/bootstack/job/lp-charm-test-functest/1037/
    None: https://jenkins.canonical.com/bootstack/job/lp-update-mp/327/

Click here to trigger a rebuild:
https://jenkins.canonical.com/bootstack/job/lp-charm-logrotated-ci/51//rebuild

review: Approve (continuous-integration)

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision b5d0cd927b51e2741af3b967b7cae8fd0393f8ef

diff --git a/src/config.yaml b/src/config.yaml
index 2a14f0b..def9295 100644
--- a/src/config.yaml
+++ b/src/config.yaml
@@ -29,13 +29,16 @@ options:
     type: string
     default: 'unset'
     description: |
-      If logrotate-cronjob-frequency is set to 'daily' then this value determines whether
-      to update the cron.daily schedule.
+      This value determines the time schedule with which the cron.daily job in
+      /etc/crontab runs. This is done in order to control the execution time for
+      the `/etc/cron.daily/logrotate` job.
       Valid options:
       'unset': No change to default cron.daily schedule.
       'set,HOUR:MINUTE': cron.daily schedule will be set to timestamp HOUR:MINUTE (24H) daily.
       'random,START_HOUR:START_MINUTE,END_HOUR:END_MINUTE': cron.daily schedule will be set
       to some random value between START_HOUR:START_MINUTE,END_HOUR:END_MINUTE (24H) daily.
+      Note that all cron.daily jobs are affected by this config. They still run once a
+      day but according to the time specified here.
   override:
     type: string
     default: '[]'
diff --git a/src/lib/lib_cron.py b/src/lib/lib_cron.py
index f2383cc..94352d0 100644
--- a/src/lib/lib_cron.py
+++ b/src/lib/lib_cron.py
@@ -1,6 +1,8 @@
 """Cron helper module."""
 import os
+import random
 import re
+from datetime import datetime
 
 from charmhelpers.core import hookenv
 
@@ -70,8 +72,7 @@ class CronHelper:
                 cron_file.write(cron_job)
             os.chmod(cron_file_path, 0o755)
 
-            # update cron.daily schedule if logrotate-cronjob-frequency set to "daily"
-            if self.cronjob_frequency == 1 and self.validate_cron_conf():
+            if self.validate_cron_daily_schedule_conf():
                 self.update_cron_daily_schedule()
         else:
             self.cleanup_etc_config()
@@ -108,11 +109,9 @@ class CronHelper:
 
         elif schedule_type == "random":
             cron_daily_start, _, cron_daily_end = schedule_value.partition(",")
-            cron_end_hour, _, cron_end_minute = cron_daily_end.partition(":")
-            cron_start_hour, _, cron_start_minute = cron_daily_start.partition(":")
-
-            cron_daily_hour = cron_start_hour + "~" + cron_end_hour
-            cron_daily_minute = cron_start_minute + "~" + cron_end_minute
+            cron_daily_hour, cron_daily_minute = CronHelper.get_random_time(
+                cron_daily_start, cron_daily_end
+            )
 
         elif schedule_type == "unset":
             # Revert to default ubuntu/debian values for daily cron job
@@ -126,6 +125,31 @@ class CronHelper:
 
         return cron_daily_timestamp
 
+    @staticmethod
+    def get_random_time(start_time, end_time):
+        """Return a random time between the provided time range.
+
+        start_time and end_time are both timestamps in the format "08:30".
+        A random time between start_time and end_time is returned as
+        two values for the random_hour and random_minute strings.
+        """
+        # Convert start and end times to integers
+        start_hour, start_minute = [int(t) for t in start_time.split(":")]
+        end_hour, end_minute = [int(t) for t in end_time.split(":")]
+
+        # Calculate the total number of minutes between the start and end times
+        total_start_minutes = start_hour * 60 + start_minute
+        total_end_minutes = end_hour * 60 + end_minute
+        total_minutes_range = total_end_minutes - total_start_minutes
+
+        # Generate a random number of minutes within the range
+        random_minutes = random.randint(0, total_minutes_range)
+
+        # Calculate the hour and minute components of the random time
+        random_hour = (total_start_minutes + random_minutes) // 60
+        random_minute = (total_start_minutes + random_minutes) % 60
+        return str(random_hour), str(random_minute)
+
     def write_to_crontab(self, cron_daily_timestamp):
         """Write daily cronjob with provided timestamp to /etc/crontab."""
         cron_pattern = re.compile(r".*\/etc\/cron.daily.*")
@@ -136,7 +160,7 @@ class CronHelper:
         updated_cron_daily = ""
         if cron_daily:
             updated_cron_daily = re.sub(
-                r"\d?\d(~\d\d)?\s\d?\d(~\d\d)?\t",
+                r"\d?\d\s\d?\d\t",
                 cron_daily_timestamp + "\t",
                 cron_daily[0],
             )
@@ -144,8 +168,11 @@ class CronHelper:
             with open(r"/etc/crontab", "w") as crontab:
                 crontab.write(updated_data)
 
-    def validate_cron_conf(self):
-        """Block the unit and exit the hook if there is invalid configuration."""
+    def validate_cron_daily_schedule_conf(self):
+        """Validate configuration for update-cron-daily-schedule.
+
+        Block the unit and exit the hook in case of invalid configuration.
+        """
         try:
             conf = self.cron_daily_schedule
             conf = conf.split(",")
@@ -158,9 +185,9 @@ class CronHelper:
             result = True
             # run additional validation functions
             if operation == "set":
-                result = self._validate_set_schedule(conf)
+                result = CronHelper._validate_set_schedule(conf[1])
             elif operation == "random":
-                result = self._validate_random_schedule(conf)
+                result = CronHelper._validate_random_schedule(conf[1], conf[2])
 
             return result
 
@@ -174,40 +201,58 @@ class CronHelper:
             )
             raise self.InvalidCronConfig(err)
 
-    def _validate_set_schedule(self, conf):
-        """Validate update-cron-daily-schedule when the "set" keyword exists."""
-        cron_daily_time = conf[1].split(":")
-        if not self._valid_timestamp(cron_daily_time):
+    @staticmethod
+    def _validate_set_schedule(cron_daily_time):
+        """Validate update-cron-daily-schedule when the "set" keyword exists.
+
+        cron_daily_time is a time in the format "08:30".
+        """
+        if not CronHelper._valid_timestamp(cron_daily_time):
             raise ValueError(
                 "Invalid value for update-cron-daily-schedule: \
                     {}".format(
-                    conf[1]
+                    cron_daily_time
                 )
             )
         else:
             return True
 
-    def _validate_random_schedule(self, conf):
-        """Validate update-cron-daily-schedule when the "random" keyword exists."""
-        cron_daily_start_time = conf[1].split(":")
-        cron_daily_end_time = conf[2].split(":")
+    @staticmethod
+    def _validate_random_schedule(start_time, end_time):
+        """Validate update-cron-daily-schedule when the "random" keyword exists.
+
+        start_time and end_time are timestamps in the format "08:30".
+        """
         if not (
-            self._valid_timestamp(cron_daily_start_time)
-            and self._valid_timestamp(cron_daily_end_time)
-            and int(cron_daily_start_time[0]) <= int(cron_daily_end_time[0])
-            and int(cron_daily_start_time[1]) <= int(cron_daily_end_time[1])
+            CronHelper._valid_timestamp(start_time)
+            and CronHelper._valid_timestamp(end_time)
         ):
+            raise ValueError("Invalid time provided for random schedule.")
+
+        start_time_datetime = datetime.strptime(start_time, "%H:%M")
+        end_time_datetime = datetime.strptime(end_time, "%H:%M")
+        if start_time_datetime < end_time_datetime:
+            return True
+        elif start_time_datetime == end_time_datetime:
             raise ValueError(
-                "Invalid value for update-cron-daily-schedule: \
-                    {},{}".format(
-                    conf[1], conf[2]
-                )
+                f"Random time range is invalid."
+                f"Start time ({start_time}) and "
+                f"end time ({end_time}) can't be equal."
             )
         else:
-            return True
+            raise ValueError(
+                f"Random time range is invalid."
+                f"Start time ({start_time}) should not be "
+                f"after end time ({end_time})."
+            )
 
-    def _valid_timestamp(self, timestamp):
-        """Validate the timestamp."""
+    @staticmethod
+    def _valid_timestamp(timestamp):
+        """Validate the timestamp.
+
+        timestamp is in the format "08:30".
+        """
+        timestamp = timestamp.split(":")
         return int(timestamp[0]) in range(24) and int(timestamp[1]) in range(60)
 
 
@@ -216,9 +261,7 @@ def main():
     hookenv.log("Executing cron job.", level=hookenv.INFO)
     hookenv.status_set("maintenance", "Executing cron job.")
     cronhelper = CronHelper()
-    cronhelper.read_config()
     cronhelper.update_logrotate_etc()
-    cronhelper.install_cronjob()
     hookenv.log("Cron job completed.", level=hookenv.INFO)
     hookenv.status_set("active", "Unit is ready.")
 
diff --git a/src/tests/unit/conftest.py b/src/tests/unit/conftest.py
index 9eff063..d6bbe2e 100644
--- a/src/tests/unit/conftest.py
+++ b/src/tests/unit/conftest.py
@@ -80,13 +80,8 @@ def logrotate(tmpdir, mock_hookenv_config, mock_charm_dir, monkeypatch):
 
 
 @pytest.fixture
-def cron(monkeypatch):
+def cron():
     """Cron fixture."""
     from lib_cron import CronHelper
 
-    helper = CronHelper
-
-    # Any other functions that load helper will get this version
-    monkeypatch.setattr("lib_cron.CronHelper", lambda: helper)
-
-    return helper
+    return CronHelper
diff --git a/src/tests/unit/test_logrotate.py b/src/tests/unit/test_logrotate.py
index 3af2c13..207661f 100644
--- a/src/tests/unit/test_logrotate.py
+++ b/src/tests/unit/test_logrotate.py
@@ -233,35 +233,98 @@ class TestCronHelper:
         cron_config.cronjob_frequency = 1
         cron_config.cron_daily_schedule = "unset"
 
-        assert cron_config.validate_cron_conf()
+        assert cron_config.validate_cron_daily_schedule_conf()
 
     @pytest.mark.parametrize(
-        ("cron_schedule, exp_pattern"),
+        ("cron_schedule, random_hour, random_minute, exp_pattern"),
         [
-            ("random,06:00,07:50", "00~50 06~07"),
-            ("random,06:00,07:00", "00~00 06~07"),
-            ("random,07:00,07:45", "00~45 07~07"),
-            ("set,08:00", "00 08"),
-            ("unset", "25 6"),
+            ("random,06:00,07:50", "6", "30", "30 6"),
+            ("random,07:00,09:00", "9", "5", "5 9"),
+            ("random,07:10,10:45", "10", "10", "10 10"),
+            ("set,08:00", "0", "0", "00 08"),
+            ("unset", "0", "0", "25 6"),
         ],
     )
-    def test_cron_daily_schedule(self, cron, cron_schedule, exp_pattern, mocker):
+    def test_update_cron_daily_schedule(
+        self, cron, cron_schedule, random_hour, random_minute, exp_pattern, mocker
+    ):
         """Test the validate and update random cron.daily schedule."""
         cron_config = cron()
         cron_config.cronjob_enabled = True
         cron_config.cronjob_frequency = 1
         cron_config.cron_daily_schedule = cron_schedule
 
-        mock_method = mocker.Mock()
-        mocker.patch.object(cron, "write_to_crontab", new=mock_method)
+        mock_write_to_crontab = mocker.Mock()
+        mocker.patch.object(cron, "write_to_crontab", new=mock_write_to_crontab)
+
+        mock_get_random_time = mocker.Mock()
+        mocker.patch.object(cron, "get_random_time", new=mock_get_random_time)
+        mock_get_random_time.return_value = random_hour, random_minute
 
         updated_cron_daily = cron_config.update_cron_daily_schedule()
 
-        assert cron_config.validate_cron_conf()
+        assert cron_config.validate_cron_daily_schedule_conf()
         assert updated_cron_daily.split("\t")[0] == exp_pattern
-        mock_method.assert_called_once_with(exp_pattern)
+        mock_write_to_crontab.assert_called_once_with(exp_pattern)
+
+    @pytest.mark.parametrize(
+        ("start_time", "end_time"),
+        [
+            ("06:30", "09:45"),
+            ("7:42", "12:35"),
+            ("18:10", "21:45"),
+            ("2:8", "4:6"),
+            ("7:30", "7:30"),
+            ("30:25", "32:40"),
+        ],
+    )
+    def test_get_random_time(self, cron, start_time, end_time):
+        """Test random time setting for update-cron-daily-schedule."""
+        cron_config = cron()
 
-    @pytest.mark.parametrize("cron_daily_timestamp", ["00 08", "25 6", "00~50 06~07"])
+        # generate set containing all possible minutes within time_range
+        start_hour, start_minute = [int(t) for t in start_time.split(":")]
+        end_hour, end_minute = [int(t) for t in end_time.split(":")]
+        all_minutes_in_range = set()
+        current_hour = start_hour
+        current_minute = start_minute
+        while current_hour < end_hour or (
+            current_hour == end_hour and current_minute <= end_minute
+        ):
+            all_minutes_in_range.add(f"{current_hour}:{current_minute}")
+            current_minute += 1
+            if current_minute == 60:
+                current_minute = 0
+                current_hour += 1
+
+        # run `get_random_time` multiple times, collect output and
+        # see if we're able to obtain all possible minutes in range.
+        random_time_set = set()
+        for _ in range(5000):
+            random_hour, random_minute = cron_config.get_random_time(
+                start_time, end_time
+            )
+            random_time = f"{random_hour}:{random_minute}"
+            random_time_set.add(random_time)
+
+        assert all_minutes_in_range == random_time_set
+
+    @pytest.mark.parametrize(
+        ("start_time", "end_time"),
+        [
+            ("09:30", "06:45"),  # start_time > end_time
+            ("130:45", "18:125"),  # invalid time
+        ],
+    )
+    def test_invalid_get_random_time(self, cron, start_time, end_time):
+        """Test invalid time range arguments for get_random_time."""
+        cron_config = cron()
+        with pytest.raises(ValueError):
+            cron_config.get_random_time(start_time, end_time)
+
+    @pytest.mark.parametrize(
+        "cron_daily_timestamp", ["00 08", "25 6", "40 18", "5 7", "20 4", "5 35"]
+    )
     def test_write_to_crontab(self, cron, cron_daily_timestamp, mocker):
         """Test function that writes updated data to /etc/crontab."""
         cron_config = cron()
@@ -297,23 +360,48 @@ class TestCronHelper:
     @pytest.mark.parametrize(
         ("cron_schedule"),
         [
+            ("random,06:00,07:00"),
+            ("random,7:50,9:30"),
+            ("random,15:00,19:30"),
+            ("random,08:40,09:20"),
+            ("set,8:00"),
+            ("set,12:10"),
+            ("unset"),
+        ],
+    )
+    def test_valid_cron_daily_schedule(self, cron, cron_schedule):
+        """Test valid configuration for cron.daily schedule."""
+        cron_config = cron()
+        cron_config.cronjob_enabled = True
+        cron_config.cronjob_frequency = 1
+        cron_config.cron_daily_schedule = cron_schedule
+        try:
+            cron_config.validate_cron_daily_schedule_conf()
+        except cron_config.InvalidCronConfig:
+            pytest.fail("InvalidCronConfig should not be raised.")
+
+    @pytest.mark.parametrize(
+        ("cron_schedule"),
+        [
             ("random,07:00,06:50"),
             ("random,07:50,07:00"),
             ("random,59:50,07:00"),
             ("random,07:00,39:00"),
+            ("random,09:20,08:40"),
             ("set,28:00"),
             ("set,02:80"),
+            ("invalid_setting"),
         ],
     )
     def test_invalid_cron_daily_schedule(self, cron, cron_schedule):
-        """Test the validate and update random cron.daily schedule."""
+        """Test invalid configuration for cron.daily schedule."""
         cron_config = cron()
         cron_config.cronjob_enabled = True
         cron_config.cronjob_frequency = 1
         cron_config.cron_daily_schedule = cron_schedule
 
         with pytest.raises(cron_config.InvalidCronConfig) as err:
-            cron_config.validate_cron_conf()
+            cron_config.validate_cron_daily_schedule_conf()
 
         assert err.type == cron_config.InvalidCronConfig
 
@@ -330,6 +418,8 @@ class TestCronHelper:
         mocker.patch("lib_cron.os.getcwd", return_value=mock_charm_dir)
         mock_open = mocker.patch("lib_cron.open")
         mock_handle = mock_open.return_value.__enter__.return_value
+        mock_write_to_crontab = mocker.Mock()
+        mocker.patch.object(cron, "write_to_crontab", new=mock_write_to_crontab)
         expected_files_to_be_removed = [
             "/etc/cron.hourly/charm-logrotate",
             "/etc/cron.daily/charm-logrotate",
@@ -340,6 +430,7 @@ class TestCronHelper:
         cron_config = cron()
         cron_config.cronjob_enabled = True
         cron_config.cronjob_frequency = 2
+        cron_config.cron_daily_schedule = "unset"
         cron_config.install_cronjob()
 
         mock_exists.assert_has_calls(