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.

Sign up for GitHub

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

Jump to bottom

[beta] IX-F importer: Make Importer return non-zero when there is an error that Ops should see #825

Closed
ccaputo opened this issue Aug 24, 2020 · 7 comments · Fixed by #841
Closed

[beta] IX-F importer: Make Importer return non-zero when there is an error that Ops should see #825

ccaputo opened this issue Aug 24, 2020 · 7 comments · Fixed by #841
Assignees
@arnoldnipper
Labels
Operations Operations Committee Time:Minor Up to 4 hours
Milestone
2.23

Comments

@ccaputo
Copy link
Contributor

ccaputo commented Aug 24, 2020

As far as I can tell, at present, the IX-F JSON Importer has an exit status of zero even when a particular import fails. Since Ops normally runs the importer wrapped with chronic, errors indicated to stdout or stderr can be missed due to this. Thus a change is needed to make the exit status be non-zero if any import fails or has an error, or for that matter if there are any notices that Ops should become aware of.

As an example, the errors noted at #795 (comment) might have been missed had I not manually run the importer.

The robustness of the Importer should not be changed by this. Ie., it should continue to run as it does now, when a single import fails.

@grizz & @job I have marked this as Ready for Implementation. Please override if I am wrong to do so.
@ccaputo ccaputo added the Operations Operations Committee label Aug 24, 2020
@ccaputo ccaputo added this to the 4 Ready for Implementation milestone Aug 24, 2020
@ccaputo
Copy link
Contributor Author

ccaputo commented Aug 24, 2020

@mcmanuss8 & @grizz: I don't consider this a showstopper, but I do think it should be prioritized for the next beta release.

@ccaputo ccaputo mentioned this issue Aug 27, 2020
Closed
@arnoldnipper arnoldnipper assigned arnoldnipper and unassigned vegu Sep 1, 2020
@vegu vegu added the Time:Minor Up to 4 hours label Sep 3, 2020
@vegu
Copy link
Contributor

vegu commented Sep 10, 2020 •
edited
Loading

Summary

We need to tackle two main issues here

  1. When importer command is done return a non-zero exit code if we had any unhandled error (e.g the email import issue we fixed in [prod] IX-F importer: NameError: name 'EmailMultiAlternatives' is not defined #831) at the end of the import run
  2. Output those errors at the end of the run

I think we can accomplish both by collecting those errors and simply re-raising them at the end of the run serving both purposes of outputting the error information as well as causing the importer command to exit with non-zero return code.

@arnoldnipper
Copy link
Contributor

Where is the error report going to? May I suggest to include @peeringdb/ac and flag the report appropriately?

@ccaputo
Copy link
Contributor Author

ccaputo commented Sep 10, 2020

Where is the error report going to? May I suggest to include @peeringdb/ac and flag the report appropriately?

My intention when I created this issue is that the error report go to stderr or stdout and get handled by Ops. This is not about IX-F JSON errors so much as about the operational functioning of the Importer itself, such as the mailer failing to work due to an issue with the cloud provider. Ops will forward issues to @peeringdb/ac when it makes sense to, but Ops needs to find out immediately when the Importer is having an operational problem.

@ccaputo
Copy link
Contributor Author

ccaputo commented Sep 10, 2020

Where is the error report going to? May I suggest to include @peeringdb/ac and flag the report appropriately?

My intention when I created this issue is that the error report go to stderr or stdout and get handled by Ops. This is not about IX-F JSON errors so much as about the operational functioning of the Importer itself, such as the mailer failing to work due to an issue with the cloud provider. Ops will forward issues to @peeringdb/ac when it makes sense to, but Ops needs to find out immediately when the Importer is having an operational problem.

I have inquired about having DeskPro tickets be created for @peeringdb/ac issues such as bad IX-F JSON data or bad URLs, while Ops gets informed about the more operational stuff. Stay tuned...

@ccaputo
Copy link
Contributor Author

ccaputo commented Sep 10, 2020

@arnoldnipper, can you speak to your thoughts about this ticket with respect to your comment #794 (comment) ? The code to create a DeskPro ticket for an IX-F JSON error is currently commented out at https://github.com/peeringdb/peeringdb/blob/master/peeringdb_server/ixf.py#L1643 due to #794. Thanks.

@arnoldnipper
Copy link
Contributor

If the IX gets informed then there is no need to open a DeskPRO ticket

vegu added a commit that referenced this issue Sep 25, 2020
@vegu
make changes from #825 play nice with changes from #833
72405df
@grizz grizz mentioned this issue Sep 25, 2020
Merged
grizz added a commit that referenced this issue Sep 29, 2020
@grizz @egfrank @vegu
Support 202009 (#841)
1fa46e2 
* Add EmailMultiAlternatives import

* Add strip_tags import

* Add settings imports and new email test

* Add email increment to ixf and tests

* IX-F Importer: suggested update when it should be add + remove #832

* Take email increment out of if-else

* Add max and min speed settings

* Change validation check for models

* new speed validation

* Add basic user command

* Add pdb cleanup users tool

* Add pretty printing for speed

* Add users as a subparser

* add translation override to signals

* Add parser as parent of subparser

* refactor and change test

* Move override to cover single variable

* Add tooltip option for individual checkboxes

* address 'fix me' issue with field_help helper func

* Add zipcode validator and black format

* Make website required input but zipcode dependent on country

* Add net POC requirement to Netixlan serializer

* Website is now blank=False ie required in all forms

* refine error message

* Require email is not blank and add test

* Change error message

* add website and zipcode test, edit zipcode error message

* change placement of tooltip

* add question mark

* Add comment

* Add runtime error logging for ixp member import

* add uncaught error test

* delete two unused methods

* Rename test file and add different tests

* Add missing email imports (reproduces changes in hot_fix_gh_831)

* add resend methods

* Add missing email imports (reproduces changes in hot_fix_gh_831)

* Add pytest-mock to pipfile

* Add resend email mechanism

* Add email resending

* remove failing assertion

* fix for ticket_aged_proposals

* Wrap resending emails in conditional for commit

* Add resend email tests

* fix mail_Debug bug

* Figure out production mailing and resending settings

* Add stale info field

* default IXF_RESEND_FAILED_EMAILS to False
fix issue with sent being set even if email was not sent
fix issue with output stating resending of emails even if it wasnt

* IX-F Preview - shows the consolidated delete operation when it shouldn't (#824)

* black format (v 19.10)

* black formatting

* black formatting

* pipfile relock

* make changes from #825 play nice with changes from #833

* black to pipenv dev packages

Co-authored-by: Elliot Frank <elliot@20c.com>
Co-authored-by: Stefan Pratter <stefan@20c.com>
@vegu vegu mentioned this issue Nov 16, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arnoldnipper arnoldnipper

Labels
Operations Operations Committee Time:Minor Up to 4 hours
Projects
None yet
Milestone
2.23
Development

Successfully merging a pull request may close this issue.

Support 202009 peeringdb/peeringdb
4 participants
@grizz @vegu @ccaputo @arnoldnipper