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

Multiple jobs #55

Closed
wants to merge 13 commits into from
Closed

Multiple jobs #55

wants to merge 13 commits into from

Conversation

ace2107
Copy link

@ace2107 ace2107 commented Nov 2, 2018

Solving issue #38
If this looks good the InspectionResponse description can be deleted too

@ace2107 ace2107 requested a review from fridex November 2, 2018 22:31
@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2018
@ghost
Copy link

ghost commented Nov 2, 2018

Build failed.

@ghost
Copy link

ghost commented Nov 2, 2018

Build failed.

@ghost
Copy link

ghost commented Nov 2, 2018

Build succeeded.

Copy link
Contributor

@fridex fridex left a comment

Choose a reason for hiding this comment

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

I would suggest to open PRs per one specific feature - its easier for reviewer as well as for developer to adjust based on review comments.

create_inspect_buildconfig(_OPENSHIFT, inspection_id, dockerfile, specification)
inspection_ids = []
for _ in range(job_count):
inspection_ids.append(_generate_inspection_id())
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we want it like this. Ideally we would like to have one ID per request to make only one build (we don't need to build the same thing multiple times). For this build then there should be executed N jobs. These jobs should be referenced with the same ID due to tractability so we can easily point to specific inspections from Thoth.

This can be accomplish by using labels - we mark a job with a label that is set to inspection_id so we can query OpenShift API for these ids. The actual job names can then be numbered like <inspection_id>-0, <inspection_id>-1, ...

Copy link
Member

@goern goern Nov 3, 2018

Choose a reason for hiding this comment

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

in general, this is a #techdebt and ok for a POC, but we should not forget to redesign this using the Operator pattern

see https://trello.com/c/J2fcclgM

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something here - how will operator be helpful here?

Copy link
Member

Choose a reason for hiding this comment

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

We have a restful interface (CRD), and logic that is managing things like BuildConfig and ImageStream (secondary resources), this logic adjusts parameters based on an observed cluster status (#38), and Builds etc need to be cleaned up after they are not longer in use. Sounds like the perfect description of a Kubernets-native Application, thus pattern to implement this is an Operator. Not saying we need to do it this year, but we should keep it in mind.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if we want it like this. Ideally we would like to have one ID per request to make only one build (we don't need to build the same thing multiple times). For this build then there should be executed N jobs. These jobs should be referenced with the same ID due to tractability so we can easily point to specific inspections from Thoth.

This can be accomplish by using labels - we mark a job with a label that is set to inspection_id so we can query OpenShift API for these ids. The actual job names can then be numbered like <inspection_id>-0, <inspection_id>-1, ...

Havent solved this as of now. Can we merge this as of now as POC ? and then implement as needed

@ghost
Copy link

ghost commented Nov 9, 2018

Build succeeded.

This was referenced Nov 9, 2018
amun/core.py Outdated
@@ -137,23 +134,20 @@ def create_inspect_job(openshift: OpenShift, image_stream_name: str, specificati
template = response['items'][0]

if 'run' in specification:
if 'requests' in specification['run']['requests']:
Copy link
Author

Choose a reason for hiding this comment

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

@fridex
Copy link
Contributor

fridex commented Nov 15, 2018

@ace2107 thanks for the implementation. I would postpone this one a little. For now we are OK with one job submission to see end2end flow. We will definitely need this later, but ideally referenced with one inspect id and one build per inspection type. WDYT?

@ace2107
Copy link
Author

ace2107 commented Nov 15, 2018

Yes, we can have one build and job for now to make sure it is working end to end and later have it with -0,1,2... That will just be a slight change in the code , and can be implemented now too if needed

@ghost
Copy link

ghost commented Nov 16, 2018

Build failed.

  • thoth-coala : RETRY_LIMIT in 1m 14s

@ghost
Copy link

ghost commented Nov 16, 2018

Build failed.

  • thoth-coala : RETRY_LIMIT in 1m 05s

@fridex fridex closed this Jan 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants