Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add all expid_dir_path functions to autosubmit.py, add attached files to emails #1997

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 37 additions & 17 deletions autosubmit/notifications/mail_notifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,30 @@
import smtplib
import email.utils
from email.mime.text import MIMEText
from email.mime.multipart import MIMEMultipart
from email.mime.application import MIMEApplication
from log.log import Log
from pathlib import Path
from autosubmitconfigparser.config.basicconfig import BasicConfig

isimo00 marked this conversation as resolved.
Show resolved Hide resolved
class MailNotifier:
def __init__(self, basic_config):
self.config = basic_config

def notify_experiment_status(self, exp_id,mail_to,platform):
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
message_text = self._generate_message_experiment_status(exp_id, platform)
message = MIMEText(message_text)
message = MIMEMultipart()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to confirm if this is the best option. I remember the discussion, but I didn't have time to ponder about it, sorry.

@mcastril, here we have a few options, basically,

  1. we send a notification as we already do, but include the location of the logs in the VM/hub/somewhere
  2. we attach the complete log as-is into the email body
  3. we compress and then attach the logs (and delete the compressed archive as the original logs will remain anyway) into the email body

I think I am more included towards 1) to simplify or a 1.5) option, where we let users choose whether they want the location or the attachment, but I had seen a comment about doing the option 2), which is implemented here.

WDYT, Miguel?

message['From'] = email.utils.formataddr(('Autosubmit', self.config.MAIL_FROM))
message['Subject'] = f'[Autosubmit] Warning a remote platform is malfunctioning'
message['Subject'] = f'[Autosubmit] Warning: a remote platform is malfunctioning'
message['Date'] = email.utils.formatdate(localtime=True)
message.attach(MIMEText(message_text))

try:
files = [f for f in BasicConfig.expid_aslog_dir(exp_id).glob('*_run.log') if Path(f).is_file()]
self._attach_files(message, files)
except BaseException as e:
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
Log.printlog('An error has occurred while attaching log files to a warning email about remote_platforms ', 6011)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we fail to attach any file, we just print the log? Or should we move this into the for-loop in attach_files? (i.e. log when a file fails, but keep trying the remaining ones?)

And should we just log, or fail the process?

And should we perhaps log but also add something in the email/mime-message, in the notification saying "Here's your notification, but sorry I couldn't attach file ABC.log for reasons"? 😬

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally we'd want to attach as many log files as possible; so omit the ones we can't retrieve (if any) but still attach the rest. Modifying the mail body to explain this would also be nice, i.e. "This email has attached all the log files we could retrieve, there might be some missing".
I can add another log in the attach_files function and write more specific log messages. I'd rather have a new one than moving this one since they can catch different exceptions - for instance, what if the ASLOGS dir doesn't exist? -.

I'm also wondering if we should compress the log files before attaching them, to delay reaching the file attachment limit (what is it?) Overall, attaching the files is a nice touch but the most relevant information for the user is the log path, which is always printed in the mail body.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If any of these logs include things like compilation of a model like IFS with debug info, that might be a very-very-very large file (I've seen >100MB when someone enabled some print+debug in the compilation). Even zipping it could fail to send notifications due to server size limit.

IMHO it would be safer to add location, and if we want to give the file for the user convenience, then maybe a toggle that enables the files to be attached.


for mail in mail_to:
message['To'] = email.utils.formataddr((mail, mail))
try:
Expand All @@ -53,26 +65,34 @@ def notify_status_change(self, exp_id, job_name, prev_status, status, mail_to):
Log.printlog('Trace:{0}\nAn error has occurred while sending a mail for the job {0}'.format(e,job_name), 6011)

def _send_mail(self, mail_from, mail_to, message):
server = smtplib.SMTP(self.config.SMTP_SERVER,timeout=60)
server = smtplib.SMTP(self.config.SMTP_SERVER, timeout=60)
server.sendmail(mail_from, mail_to, message.as_string())
server.quit()

def _attach_files(self, message, files):
for f in files or []:
with open(f, "rb") as file:
part = MIMEApplication(file.read(), Name = Path(f).name)
part['Content-Disposition'] = 'attachment; filename="%s"' % Path(f).name
message.attach(part)
isimo00 marked this conversation as resolved.
Show resolved Hide resolved


