Merge ~dashmage/charm-logrotated:bug/2017798-update-cron-daily-location into charm-logrotated:master
- Git
- lp:~dashmage/charm-logrotated
- bug/2017798-update-cron-daily-location
- Merge into master
Status: | Superseded |
---|---|
Proposed branch: | ~dashmage/charm-logrotated:bug/2017798-update-cron-daily-location |
Merge into: | charm-logrotated:master |
Diff against target: |
201 lines (+62/-26) 4 files modified
src/lib/lib_cron.py (+19/-14) src/tests/functional/requirements.txt (+1/-0) src/tests/functional/test_logrotate.py (+13/-5) src/tests/unit/test_logrotate.py (+29/-7) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Eric Chen | Needs Fixing | ||
Tianqi Xiao (community) | Needs Information | ||
🤖 prod-jenkaas-bootstack (community) | continuous-integration | Approve | |
Chi Wai CHAN | Approve | ||
Robert Gildein | Approve | ||
BootStack Reviewers | Pending | ||
JamesLin | Pending | ||
Review via email: mp+442506@code.launchpad.net |
This proposal has been superseded by a proposal from 2023-05-19.
Commit message
Change from sudo user to system crontab for cron daily
Description of the change
`update-
Fixes LP#2017798
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Ashley James (dashmage) wrote : | # |
I see one potential problem now - in an upgrade scenario, even after implementing this fix, the sudo user crontab would still exist if cron-update-
I propose that we add a function that checks whether the `/var/spool/
Robert Gildein (rgildein) : | # |
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:6bb452dd5ed
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Ashley James (dashmage) wrote (last edit ): | # |
> Is this really wisely to overwritte system crontab? Why not put new file to /etc/cron.d/<name>?
Even if we add this as a new file to `/etc/cron.
In case we're modifying the time for running cron.daily scripts and want to add it as a separate file in `/etc/cron.d`, we would need to remove the line from `/etc/crontab`. I prefer we make the change directly in `/etc/crontab` itself for changing the time.
Edit: Just to clarify, the `update-
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:8a44ee34e84
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:86032494a23
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:8a44ee34e84
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:86032494a23
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:8a44ee34e84
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Robert Gildein (rgildein) : | # |
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:86032494a23
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:714a28da31c
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:e4c83fa67de
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
FAILED: Continuous integration, rev:e4c83fa67de
https:/
Executed test runs:
FAILURE: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:c73e48e05b5
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Eric Chen (eric-chen) wrote : | # |
I am curious
1. who is the runner when this library being called. (Ubuntu? Juju?) why they have root permission.
2. what's the different between f.flush and f.seek.
3. The logic looks like a magic if we want to copy the temp file. Is there better way to avoid the magic flush() or seek() function? I prefer not to use some implictly ability of flush() or seek() here. I prefer to make it just like a human do here.
#1 create temp file
#2 write data and close
#3 copy file
#4 delete temp file
Ashley James (dashmage) wrote : | # |
> I am curious
>
> 1. who is the runner when this library being called. (Ubuntu? Juju?) why they
> have root permission.
After looking into this, the script is called in cron.daily (or weekly/monthly depending on config) and it runs as root. Here is the relevant part in the code [1]. Which is why the copy command works without "sudo". I propose we can remove the sudo while copying from the tmp file.
```
$ cat /etc/cron.
#!/bin/bash
/usr/bin/sudo /usr/bin/juju-run logrotated/1 "/var/lib/
```
[1]: https:/
> 2. what's the different between f.flush and f.seek.
f.seek() would make the file pointer return to zero so that we can read contents from the start of the file. f.flush() is used to clear the internal buffer and put contents into the file explicitly before f.close() or reaching the end of the context manager (where the file is automatically closed). In this situation we could also use f.flush and push the contents to the tmp file before we read it, I just chose to pick f.seek here.
> 3. The logic looks like a magic if we want to copy the temp file. Is there
> better way to avoid the magic flush() or seek() function? I prefer not to use
> some implictly ability of flush() or seek() here. I prefer to make it just
> like a human do here.
>
> #1 create temp file
> #2 write data and close
> #3 copy file
> #4 delete temp file
In case you suggest to avoid using seek/flush, we can do it this way to make it more like how a human would do it:
```
with open(r"
tmp_
try:
cmd = ["cp", "/tmp/crontab", "/etc/crontab"]
subprocess.
except subprocess.
hookenv.log(
f"Writing to system crontab failed with error: {exception}",
)
raise
os.remove(
```
Eric Chen (eric-chen) : | # |
Eric Chen (eric-chen) wrote : | # |
Based on the description you provided in the document, f.flush() is more reasonable. There is no clue that f.seek() will flush the data into the disk. Anyway, I prefer we handle it like a human way unless there is any specific reason we must copy the file before we close the file handle.
Eric Chen (eric-chen) wrote : | # |
I'm so surprise that we will execute /var/lib/
- event handler under src/reactive
- juju actions under src/actions
Could you check the reason why we need to design like this?
BTW, update_
src/actions -> update_cronjob -> cron.install_
The runner of these two behaviors may be different. Could you also confirm it?
1. from cron.daily
2. from juju actions
Eric Chen (eric-chen) wrote : | # |
Change the status to "Needs Fixing"
Ashley James (dashmage) wrote (last edit ): | # |
> I'm so surprise that we will execute /var/lib/
> logrotated-
> to me. I suppose that every library should be used during
>
> - event handler under src/reactive
> - juju actions under src/actions
>
> Could you check the reason why we need to design like this?
After going through the code, this is what I could infer ->
The cron_lib.py code should be executed by the cronjob because the charm is used to rotate the logrotate.d files at the regular interval that is determined by the time configured in the cronjob. The event handlers under src/reactive defines install/
In my view - the juju action allows to install the cronjob/ update logrotate files manually by the operator if they don't want to wait for the cronjob trigger time.
======
> BTW, update_
> logrotated, it may be triggered from
>
> src/actions -> update_cronjob -> cron.install_
> update_
>
> The runner of these two behaviors may be different. Could you also confirm it?
>
> 1. from cron.daily
> 2. from juju actions
So even if this functionality is triggered from src/actions (with update_cronjob) or during execution of the install hook event handler or config changed handler, they utilize the same `CronHelper.
cron.daily is set with the `logrotate-
- 53248e0... by Ashley James
-
Fix flaky functional test
- Use tenacity.Retrying while checking for file in
`test_reconfigure_cronjob_ frequency`
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:7bc0ebbcff3
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
🤖 prod-jenkaas-bootstack (prod-jenkaas-bootstack) wrote : | # |
PASSED: Continuous integration, rev:a213ad7957b
https:/
Executed test runs:
SUCCESS: https:/
None: https:/
Click here to trigger a rebuild:
https:/
Tianqi Xiao (txiao) wrote : | # |
Have you considered using a mature library (e.g. python-crontab [1]) to interact with crontab? I'm a bit worried that there are some consequences we have not considered when directly overwriting the cron file.
Eric Chen (eric-chen) wrote : | # |
I cannot fully understand your answer above. I want to know why we need to execute cron_lib.py regularly. But I didn't find the real reason in your explanation.
My major concern is
1. In general case, the library in charm is used under the juju framework. The design in logrotate breaks the common situation and we should know the real reason. Maybe the solution is not to copy the file, we should just avoid this behavior.
2. Sudo is a dangerous command, we should only use it when needed. We should also need to know how the executor gets the permission?
3. subprocess is always my last choice to handle the program. because it implies many assumptions of the environment. So I agree with Tianqi's comment, we should consider the mature library. Even when we copy the file, we should think about how to use shutil.move if possible. (In the previous commit, you didn't use shutil.move when there is no "sudo").
4. "Retry" is also something I don't like a lot. Please write down the reason why we cannot avoid it. If there is a bug that only happen with 30% possibility, it will be concealed by the "Retry" mechanism.
I suggest that you can write a spec to clarify all the things and purpose the proposal again.
Ashley James (dashmage) wrote : | # |
Problem
-------
Currently when the logrotate-
Fixes LP#2017798 [1]
Proposal
--------
For daily cronjobs, create a new cron file in /etc/cron.d where the charm will be able to manage the execution time without messing with the system crontab (/etc/crontab) or the root user’s crontab. It also prevents the charm from running commands with sudo.
I’m proposing to add a new file to /etc/cron.d instead of modifying the cron.daily execution time directly in the system crontab (/etc/crontab) because this would affect the execution of other scripts in cron.daily.
Changes performed by MP
-------
- Use tenacity.retry while checking whether a file exists in the functional tests. This prevents failure in cases of performance issues on the CI server.
- 2 new files are now managed by the charm --> `cronjob_
- In the `update_
- Renames the function to something more appropriate (`create_
- Replaces updating root user crontab with creating a new cron file in /etc/cron.d for daily cronjob
- Replace dangling `open` function calls with a context manager in `install_cronjob`.
- `cleanup_
I'm still working on these:
- Fix existing unit tests in order to not write directly to the crontab of the system where it's being run. Mock the required files which are being accessed.
- Re-write unit test for `create_
Other Notes
------------
I couldn’t use the python-crontab [2] library in order to manage the crontab changes since it does not support the random execution time syntax (00~45 08~09). This feature is not supported by the cron package which is used by Ubuntu in the first place [3]. A separate bug report has been raised for this issue [4].
-------------
[1]: https:/
[2]: https:/
[3]: https:/
[4]: https:/
Mert Kirpici (mertkirpici) wrote : | # |
I am a little bit late to the discussion here, but I think I found the reason for the self-installing cronjob design decision in this charm, that Eric, Ray, Ashley and I were wondering at different times.
Taking from the original bug report:
LP #1832296: Create functionality to manage logrotate files
https:/
"""
- As some packages might change the logrotate configurations - a cronjob option needs to be defined, where it will constantly update the configurations with regards to the retention period defined
"""
So it is for making sure, our retention settings are taking precedence if any other package/operator makes changes to the /etc/logrotate.d/ files. While I am not sure that it would be the right place to do this, but i suppose we could move this functionality to the "update-status" hook, since it is fired periodically.
Unmerged commits
- 78a0422... by Ashley James
-
Remove cron daily job from root user crontab.
Currently while modifying cron daily job execution time, it is
updated in the root user's crontab (/var/spool/cron/crontabs/ root).
This leads to a duplicate execution along with the execution of
cron.daily scripts in /etc/crontab.This commit aims to manage the execution of the charms' daily cron job
solely from the system crontab, /etc/crontab. It also fixes the existing
unit test `test_cron_daily_schedule` and mocks the crontab instead of
directly writing to it. - 53248e0... by Ashley James
-
Fix flaky functional test
- Use tenacity.Retrying while checking for file in
`test_reconfigure_cronjob_ frequency`
Preview Diff
1 | diff --git a/src/lib/lib_cron.py b/src/lib/lib_cron.py | |||
2 | index 1f30ddc..a314a55 100644 | |||
3 | --- a/src/lib/lib_cron.py | |||
4 | +++ b/src/lib/lib_cron.py | |||
5 | @@ -1,7 +1,6 @@ | |||
6 | 1 | """Cron helper module.""" | 1 | """Cron helper module.""" |
7 | 2 | import os | 2 | import os |
8 | 3 | import re | 3 | import re |
9 | 4 | import subprocess | ||
10 | 5 | 4 | ||
11 | 6 | from charmhelpers.core import hookenv | 5 | from charmhelpers.core import hookenv |
12 | 7 | 6 | ||
13 | @@ -62,15 +61,14 @@ class CronHelper: | |||
14 | 62 | logrotate_unit = hookenv.local_unit() | 61 | logrotate_unit = hookenv.local_unit() |
15 | 63 | python_venv_path = os.getcwd().replace("charm", "") + ".venv/bin/python3" | 62 | python_venv_path = os.getcwd().replace("charm", "") + ".venv/bin/python3" |
16 | 64 | # upgrade to template if logic increases | 63 | # upgrade to template if logic increases |
17 | 65 | cron_file = open(cron_file_path, "w") | ||
18 | 66 | cron_job = """#!/bin/bash | 64 | cron_job = """#!/bin/bash |
19 | 67 | /usr/bin/sudo /usr/bin/juju-run {} "{} {}" | 65 | /usr/bin/sudo /usr/bin/juju-run {} "{} {}" |
20 | 68 | """.format( | 66 | """.format( |
21 | 69 | logrotate_unit, python_venv_path, cronjob_path | 67 | logrotate_unit, python_venv_path, cronjob_path |
22 | 70 | ) | 68 | ) |
26 | 71 | cron_file.write(cron_job) | 69 | with open(cron_file_path, "w") as cron_file: |
27 | 72 | cron_file.close() | 70 | cron_file.write(cron_job) |
28 | 73 | os.chmod(cron_file_path, 700) | 71 | os.chmod(cron_file_path, 0o744) |
29 | 74 | 72 | ||
30 | 75 | # update cron.daily schedule if logrotate-cronjob-frequency set to "daily" | 73 | # update cron.daily schedule if logrotate-cronjob-frequency set to "daily" |
31 | 76 | if self.cronjob_frequency == 1 and self.validate_cron_conf(): | 74 | if self.cronjob_frequency == 1 and self.validate_cron_conf(): |
32 | @@ -123,7 +121,16 @@ class CronHelper: | |||
33 | 123 | else: | 121 | else: |
34 | 124 | raise RuntimeError("Unknown daily schedule type: {}".format(schedule_type)) | 122 | raise RuntimeError("Unknown daily schedule type: {}".format(schedule_type)) |
35 | 125 | 123 | ||
37 | 126 | cron_daily_timestamp = cron_daily_minute + " " + cron_daily_hour + "\t" | 124 | cron_daily_timestamp = cron_daily_minute + " " + cron_daily_hour |
38 | 125 | self.write_to_crontab(cron_daily_timestamp) | ||
39 | 126 | |||
40 | 127 | return cron_daily_timestamp | ||
41 | 128 | |||
42 | 129 | def write_to_crontab(self, cron_daily_timestamp, target="/etc/crontab"): | ||
43 | 130 | """Write daily cronjob with provided timestamp to target crontab. | ||
44 | 131 | |||
45 | 132 | Default value for target is system crontab (/etc/crontab). | ||
46 | 133 | """ | ||
47 | 127 | cron_pattern = re.compile(r".*\/etc\/cron.daily.*") | 134 | cron_pattern = re.compile(r".*\/etc\/cron.daily.*") |
48 | 128 | with open(r"/etc/crontab", "r") as crontab: | 135 | with open(r"/etc/crontab", "r") as crontab: |
49 | 129 | data = crontab.read() | 136 | data = crontab.read() |
50 | @@ -132,15 +139,13 @@ class CronHelper: | |||
51 | 132 | updated_cron_daily = "" | 139 | updated_cron_daily = "" |
52 | 133 | if cron_daily: | 140 | if cron_daily: |
53 | 134 | updated_cron_daily = re.sub( | 141 | updated_cron_daily = re.sub( |
55 | 135 | r"\d?\d(~\d\d)?\s\d?\d(~\d\d)?\t", cron_daily_timestamp, cron_daily[0] | 142 | r"\d?\d(~\d\d)?\s\d?\d(~\d\d)?\t", |
56 | 143 | cron_daily_timestamp + "\t", | ||
57 | 144 | cron_daily[0], | ||
58 | 136 | ) | 145 | ) |
66 | 137 | data = data.replace(cron_daily[0], updated_cron_daily) | 146 | updated_data = data.replace(cron_daily[0], updated_cron_daily) |
67 | 138 | with open(r"/tmp/crontab", "w") as crontab: | 147 | with open(target, "w") as crontab: |
68 | 139 | crontab.write(data) | 148 | crontab.write(updated_data) |
62 | 140 | |||
63 | 141 | subprocess.check_output(["sudo", "crontab", "-u", "root", "/tmp/crontab"]) | ||
64 | 142 | |||
65 | 143 | return updated_cron_daily | ||
69 | 144 | 149 | ||
70 | 145 | def validate_cron_conf(self): | 150 | def validate_cron_conf(self): |
71 | 146 | """Block the unit and exit the hook if there is invalid configuration.""" | 151 | """Block the unit and exit the hook if there is invalid configuration.""" |
72 | diff --git a/src/tests/functional/requirements.txt b/src/tests/functional/requirements.txt | |||
73 | index 6c4de97..4128a5d 100644 | |||
74 | --- a/src/tests/functional/requirements.txt | |||
75 | +++ b/src/tests/functional/requirements.txt | |||
76 | @@ -7,3 +7,4 @@ mock | |||
77 | 7 | pytest | 7 | pytest |
78 | 8 | pytest-asyncio | 8 | pytest-asyncio |
79 | 9 | requests | 9 | requests |
80 | 10 | tenacity | ||
81 | diff --git a/src/tests/functional/test_logrotate.py b/src/tests/functional/test_logrotate.py | |||
82 | index 0afbb82..7088371 100644 | |||
83 | --- a/src/tests/functional/test_logrotate.py | |||
84 | +++ b/src/tests/functional/test_logrotate.py | |||
85 | @@ -9,6 +9,8 @@ import pytest | |||
86 | 9 | 9 | ||
87 | 10 | import pytest_asyncio | 10 | import pytest_asyncio |
88 | 11 | 11 | ||
89 | 12 | import tenacity | ||
90 | 13 | |||
91 | 12 | pytestmark = pytest.mark.asyncio | 14 | pytestmark = pytest.mark.asyncio |
92 | 13 | SERIES = ["bionic", "focal", "jammy"] | 15 | SERIES = ["bionic", "focal", "jammy"] |
93 | 14 | 16 | ||
94 | @@ -118,10 +120,17 @@ async def test_reconfigure_cronjob_frequency(model, deploy_app, unit, jujutools) | |||
95 | 118 | await model.block_until(lambda: deploy_app.status == "active") | 120 | await model.block_until(lambda: deploy_app.status == "active") |
96 | 119 | config = await deploy_app.get_config() | 121 | config = await deploy_app.get_config() |
97 | 120 | 122 | ||
102 | 121 | result = await jujutools.run_command( | 123 | # Retry because test fails sometimes when cron file isn't ready yet |
103 | 122 | "test -f /etc/cron.weekly/charm-logrotate", unit | 124 | for attempt in tenacity.Retrying( |
104 | 123 | ) | 125 | stop=tenacity.stop_after_attempt(3), |
105 | 124 | weekly_cronjob_exists = result["return-code"] == 0 | 126 | wait=tenacity.wait_exponential(multiplier=1, min=2, max=10), |
106 | 127 | ): | ||
107 | 128 | with attempt: | ||
108 | 129 | result = await jujutools.run_command( | ||
109 | 130 | "test -f /etc/cron.weekly/charm-logrotate", unit | ||
110 | 131 | ) | ||
111 | 132 | weekly_cronjob_exists = result["return-code"] == 0 | ||
112 | 133 | assert weekly_cronjob_exists | ||
113 | 125 | 134 | ||
114 | 126 | result = await jujutools.run_command( | 135 | result = await jujutools.run_command( |
115 | 127 | "test -f /etc/cron.daily/charm-logrotate", unit | 136 | "test -f /etc/cron.daily/charm-logrotate", unit |
116 | @@ -130,7 +139,6 @@ async def test_reconfigure_cronjob_frequency(model, deploy_app, unit, jujutools) | |||
117 | 130 | 139 | ||
118 | 131 | assert config["logrotate-cronjob-frequency"]["value"] == "weekly" | 140 | assert config["logrotate-cronjob-frequency"]["value"] == "weekly" |
119 | 132 | assert not daily_cronjob_exists | 141 | assert not daily_cronjob_exists |
120 | 133 | assert weekly_cronjob_exists | ||
121 | 134 | 142 | ||
122 | 135 | 143 | ||
123 | 136 | async def test_configure_override_01(model, deploy_app, jujutools, unit): | 144 | async def test_configure_override_01(model, deploy_app, jujutools, unit): |
124 | diff --git a/src/tests/unit/test_logrotate.py b/src/tests/unit/test_logrotate.py | |||
125 | index 8d329e5..ee136cd 100644 | |||
126 | --- a/src/tests/unit/test_logrotate.py | |||
127 | +++ b/src/tests/unit/test_logrotate.py | |||
128 | @@ -1,6 +1,7 @@ | |||
129 | 1 | """Main unit test module.""" | 1 | """Main unit test module.""" |
130 | 2 | import json | 2 | import json |
131 | 3 | import os | 3 | import os |
132 | 4 | import re | ||
133 | 4 | from textwrap import dedent | 5 | from textwrap import dedent |
134 | 5 | from unittest import mock | 6 | from unittest import mock |
135 | 6 | 7 | ||
136 | @@ -245,18 +246,40 @@ class TestCronHelper: | |||
137 | 245 | ("unset", "25 6"), | 246 | ("unset", "25 6"), |
138 | 246 | ], | 247 | ], |
139 | 247 | ) | 248 | ) |
141 | 248 | def test_cron_daily_schedule(self, cron, cron_schedule, exp_pattern): | 249 | def test_cron_daily_schedule(self, cron, cron_schedule, exp_pattern, mocker): |
142 | 249 | """Test the validate and update random cron.daily schedule.""" | 250 | """Test the validate and update random cron.daily schedule.""" |
143 | 250 | cron_config = cron() | 251 | cron_config = cron() |
144 | 251 | cron_config.cronjob_enabled = True | 252 | cron_config.cronjob_enabled = True |
145 | 252 | cron_config.cronjob_frequency = 1 | 253 | cron_config.cronjob_frequency = 1 |
146 | 253 | cron_config.cron_daily_schedule = cron_schedule | 254 | cron_config.cron_daily_schedule = cron_schedule |
147 | 254 | 255 | ||
149 | 255 | assert cron_config.validate_cron_conf() | 256 | mock_method = mocker.Mock() |
150 | 257 | mocker.patch.object(cron, "write_to_crontab", new=mock_method) | ||
151 | 256 | 258 | ||
152 | 257 | updated_cron_daily = cron_config.update_cron_daily_schedule() | 259 | updated_cron_daily = cron_config.update_cron_daily_schedule() |
153 | 258 | 260 | ||
154 | 261 | assert cron_config.validate_cron_conf() | ||
155 | 259 | assert updated_cron_daily.split("\t")[0] == exp_pattern | 262 | assert updated_cron_daily.split("\t")[0] == exp_pattern |
156 | 263 | mock_method.assert_called_once_with(exp_pattern) | ||
157 | 264 | |||
158 | 265 | @pytest.mark.parametrize("cron_daily_timestamp", ["00 08", "25 6", "00~50 06~07"]) | ||
159 | 266 | def test_write_to_crontab(self, cron, cron_daily_timestamp, tmp_path): | ||
160 | 267 | """Test function that writes updated data to crontab file.""" | ||
161 | 268 | cron_config = cron() | ||
162 | 269 | tmp_cronjob = tmp_path / "cron_file" | ||
163 | 270 | |||
164 | 271 | # write updated content into temp crontab file | ||
165 | 272 | cron_config.write_to_crontab(cron_daily_timestamp, target=tmp_cronjob) | ||
166 | 273 | |||
167 | 274 | with open(tmp_cronjob) as crontab: | ||
168 | 275 | data = crontab.read() | ||
169 | 276 | |||
170 | 277 | # get cron daily job from crontab data | ||
171 | 278 | cron_pattern = re.compile(r".*\/etc\/cron.daily.*") | ||
172 | 279 | cron_daily = cron_pattern.findall(data) | ||
173 | 280 | |||
174 | 281 | # assert if timestamps are equal | ||
175 | 282 | assert cron_daily[0].split("\t")[0] == cron_daily_timestamp | ||
176 | 260 | 283 | ||
177 | 261 | @pytest.mark.parametrize( | 284 | @pytest.mark.parametrize( |
178 | 262 | ("cron_schedule"), | 285 | ("cron_schedule"), |
179 | @@ -292,9 +315,9 @@ class TestCronHelper: | |||
180 | 292 | return_value=os.path.join(mock_charm_dir, "lib/lib_cron.py"), | 315 | return_value=os.path.join(mock_charm_dir, "lib/lib_cron.py"), |
181 | 293 | ) | 316 | ) |
182 | 294 | mocker.patch("lib_cron.os.getcwd", return_value=mock_charm_dir) | 317 | mocker.patch("lib_cron.os.getcwd", return_value=mock_charm_dir) |
186 | 295 | mock_open = mocker.patch("lib_cron.open") | 318 | mock_open = mocker.mock_open() |
187 | 296 | mock_handle = mock_open.return_value | 319 | mocker.patch("lib_cron.open", mock_open) |
188 | 297 | 320 | mock_handle = mock_open.return_value.__enter__.return_value | |
189 | 298 | expected_files_to_be_removed = [ | 321 | expected_files_to_be_removed = [ |
190 | 299 | "/etc/cron.hourly/charm-logrotate", | 322 | "/etc/cron.hourly/charm-logrotate", |
191 | 300 | "/etc/cron.daily/charm-logrotate", | 323 | "/etc/cron.daily/charm-logrotate", |
192 | @@ -322,8 +345,7 @@ class TestCronHelper: | |||
193 | 322 | """ # noqa | 345 | """ # noqa |
194 | 323 | ) | 346 | ) |
195 | 324 | ) | 347 | ) |
198 | 325 | mock_handle.close.assert_called_once() | 348 | mock_chmod.assert_called_once_with("/etc/cron.weekly/charm-logrotate", 0o744) |
197 | 326 | mock_chmod.assert_called_once_with("/etc/cron.weekly/charm-logrotate", 700) | ||
199 | 327 | 349 | ||
200 | 328 | def test_install_cronjob_removes_etc_config_when_cronjob_disabled( | 350 | def test_install_cronjob_removes_etc_config_when_cronjob_disabled( |
201 | 329 | self, cron, mocker | 351 | self, cron, mocker |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.