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

Add SurveyJob Support in Foreman #724

Merged
merged 16 commits into from
Oct 31, 2018
Merged

Add SurveyJob Support in Foreman #724

merged 16 commits into from
Oct 31, 2018

Conversation

Miserlou
Copy link
Contributor

@Miserlou Miserlou commented Oct 9, 2018

Issue Number

#715

Purpose/Implementation Notes

Adds SurveyJob Support in Foreman. Pretty simple implementation, uses the existing pattern.

Types of changes

  • New feature (non-breaking change which adds functionality)

Functional tests

New test

@ghost ghost assigned Miserlou Oct 9, 2018
kurtwheeler
kurtwheeler reviewed Oct 9, 2018
View reviewed changes
foreman/data_refinery_foreman/foreman/main.py Outdated
nomad_client = Nomad(nomad_host, port=int(nomad_port), timeout=5)
if dispatch:
nomad_response = nomad_client.job.dispatch_job("SURVEYOR", meta={"JOB_NAME": "SURVEYOR",
"JOB_ID": str(job.id)})
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should switch surveyor jobs over to be expecting these meta values, however they are not currently what is accepted by surveyor jobs. They just need an accession.

kurtwheeler
kurtwheeler suggested changes Oct 9, 2018
View reviewed changes
Copy link
Contributor

@kurtwheeler kurtwheeler left a comment

Choose a reason for hiding this comment

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

Besides the one comment I left, I think there's a couple things that need to be done still. The first is that this only deals with SURVEYOR jobs and doesn't do much for SURVEYOR_DISPATCHER jobs. The other is adding an interrupt handler to both types of jobs so if a deploy interrupts them they will mark themselves as failed and decrement their num_retries.

@ghost ghost added the in progress label Oct 9, 2018
@Miserlou
Copy link
Contributor Author

Can you take another look at this? Has support for RAM increasing on surveyors now.

kurtwheeler
kurtwheeler suggested changes Oct 23, 2018
View reviewed changes
Copy link
Contributor

@kurtwheeler kurtwheeler left a comment

Choose a reason for hiding this comment

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

There's a few more changes to make but this looks pretty good. Besides the unit test you added here, have you tested this?

foreman/data_refinery_foreman/foreman/main.py Outdated
job_type, nomad_job, nomad_host, nomad_port, job=str(new_job.id))
except Exception as e:
logger.exception('Unable to Dispatch Nomad Job.',
job_name=job_type.value,
Copy link
Contributor

Choose a reason for hiding this comment

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

job_type is undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

As is nomad_job

foreman/data_refinery_foreman/surveyor/management/commands/surveyor_dispatcher.py Outdated
except URLNotFoundNomadException:
logger.error("Dispatching Surveyor Nomad job to host %s and port %s failed.",
job_type, nomad_job, nomad_host, nomad_port, accession_code=accession)
SURVEYOR_JOB_NAME, nomad_job, nomad_host, nomad_port, accession_code=accession)
Copy link
Contributor

Choose a reason for hiding this comment

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

nomad_job is undefined.

foreman/data_refinery_foreman/surveyor/management/commands/surveyor_dispatcher.py Outdated
except Exception as e:
logger.exception('Unable to Dispatch Nomad Job.',
job_name=job_type.value,
job_name=SURVEYOR_JOB_NAME,
job_id=str(job.id),
Copy link
Contributor

Choose a reason for hiding this comment

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

job is undefined.

@ghost ghost added the in progress label Oct 25, 2018
@kurtwheeler
Copy link
Contributor

I think dev needs to be pulled into this branch.

@ghost ghost added the in progress label Oct 26, 2018
@ghost ghost added the in progress label Oct 30, 2018
@Miserlou Miserlou merged commit 7f39543 into dev Oct 31, 2018
@kurtwheeler
Copy link
Contributor

From the staging test:

data-refinery-log-group-circleci-staging log-stream-foreman-circleci-staging 2018-11-02 14:08:10,503 i-0425dc317a979341e data_refinery_foreman.foreman.main ERROR [survey_job: 89]: Couldn't query Nomad about SurveyJob Job.
data-refinery-log-group-circleci-staging log-stream-foreman-circleci-staging Traceback (most recent call last):
data-refinery-log-group-circleci-staging log-stream-foreman-circleci-staging   File "/home/user/data_refinery_foreman/foreman/main.py", line 518, in retry_hung_survey_jobs
data-refinery-log-group-circleci-staging log-stream-foreman-circleci-staging     job_status = nomad_client.job.get_job(job.nomad_job_id)["Status"]
data-refinery-log-group-circleci-staging log-stream-foreman-circleci-staging AttributeError: 'SurveyJob' object has no attribute 'nomad_job_id'
data-refinery-log-group-circleci-staging log-stream-foreman-circleci-staging 2018-11-02 14:08:10,503 i-0425dc317a979341e data_refinery_foreman.foreman.main ERROR [survey_job: 90]: Couldn't query Nomad about SurveyJob Job.

You never ran half the code in this PR.... dude, you gotta start testing your stuff better

This was referenced Nov 2, 2018
Merged
Merged
kurtwheeler pushed a commit that referenced this pull request Jan 10, 2019
Merge pull request #724 from AlexsLemonade/miserlou/715
9901936 
Add SurveyJob Support in Foreman
@wvauclain wvauclain deleted the miserlou/715 branch July 11, 2019 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kurtwheeler kurtwheeler kurtwheeler approved these changes

Assignees

@Miserlou Miserlou

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Miserlou @kurtwheeler