Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

Commit

Permalink
Update aggregate_by_status to return objects instead of arrays, add c…
Browse files Browse the repository at this point in the history 
…heck_resources argument

Summary:
This makes the following changes:

- Moves default CPU and memory usage into the constants file.
- Updates aggregate_by_status to return hashes of info with names, instead of an array. This makes future updates easier, and improves readability.
- Adds check_resources argument that adds cpu and memory aggregate data to the hashes, at the expense of a potentially slower loading time.
- Updates Changes UI to work with new format of endpoint.

We can't land this until:

- test properly, without mocks

Test Plan: wrote tests, but a bit worried the mock test isn't enough to prove this endpoint works.

Reviewers: kylec

Reviewed By: kylec

Subscribers: treaster, changesbot

Tags: #changes_ui

Differential Revision: https://tails.corp.dropbox.com/D223367
  • Loading branch information
Robert Lord committed Aug 31, 2016
1 parent 93a0030 commit d9ca2b7
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 30 deletions.
56 changes: 46 additions & 10 deletions changes/api/jobstep_aggregate_by_status.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@
from changes.api.base import APIView
from changes.constants import Status
from changes.models.jobstep import JobStep
from changes.constants import DEFAULT_CPUS, DEFAULT_MEMORY_MB
from changes.models.jobplan import JobPlan
from flask_restful.reqparse import RequestParser
from flask_restful.types import boolean


def status_name(value, name):
Expand All @@ -18,6 +21,7 @@ class JobStepAggregateByStatusAPIView(APIView):
get_parser = RequestParser()
default_statuses = (Status.pending_allocation, Status.queued, Status.in_progress, Status.allocated)
get_parser.add_argument('status', type=status_name, location='args', default=default_statuses)
get_parser.add_argument('check_resources', type=boolean, location='args', default=False)

def get(self):
"""GET method that returns aggregated data regarding jobsteps.
Expand All @@ -28,6 +32,7 @@ def get(self):
Args (in the form of a query string):
status (Optional[str]): A specific status to look for.
If not specified, will search all statuses.
check_resources (Optional[bool]): If shown, adds cpu and memory data to output.
Returns:
{
Expand All @@ -45,24 +50,55 @@ def get(self):
where [status_data] is:
{
'status value': [ count of jobsteps in status,
oldest jobstep create time,
jobstep_id of oldest ],
'status value': {
'count': count of jobsteps in status,
'created': oldest jobstep create time,
'jobstep_id': jobstep_id of oldest,
'cpus': cpu sum of all jobsteps, only shown if check_resources is set
'mem': memory sum of all jobsteps, only shown if check_resources is set
},
...
}
"""
args = self.get_parser.parse_args()

buildstep_for_job_id = None
default = {
"count": 0,
"created": None,
"jobstep_id": None,
}

if args.check_resources:
# cache of buildsteps, so we hit the db less
buildstep_for_job_id = {}
default.update({
"cpus": 0,
"mem": 0,
})

def process_row(agg, jobstep):
status = jobstep.status.name
count, date_created, jobstep_id = agg.get(status, (0, None, None))
count += 1
current = agg.get(status) or default.copy()
current['count'] += 1

if args.check_resources:
if jobstep.job_id not in buildstep_for_job_id:
buildstep_for_job_id[jobstep.job_id] = JobPlan.get_build_step_for_job(jobstep.job_id)[1]
buildstep = buildstep_for_job_id[jobstep.job_id]
limits = buildstep.get_resource_limits()
req_cpus = limits.get('cpus', DEFAULT_CPUS)
req_mem = limits.get('memory', DEFAULT_MEMORY_MB)

current['cpus'] += req_cpus
current['mem'] += req_mem

# Track the oldest jobstep we've seen.
if date_created is None or jobstep.date_created < date_created:
date_created = jobstep.date_created
jobstep_id = jobstep.id
agg[status] = (count, date_created, jobstep_id)
if current['created'] is None or jobstep.date_created < current['created']:
current['created'] = jobstep.date_created
current['jobstep_id'] = jobstep.id
agg[status] = current

args = self.get_parser.parse_args()
jobsteps = JobStep.query.filter(
JobStep.status.in_(args.status),
)
Expand Down
5 changes: 3 additions & 2 deletions changes/api/jobstep_allocate.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
from changes.models.job import Job
from changes.models.jobplan import JobPlan
from changes.models.jobstep import JobStep
from changes.constants import DEFAULT_CPUS, DEFAULT_MEMORY_MB