isimo00 marked this conversation as resolved.
Show resolved Hide resolved
@staticmethod
def _generate_message_text(exp_id, job_name, prev_status, status):
return f'Autosubmit notification\n' \
f'-------------------------\n\n' \
f'Experiment id: {str(exp_id)} \n\n' \
+ f'Job name: {str(job_name)} \n\n' \
f'The job status has changed from: {str(prev_status)} to {str(status)} \n\n\n\n\n' \
f'INFO: This message was auto generated by Autosubmit, '\
f'remember that you can disable these messages on Autosubmit config file. \n'
return f'''Autosubmit notification\n
-------------------------\n\n
Experiment id: {str(exp_id)} \n\n
Job name: {str(job_name)} \n\n
The job status has changed from: {str(prev_status)} to {str(status)} \n\n\n\n\n
INFO: This message was auto generated by Autosubmit,
remember that you can disable these messages on Autosubmit config file. \n'''

@staticmethod
def _generate_message_experiment_status(exp_id, platform=""):
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
return f'Autosubmit notification: Remote Platform issues\n' \
f'-------------------------\n\n' \
f'Experiment id:{str(exp_id)} \n\n' \
+ f'Platform affected:{str(platform.name)} using as host:{str(platform.host)} \n\n' \
f'[WARN] Autosubmit encountered an issue with an remote_platform.\n It will resume itself, whenever is possible\n If issue persist, you can change the host IP or put multiple hosts in the platform.yml' + '\n\n\n\n\n' \
f'INFO: This message was auto generated by Autosubmit, '\
f'remember that you can disable these messages on Autosubmit config file. \n'
return f'''Autosubmit notification: Remote Platform issues\n-------------------------\n
Experiment id: {str(exp_id)}
Logs and errors: {BasicConfig.expid_aslog_dir(exp_id)}
Attached to this message you will find the related _run.log files.
Platform affected: {str(platform.name)} using as host: {str(platform.host)}\n\n[WARN] Autosubmit encountered an issue with a remote platform.\n It will resume itself, whenever is possible\n If this issue persists, you can change the host IP or put multiple hosts in the platform.yml file' + '\n\n\n\n\nINFO: This message was auto generated by Autosubmit,
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
remember that you can disable these messages on Autosubmit config file.\n'''
27 changes: 15 additions & 12 deletions test/unit/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from shutil import rmtree
from tempfile import TemporaryDirectory
from typing import Any, Dict, Callable, List, Protocol, Optional
import os

from autosubmit.autosubmit import Autosubmit
from autosubmit.platforms.slurmplatform import SlurmPlatform, ParamikoPlatform
Expand All @@ -27,7 +28,6 @@ class AutosubmitExperiment:
status_dir: Path
platform: ParamikoPlatform


isimo00 marked this conversation as resolved.
Show resolved Hide resolved
@pytest.fixture(scope='function')
def autosubmit_exp(autosubmit: Autosubmit, request: pytest.FixtureRequest) -> Callable:
"""Create an instance of ``Autosubmit`` with an experiment."""
Expand All @@ -36,17 +36,21 @@ def autosubmit_exp(autosubmit: Autosubmit, request: pytest.FixtureRequest) -> Ca
tmp_dir = TemporaryDirectory()
tmp_path = Path(tmp_dir.name)


isimo00 marked this conversation as resolved.
Show resolved Hide resolved
def _create_autosubmit_exp(expid: str):
# directories used when searching for logs to cat
root_dir = tmp_path
BasicConfig.LOCAL_ROOT_DIR = str(root_dir)
exp_path = root_dir / expid
exp_tmp_dir = exp_path / BasicConfig.LOCAL_TMP_DIR
aslogs_dir = exp_tmp_dir / BasicConfig.LOCAL_ASLOG_DIR
status_dir = exp_path / 'status'
aslogs_dir.mkdir(parents=True, exist_ok=True)
status_dir.mkdir(parents=True, exist_ok=True)

exp_path = BasicConfig.expid_dir(expid)

# directories used when searching for logs to cat
exp_tmp_dir = BasicConfig.expid_tmp_dir(expid)
aslogs_dir = BasicConfig.expid_aslog_dir(expid)
status_dir =exp_path / 'status'
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
if not os.path.exists(aslogs_dir):
os.makedirs(aslogs_dir)
if not os.path.exists(status_dir):
os.makedirs(status_dir)
isimo00 marked this conversation as resolved.
Show resolved Hide resolved

