CI Director

Merge lp://staging/~sinzui/ci-director/generic-jobs into lp://staging/ci-director

generic-jobs
Merge into trunk

Proposed by Curtis Hovey on 2014-07-09

Status:	Merged
Merged at revision:	113
Proposed branch:	lp://staging/~sinzui/ci-director/generic-jobs
Merge into:	lp://staging/ci-director
Diff against target:	713 lines (+472/-22) 4 files modified cidirector/cidirector.py (+146/-5) cidirector/storage.py (+40/-3) cidirector/tests/test_cidirector.py (+199/-5) cidirector/tests/test_storage.py (+87/-9)
To merge this branch:	bzr merge lp://staging/~sinzui/ci-director/generic-jobs
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Aaron Bentley (community)		2014-07-09	Approve on 2014-07-14
Review via email: mp+226204@code.staging.launchpad.net

Description of the change

This branch introduces a generic Job class called ResourcefulJob. It can be configured with the job it requires to have succeeded, those it conflicts with, and the failure-threshold. You can also pass a group_name to create a subgroup in the state-file's yaml.
  * The get_available_jobs()/config_from_description() parses an INI-like config
    in the job description. We do extra calls to jenkins to get the description.
  * The only difference between maybe_build() and the base class is the job status check
    ...maybe we want to extract the status check to a helper method.
  * I left two comments in ResultJudge.get_candidate_determine_result_jobs() and
    CIDirector.schedule_builds(). I don't know the best way to make the new class
    co-exist with the current jobs. I don't want to create duplicate jobs when I start
    adding configuration to the current jobs.

The StageFile gains to new methods to get and iterate over grouped jobs. These two methods can do the work of all the other specialised methods except for the build-revision (build) job, which I think is too special to replace.
  * These methods only work with the current version. I didn't see a need
    (from my reading of the code) to support older versions. Maybe you know
    of a reason to pass the revision_build?

My hope is to get this branch safe to land. Maybe land a separate branch to solve the XXX problems. I want to add configuration to jobs to convert them to ResourcefulJobs. If config doesn't work. I remove it, fix ci-director, the retry the config. When all the current jobs are converted to ResourcefulJob, I can remove the specific jobs.

Revision history for this message

Curtis Hovey (sinzui) wrote on 2014-07-09:

The make-binaries-trusty-amd64 might looks like this

[ci-director]
group-name: prepare-release
requires: make-source-packages
conflicts: run-unit-tests-trusty-amd64
failure-threshold: 2

The publish-revision job might looks like this

[ci-director]
group-name: prepare-release
requires: make-binaries-trusty-amd64 make-binaries-precise-amd64 make-binaries-utopic-amd64 make-binaries-trusty-arm64 make-binaries-trusty-ppc64 make-binaries-trusty-i386
failure-threshold: 2

Revision history for this message

Aaron Bentley (abentley) wrote on 2014-07-10:

I think there is no guarantee that iter_grouped_jobs will iterate through self.requires. The state file typically only contains information for jobs that have started, so it could skip jobs that are listed in self.requires but have not been started. Am I wrong?

The incorrect failure_thresholds can be addressed in another branch, but they should be addressed soon. Probably the correct fix for the incorrect failure_threshold is to store the value in the state file. But that sort of fix will be tricky, because iter_grouped_jobs / get_prerequisite_jobs should be creating missing jobs, but it won't have the information needed. The expedient solution may be to get the threshold from self.jenkins.get_job_info(job_id).get('description') and config_from_description.

An alternative way of doing this incrementally would be to make e.g. PublicationJob a subclass of ResourcefulJob with a constructor that supplies constant values to ResourcefulJob's constructor.

review: Needs Information

lp://staging/~sinzui/ci-director/generic-jobs updated on 2014-07-11

125. By Curtis Hovey on 2014-07-10: Fix grammar.
126. By Curtis Hovey on 2014-07-10: Save the failure-threshold in the data so that it can be reused in ambiguous situations.
127. By Curtis Hovey on 2014-07-10: Always record the failure_threshold with with data so that can be underastood
by an unopinioned method.
128. By Curtis Hovey on 2014-07-10: Always store the failure_threshol, even when the owner jobs knows the value.
129. By Curtis Hovey on 2014-07-10: Fix grammar.
130. By Curtis Hovey on 2014-07-10: get_prerequisite_jobs() ensures every job is represented.
131. By Curtis Hovey on 2014-07-10: Revise the warning.
132. By Curtis Hovey on 2014-07-11: schedule_builds() will append ResourcefulJobs or replace specialised jobs with them.
Added missing test fix for get_prerequisite_jobs().
133. By Curtis Hovey on 2014-07-11: get_candidate_determine_result_jobs() includes resourcefuljobs.
134. By Curtis Hovey on 2014-07-11: get_grouped_job() doesn't need to set failure_threshold because StateFileJob will.
135. By Curtis Hovey on 2014-07-11: Make description a property of JobInfo to be conistent.

Revision history for this message

Curtis Hovey (sinzui) wrote on 2014-07-11:

I have made a large number of changes, I have inflated the diff.

* I Introduced JobInfo.description to be consistent with other cases that access jenkins job_info
* ResourcefulJob.get_prerequisite_jobs() created a dict of job_ids preset to None,
  then it gets the jobs. If any of the jobs are still None, it logs which are missing.
* ResourcefulJob.maybe_build() will return False is any of the prereq jobs is None.
* ResultJudge.get_candidate_determine_result_jobs() learns the resourceful jobs first
  then only yield a specialised state file job when it is not known to be a resourceful job.
* CIDirector.schedule_builds() replaces (or appends) resourceful jobs to the list of jobs.
* get_grouped_job() now has a default failure_threshold of None.
* StateFileJob.__init__() accepts None as a failure_threshold value. It will try to get the
  value from the data in such a case. if neither the data or the callee set failure_threshold,
  1 is used. The failure-threshold is always saved to the data. This last change caused many
  mechanical changes to other tests.

Revision history for this message

Aaron Bentley (abentley) wrote on 2014-07-14:

This looks reasonable. My only major concern is that StateFile._get_job's failure_threshold is treated as authoritative. Instead, I think we should consider it a default, and make the stored value authoritative.

review: Approve

lp://staging/~sinzui/ci-director/generic-jobs updated on 2014-07-14

136. By Curtis Hovey on 2014-07-14: _get_job default failure_threshold is None. The call sites are already tested.
137. By Curtis Hovey on 2014-07-14: The failure_threshold comes from the data, but when not present the arg from
init is used or 1.
138. By Curtis Hovey on 2014-07-14: Assert the method is called only once.
139. By Curtis Hovey on 2014-07-14: Fix spelling.