# Named tuple for data from the BuildStep used to pick JobSteps to allocate,
# to make sure we don't need to refetch (and risk inconsistency).
Expand Down Expand Up @@ -128,8 +129,8 @@ def get(self):
buildstep_for_job_id[jobstep.job_id] = JobPlan.get_build_step_for_job(jobstep.job_id)[1]
buildstep = buildstep_for_job_id[jobstep.job_id]
limits = buildstep.get_resource_limits()
req_cpus = limits.get('cpus', 4)
req_mem = limits.get('memory', 8 * 1024)
req_cpus = limits.get('cpus', DEFAULT_CPUS)
req_mem = limits.get('memory', DEFAULT_MEMORY_MB)
allocation_cmd = buildstep.get_allocation_command(jobstep)
jobstep_data['project'] = jobstep.project
jobstep_data['resources'] = {
Expand Down
4 changes: 2 additions & 2 deletions changes/constants.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
# if not specified in changes.conf.py
CUSTOM_CSS_FILE = "changes.less"

# Default number of CPUs assigned to an executor
# Default number of CPUs assigned to a job executor
DEFAULT_CPUS = 4

# Default amount of RAM (in MBs) assigned to an executor
# Default amount of RAM (in MBs) assigned to a job executor
DEFAULT_MEMORY_MB = 8 * 1024


Expand Down
94 changes: 79 additions & 15 deletions tests/changes/api/test_jobstep_aggregate_by_status.py
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import datetime

import mock
from urllib import urlencode

from changes.buildsteps.base import BuildStep
from changes.testutils import APITestCase
from changes.constants import Status

Expand All @@ -13,6 +15,68 @@ def get(self, **kwargs):
query_string = '?' + urlencode(kwargs) if kwargs else ''
return self.client.get(self.path + query_string)

@mock.patch('changes.models.jobplan.JobPlan.get_build_step_for_job')
def test_get_with_resource_stats(self, get_build_step_for_job):
mock_buildstep = mock.Mock(spec=BuildStep)
mock_buildstep.get_resource_limits.return_value = {'cpus': 4, 'memory': 1024}
get_build_step_for_job.return_value = (None, mock_buildstep)

project_1 = self.create_project(slug="project_1")
build_1 = self.create_build(project_1)
job_1 = self.create_job(build_1)
jobphase_1 = self.create_jobphase(job_1)
now = datetime.datetime.now()

jobstep1 = self.create_jobstep(
jobphase_1,
status=Status.pending_allocation,
date_created=now,
cluster="cluster_c")
jobstep2 = self.create_jobstep(
jobphase_1,
status=Status.pending_allocation,
date_created=now,
cluster="cluster_c")
raw_resp = self.get(status="pending_allocation", check_resources=True)

now_iso = now.isoformat()
expected_output = {
'jobsteps': {
'by_cluster': {
"cluster_c": {
Status.pending_allocation.name: {
'count': 2,
'created': now_iso,
'jobstep_id': jobstep2.id.get_hex(),
'cpus': 8,
'mem': 2048,
},
},
},
'by_project': {
project_1.slug: {
Status.pending_allocation.name: {
'count': 2,
'created': now_iso,
'jobstep_id': jobstep2.id.get_hex(),
'cpus': 8,
'mem': 2048,
},
},
},
'global': {
Status.pending_allocation.name: {
'count': 2,
'created': now_iso,
'jobstep_id': jobstep2.id.get_hex(),
'cpus': 8,
'mem': 2048,
},
},
}
}
assert self.unserialize(raw_resp) == expected_output

def test_get_with_invalid_status(self):
raw_resp = self.get(status="meow")
assert raw_resp.status_code == 400
Expand Down Expand Up @@ -42,18 +106,18 @@ def test_get_with_specific_status(self):
'by_cluster': {
"cluster_c": {
Status.pending_allocation.name:
[1, now_iso, jobstep_pending_allocation.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_pending_allocation.id.get_hex()},
},
},
'by_project': {
project_1.slug: {
Status.pending_allocation.name:
[1, now_iso, jobstep_pending_allocation.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_pending_allocation.id.get_hex()},
},
},
'global': {
Status.pending_allocation.name:
[1, now_iso, jobstep_pending_allocation.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_pending_allocation.id.get_hex()},
},
}
}
Expand Down Expand Up @@ -113,42 +177,42 @@ def test_get(self):
'by_cluster': {
"cluster_a": {
Status.queued.name:
[1, now_iso, jobstep_queued.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_queued.id.get_hex()},
Status.in_progress.name:
[1, now_iso, jobstep_in_progress.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_in_progress.id.get_hex()},
},
"cluster_c": {
Status.pending_allocation.name:
[1, now_iso, jobstep_pending_allocation.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_pending_allocation.id.get_hex()},
},
"cluster_b": {
Status.allocated.name:
[1, now_iso, jobstep_allocated_.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_allocated_.id.get_hex()},
}
},
'by_project': {
project_1.slug: {
Status.in_progress.name:
[1, now_iso, jobstep_in_progress.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_in_progress.id.get_hex()},
Status.pending_allocation.name:
[1, now_iso, jobstep_pending_allocation.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_pending_allocation.id.get_hex()},
},
project_2.slug: {
Status.queued.name:
[1, now_iso, jobstep_queued.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_queued.id.get_hex()},
Status.allocated.name:
[1, now_iso, jobstep_allocated_.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_allocated_.id.get_hex()},
},
},
'global': {
Status.queued.name:
[1, now_iso, jobstep_queued.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_queued.id.get_hex()},
Status.in_progress.name:
[1, now_iso, jobstep_in_progress.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_in_progress.id.get_hex()},
Status.pending_allocation.name:
[1, now_iso, jobstep_pending_allocation.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_pending_allocation.id.get_hex()},
Status.allocated.name:
[1, now_iso, jobstep_allocated_.id.get_hex()],
{'count': 1, 'created': now_iso, 'jobstep_id': jobstep_allocated_.id.get_hex()},
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion webapp/pages/jobstep_summary_page.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ var GroupedJobstepSummary = React.createClass({
render() {
let rowify = (m) => {
return _.map(m, (val, key) => {
return [key, val[0], <JobstepInfo jobstepID={val[2]} />, <Age created={val[1]} />];
return [key, val['count'], <JobstepInfo jobstepID={val['jobstep_id']} />, <Age created={val['created']} />];
});
};

Expand Down

0 comments on commit d9ca2b7

Please sign in to comment.