platform_config = {
"LOCAL_ROOT_DIR": BasicConfig.LOCAL_ROOT_DIR,
"LOCAL_TMP_DIR": str(exp_tmp_dir),
Expand All @@ -59,7 +63,7 @@ def _create_autosubmit_exp(expid: str):
'QUEUING': [],
'FAILED': []
}
submit_platform_script = aslogs_dir / 'submit_local.sh'
submit_platform_script = aslogs_dir.joinpath('submit_local.sh')
submit_platform_script.touch(exist_ok=True)

return AutosubmitExperiment(
Expand Down Expand Up @@ -94,7 +98,7 @@ def autosubmit() -> Autosubmit:
@pytest.fixture(scope='function')
def create_as_conf() -> Callable: # May need to be changed to use the autosubmit_config one
def _create_as_conf(autosubmit_exp: AutosubmitExperiment, yaml_files: List[Path], experiment_data: Dict[str, Any]):
conf_dir = autosubmit_exp.exp_path / 'conf'
conf_dir = autosubmit_exp.exp_path.joinpath('conf')
conf_dir.mkdir(parents=False, exist_ok=False)
basic_config = BasicConfig
parser_factory = YAMLParserFactory()
Expand All @@ -117,7 +121,6 @@ def _create_as_conf(autosubmit_exp: AutosubmitExperiment, yaml_files: List[Path]

return _create_as_conf


class AutosubmitConfigFactory(Protocol): # Copied from the autosubmit config parser, that I believe is a revised one from the create_as_conf

def __call__(self, expid: str, experiment_data: Optional[Dict], *args: Any, **kwargs: Any) -> AutosubmitConfig: ...
Expand Down
46 changes: 28 additions & 18 deletions test/unit/test_catlog.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,33 @@
from pathlib import Path
from tempfile import TemporaryDirectory
from unittest.mock import patch
import pytest
isimo00 marked this conversation as resolved.
Show resolved Hide resolved

from autosubmit.autosubmit import Autosubmit, AutosubmitCritical
from autosubmitconfigparser.config.basicconfig import BasicConfig


isimo00 marked this conversation as resolved.
Show resolved Hide resolved
class TestJob(TestCase):

def setUp(self):
self.autosubmit = Autosubmit()
# directories used when searching for logs to cat
self.original_root_dir = BasicConfig.LOCAL_ROOT_DIR
self.root_dir = TemporaryDirectory()
BasicConfig.LOCAL_ROOT_DIR = self.root_dir.name
self.exp_path = Path(self.root_dir.name, 'a000')
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
self.tmp_dir = self.exp_path / BasicConfig.LOCAL_TMP_DIR
self.aslogs_dir = self.tmp_dir / BasicConfig.LOCAL_ASLOG_DIR
self.status_path = self.exp_path / 'status'
self.aslogs_dir.mkdir(parents=True)
self.status_path.mkdir()

self.exp_path = BasicConfig.expid_dir('a000')
self.tmp_dir = BasicConfig.expid_tmp_dir('a000')
self.log_dir = BasicConfig.expid_log_dir('a000')
self.aslogs_dir = BasicConfig.expid_aslog_dir('a000')

self.autosubmit = Autosubmit()
# directories used when searching for logs to cat

self.status_path = self.exp_path / 'status'
if not self.aslogs_dir.exists():
self.aslogs_dir.mkdir(parents = True, exist_ok = True)
if not self.status_path.exists():
self.status_path.mkdir(parents = True, exist_ok = True)
if not self.log_dir.exists():
self.log_dir.mkdir(parents=True, exist_ok=True)

def tearDown(self) -> None:
BasicConfig.LOCAL_ROOT_DIR = self.original_root_dir
Expand Down Expand Up @@ -55,15 +63,17 @@ def test_is_workflow_not_found(self, Log):
assert Log.info.call_args[0][0] == 'No logs found.'

def test_is_workflow_log_is_dir(self):
log_file_actually_dir = Path(self.aslogs_dir, 'log_run.log')
log_file_actually_dir.mkdir()
log_file_actually_dir = self.aslogs_dir / 'log_run.log'
log_file_actually_dir.mkdir(parents=True)
def _fn():
self.autosubmit.cat_log('a000', 'o', 'c')
self.assertRaises(AutosubmitCritical, _fn)

@patch('subprocess.Popen')
def test_is_workflow_out_cat(self, popen):
log_file = Path(self.aslogs_dir, 'log_run.log')
log_file = self.aslogs_dir / 'log_run.log'
if log_file.is_dir(): # dir is created in previous test
log_file.rmdir()
with open(log_file, 'w') as f:
f.write('as test')
f.flush()
Expand All @@ -75,7 +85,7 @@ def test_is_workflow_out_cat(self, popen):

@patch('subprocess.Popen')
def test_is_workflow_status_tail(self, popen):
log_file = Path(self.status_path, 'a000_anything.txt')
log_file = self.status_path / 'a000_anything.txt'
with open(log_file, 'w') as f:
f.write('as test')
f.flush()
Expand All @@ -93,19 +103,19 @@ def test_is_jobs_not_found(self, Log):
self.autosubmit.cat_log('a000_INI', file=file, mode='c')
assert Log.info.called
assert Log.info.call_args[0][0] == 'No logs found.'

def test_is_jobs_log_is_dir(self):
log_file_actually_dir = Path(self.tmp_dir, 'LOG_a000/a000_INI.20000101.out')
log_file_actually_dir = self.log_dir / 'a000_INI.20000101.out'
log_file_actually_dir.mkdir(parents=True)
def _fn():
self.autosubmit.cat_log('a000_INI', 'o', 'c')
self.assertRaises(AutosubmitCritical, _fn)

@patch('subprocess.Popen')
def test_is_jobs_out_tail(self, popen):
log_dir = self.tmp_dir / 'LOG_a000'
log_dir.mkdir()
log_file = log_dir / 'a000_INI.20200101.out'
log_file = self.log_dir / 'a000_INI.20200101.out'
if log_file.is_dir(): # dir is created in previous test
log_file.rmdir()
with open(log_file, 'w') as f:
f.write('as test')
f.flush()
Expand Down
13 changes: 10 additions & 3 deletions test/unit/test_describe.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,29 +33,35 @@ def test_describe(
if expid not in _EXPIDS:
continue
exp = autosubmit_exp(expid)

isimo00 marked this conversation as resolved.
Show resolved Hide resolved
basic_config = mocker.MagicMock()
# TODO: Whenever autosubmitconfigparser gets released with BasicConfig.expid_dir() and similar functions, the following line and a lot of mocks need to be removed. This line is especially delicate because it "overmocks" the basic_config mock, thus making the last assertion of this file "assert f'Location: {exp.exp_path}' in log_result_output" a dummy assertion. The rest of the test is still useful.
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
basic_config.expid_dir.side_effect = lambda expid: exp.exp_path
config_values = {
'LOCAL_ROOT_DIR': str(exp.exp_path.parent),
'LOCAL_ASLOG_DIR': str(exp.aslogs_dir)
}

for key, value in config_values.items():
basic_config.__setattr__(key, value)

basic_config.get.side_effect = lambda key, default='': config_values.get(key, default)
for basic_config_location in [
'autosubmit.autosubmit.BasicConfig',
'autosubmitconfigparser.config.configcommon.BasicConfig'
]:
# TODO: Better design how ``BasicConfig`` is used/injected/IOC/etc..
mocker.patch(basic_config_location, basic_config)

mocked_get_submitter = mocker.patch.object(Autosubmit, '_get_submitter')
submitter = mocker.MagicMock()

mocked_get_submitter.return_value = submitter
submitter.platforms = [1, 2]

get_experiment_descrip = mocker.patch('autosubmit.autosubmit.get_experiment_descrip')
get_experiment_descrip.return_value = [[f'{expid} description']]

isimo00 marked this conversation as resolved.
Show resolved Hide resolved
create_as_conf(
autosubmit_exp=exp,
yaml_files=[
Expand All @@ -69,6 +75,7 @@ def test_describe(
}
)


isimo00 marked this conversation as resolved.
Show resolved Hide resolved
Autosubmit.describe(
input_experiment_list=input_experiment_list,
get_from_user=get_from_user
Expand All @@ -84,4 +91,4 @@ def test_describe(
]
root_dir = exp.exp_path.parent
for expid in expids:
assert f'Location: {str(root_dir / expid)}' in log_result_output
assert f'Location: {exp.exp_path}' in log_result_output
isimo00 marked this conversation as resolved.
Show resolved Hide resolved
Loading
Loading