Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 11, 2015, 2:13 a.m., Bill Farner wrote: api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 630 https://reviews.apache.org/r/30325/diff/4/?file=859286#file859286line630 s/Key// Done and done. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review71904 --- On Feb. 10, 2015, 12:53 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 10, 2015, 11:57 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, lines 1609-1618 https://reviews.apache.org/r/30325/diff/4/?file=859288#file859288line1609 s/final// Done. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review71878 --- On Feb. 10, 2015, 12:53 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 11, 2015, 11:51 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- Rebased. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift cc2273025aa86ed17da691a143bb5b28226b124e src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 10, 2015, 7:28 p.m., Joshua Cohen wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, lines 1609-1618 https://reviews.apache.org/r/30325/diff/4/?file=859288#file859288line1609 I don't know how you feel about the need for a Supplier, but using Optional#or[1] here might read better? [1] http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Optional.html#or(com.google.common.base.Supplier) I feel like it will overcomplicate things for no good reason here. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review71840 --- On Feb. 10, 2015, 12:53 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 11, 2015, 11:35 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- Bill's comments. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift cc2273025aa86ed17da691a143bb5b28226b124e src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review72059 --- This patch does not apply cleanly on master (7b531e9), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 11, 2015, 11:35 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 11, 2015, 11:35 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift cc2273025aa86ed17da691a143bb5b28226b124e src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 11, 2015, 11:44 p.m., David McLaughlin wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, lines 1596-1603 https://reviews.apache.org/r/30325/diff/4/?file=859288#file859288line1596 It seems strange that the UPDATE_COORDINATOR can pulse the update but cannot take any other action on it. Maybe allow this role to perform all write actions on updates? Thanks for reminding. I was going to address it separately. Created https://issues.apache.org/jira/browse/AURORA-1119 - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review72060 --- On Feb. 11, 2015, 11:35 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 11, 2015, 11:35 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift cc2273025aa86ed17da691a143bb5b28226b124e src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review72071 --- Ship it! Master (b8f71fb) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 11, 2015, 11:51 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 11, 2015, 11:51 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift cc2273025aa86ed17da691a143bb5b28226b124e src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review71904 --- Ship it! api/src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/30325/#comment117831 s/Key// api/src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/30325/#comment117832 s/updateId/id/ - Bill Farner On Feb. 10, 2015, 12:53 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review71840 --- Ship it! (assuming it rebases cleanly without the need for major changes) src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/30325/#comment117708 I don't know how you feel about the need for a Supplier, but using Optional#or[1] here might read better? [1] http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Optional.html#or(com.google.common.base.Supplier) - Joshua Cohen On Feb. 10, 2015, 12:53 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review71730 --- This patch does not apply cleanly on master (68aa285), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Feb. 10, 2015, 12:53 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- CR comments. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 3, 2015, 12:16 a.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 1388
https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1388
You can check whether the primitive field is set, which will
distinguish between default int value and an explicitly set value.
I think it makes sense to reject zero at this layer.
Done.
On Feb. 3, 2015, 12:16 a.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 1535
https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535
This is too restrictive. It means that _only_ the update coordinator
role may call this RPC, and that a user cannot build something to pulse
their own updates.
You should instead do what `isAdmin` does and fall back to the
coordinator role if direct auth fails.
This is an unfortunate state of affairs, and hopefully the move to
shiro dramatically improves all this.
Maxim Khutornenko wrote:
I don't see how it's necessarily better. Pulsing can always be done under
UPDATE_COORDINATOR membership, which is specifically covering heartbeats
only. The isAdmin() requires ROOT that opens up everything else, including
the ability to kill anyone's tasks in the claster. Isn't that more
restrictive in real life? We don't expect regular users having ROOT access,
meaning they will unlikely to get to write their own pulse routine due to
that.
Or maybe you suggesting an approach similar to killTasks(), where an
admin check followed by a role validation? The problem here is that we don't
have a job key to extract the role for authorization. Perhaps we can change
the RPC to accept a job key instead but that would open up for a race we
don't want to see (e.g. late pulse arrives for a wrong update ID). We could
probably get away with both updateId and jobKey in the API to avoid
ambiguity, or perhpas just updateId and role? I am open to suggestions.
Kevin Sweeney wrote:
In Shiro this will look something like
subject.checkPermission(update:resume:mesos:prod:labrat);
With role update_coordinator having:
```
update:*
```
role root having:
```
*
```
and user mesos having:
```
*:*:mesos
```
So they'd all be allowed.
Bill Farner wrote:
Or maybe you suggesting an approach similar to killTasks(), where an
admin check followed by a role validation?
Yeah, this. I'm not asking for update heartbeats to require ROOT.
The problem here is that we don't have a job key to extract the role
for authorization.
We have an association between {{updateId}} and role owning the job,
right?
/**
* Fetches a read-only view of a job update.
*
* @param updateId Update ID to fetch.
* @return A read-only view of job update.
*/
OptionalIJobUpdate fetchJobUpdate(String updateId);
Maxim Khutornenko wrote:
We do. However, it would require accessing the store in order to
authenticate the call. This is a new pattern we have not tried before and it
may potentially interfere with moving to pure AOP auth.
Kevin Sweeney wrote:
Operations scoped to an object owned by a role will not be enforcable
with pure AOP, as many will typically require a storage lookup (usually by
ID) to determine the owner of the object being operated on.
Operations that happen on global like `snapshot:create` will be
enforceable with pure AOP.
Refactored to use UPDATE_COORDINATOR role with a user auth as fallback. The
authorization is matching the provided credentials (SessionKey) against the
operational intent (JobUpdateKey). The validation of JobUpdateKey matching the
actual stored data is deferred to application layer where a missing content
will generate an INVALID_REQUEST response (or JobUpdatePulseStatus.FINISHED as
in the current case).
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/#review70655
---
On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/
---
(Updated Jan. 30, 2015, 5:23 p.m.)
Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.
Bugs: AURORA-1009
https://issues.apache.org/jira/browse/AURORA-1009
Repository: aurora
Description
---
Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin
interface to support AOP
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Feb. 10, 2015, 12:53 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- Updating description. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description (updated) --- Implemented the `pulseJobUpdate` RPC. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java fd4d6908fe7680808cfdee9e78c37506af280068 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java a9966a823e9616d0bf9bd19fd62e632d931ddf0a src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 4bf63921f8aa4be943b2b9a7b0be9fb33f7762db Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 3, 2015, 12:16 a.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 1535
https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535
This is too restrictive. It means that _only_ the update coordinator
role may call this RPC, and that a user cannot build something to pulse
their own updates.
You should instead do what `isAdmin` does and fall back to the
coordinator role if direct auth fails.
This is an unfortunate state of affairs, and hopefully the move to
shiro dramatically improves all this.
Maxim Khutornenko wrote:
I don't see how it's necessarily better. Pulsing can always be done under
UPDATE_COORDINATOR membership, which is specifically covering heartbeats
only. The isAdmin() requires ROOT that opens up everything else, including
the ability to kill anyone's tasks in the claster. Isn't that more
restrictive in real life? We don't expect regular users having ROOT access,
meaning they will unlikely to get to write their own pulse routine due to
that.
Or maybe you suggesting an approach similar to killTasks(), where an
admin check followed by a role validation? The problem here is that we don't
have a job key to extract the role for authorization. Perhaps we can change
the RPC to accept a job key instead but that would open up for a race we
don't want to see (e.g. late pulse arrives for a wrong update ID). We could
probably get away with both updateId and jobKey in the API to avoid
ambiguity, or perhpas just updateId and role? I am open to suggestions.
Kevin Sweeney wrote:
In Shiro this will look something like
subject.checkPermission(update:resume:mesos:prod:labrat);
With role update_coordinator having:
```
update:*
```
role root having:
```
*
```
and user mesos having:
```
*:*:mesos
```
So they'd all be allowed.
Bill Farner wrote:
Or maybe you suggesting an approach similar to killTasks(), where an
admin check followed by a role validation?
Yeah, this. I'm not asking for update heartbeats to require ROOT.
The problem here is that we don't have a job key to extract the role
for authorization.
We have an association between {{updateId}} and role owning the job,
right?
/**
* Fetches a read-only view of a job update.
*
* @param updateId Update ID to fetch.
* @return A read-only view of job update.
*/
OptionalIJobUpdate fetchJobUpdate(String updateId);
We do. However, it would require accessing the store in order to authenticate
the call. This is a new pattern we have not tried before and it may potentially
interfere with moving to pure AOP auth.
- Maxim
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/#review70655
---
On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/
---
(Updated Jan. 30, 2015, 5:23 p.m.)
Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.
Bugs: AURORA-1009
https://issues.apache.org/jira/browse/AURORA-1009
Repository: aurora
Description
---
Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin
interface to support AOP capability validation.
The RB is diffed against https://reviews.apache.org/r/30225/
Diffs
-
api/src/main/thrift/org/apache/aurora/gen/api.thrift
4f603f9e7ed004e53937a45ea8edf7241e15f5cf
src/main/java/org/apache/aurora/auth/CapabilityValidator.java
45ef643ebe57c1517cdae373574331ea302a8b74
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
8c19f3b08135eb5f3098591ebf9931b42a086318
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
03d1fba76c23570c2c4102a48daf5ce035ecaaa3
Diff: https://reviews.apache.org/r/30325/diff/
Testing
---
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 2, 2015, 4:16 p.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 1535
https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535
This is too restrictive. It means that _only_ the update coordinator
role may call this RPC, and that a user cannot build something to pulse
their own updates.
You should instead do what `isAdmin` does and fall back to the
coordinator role if direct auth fails.
This is an unfortunate state of affairs, and hopefully the move to
shiro dramatically improves all this.
Maxim Khutornenko wrote:
I don't see how it's necessarily better. Pulsing can always be done under
UPDATE_COORDINATOR membership, which is specifically covering heartbeats
only. The isAdmin() requires ROOT that opens up everything else, including
the ability to kill anyone's tasks in the claster. Isn't that more
restrictive in real life? We don't expect regular users having ROOT access,
meaning they will unlikely to get to write their own pulse routine due to
that.
Or maybe you suggesting an approach similar to killTasks(), where an
admin check followed by a role validation? The problem here is that we don't
have a job key to extract the role for authorization. Perhaps we can change
the RPC to accept a job key instead but that would open up for a race we
don't want to see (e.g. late pulse arrives for a wrong update ID). We could
probably get away with both updateId and jobKey in the API to avoid
ambiguity, or perhpas just updateId and role? I am open to suggestions.
Kevin Sweeney wrote:
In Shiro this will look something like
subject.checkPermission(update:resume:mesos:prod:labrat);
With role update_coordinator having:
```
update:*
```
role root having:
```
*
```
and user mesos having:
```
*:*:mesos
```
So they'd all be allowed.
Bill Farner wrote:
Or maybe you suggesting an approach similar to killTasks(), where an
admin check followed by a role validation?
Yeah, this. I'm not asking for update heartbeats to require ROOT.
The problem here is that we don't have a job key to extract the role
for authorization.
We have an association between {{updateId}} and role owning the job,
right?
/**
* Fetches a read-only view of a job update.
*
* @param updateId Update ID to fetch.
* @return A read-only view of job update.
*/
OptionalIJobUpdate fetchJobUpdate(String updateId);
Maxim Khutornenko wrote:
We do. However, it would require accessing the store in order to
authenticate the call. This is a new pattern we have not tried before and it
may potentially interfere with moving to pure AOP auth.
Operations scoped to an object owned by a role will not be enforcable with pure
AOP, as many will typically require a storage lookup (usually by ID) to
determine the owner of the object being operated on.
Operations that happen on global like `snapshot:create` will be enforceable
with pure AOP.
- Kevin
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/#review70655
---
On Jan. 30, 2015, 9:23 a.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/
---
(Updated Jan. 30, 2015, 9:23 a.m.)
Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.
Bugs: AURORA-1009
https://issues.apache.org/jira/browse/AURORA-1009
Repository: aurora
Description
---
Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin
interface to support AOP capability validation.
The RB is diffed against https://reviews.apache.org/r/30225/
Diffs
-
api/src/main/thrift/org/apache/aurora/gen/api.thrift
4f603f9e7ed004e53937a45ea8edf7241e15f5cf
src/main/java/org/apache/aurora/auth/CapabilityValidator.java
45ef643ebe57c1517cdae373574331ea302a8b74
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
8c19f3b08135eb5f3098591ebf9931b42a086318
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
03d1fba76c23570c2c4102a48daf5ce035ecaaa3
Diff: https://reviews.apache.org/r/30325/diff/
Testing
---
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 2, 2015, 4:16 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1535 https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535 This is too restrictive. It means that _only_ the update coordinator role may call this RPC, and that a user cannot build something to pulse their own updates. You should instead do what `isAdmin` does and fall back to the coordinator role if direct auth fails. This is an unfortunate state of affairs, and hopefully the move to shiro dramatically improves all this. Maxim Khutornenko wrote: I don't see how it's necessarily better. Pulsing can always be done under UPDATE_COORDINATOR membership, which is specifically covering heartbeats only. The isAdmin() requires ROOT that opens up everything else, including the ability to kill anyone's tasks in the claster. Isn't that more restrictive in real life? We don't expect regular users having ROOT access, meaning they will unlikely to get to write their own pulse routine due to that. Or maybe you suggesting an approach similar to killTasks(), where an admin check followed by a role validation? The problem here is that we don't have a job key to extract the role for authorization. Perhaps we can change the RPC to accept a job key instead but that would open up for a race we don't want to see (e.g. late pulse arrives for a wrong update ID). We could probably get away with both updateId and jobKey in the API to avoid ambiguity, or perhpas just updateId and role? I am open to suggestions. In Shiro this will look something like subject.checkPermission(update:resume:mesos:prod:labrat); With role update_coordinator having: ``` update:* ``` role root having: ``` * ``` and user mesos having: ``` *:*:mesos ``` So they'd all be allowed. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70655 --- On Jan. 30, 2015, 9:23 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 9:23 a.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 3, 2015, 12:16 a.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 1535
https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535
This is too restrictive. It means that _only_ the update coordinator
role may call this RPC, and that a user cannot build something to pulse
their own updates.
You should instead do what `isAdmin` does and fall back to the
coordinator role if direct auth fails.
This is an unfortunate state of affairs, and hopefully the move to
shiro dramatically improves all this.
Maxim Khutornenko wrote:
I don't see how it's necessarily better. Pulsing can always be done under
UPDATE_COORDINATOR membership, which is specifically covering heartbeats
only. The isAdmin() requires ROOT that opens up everything else, including
the ability to kill anyone's tasks in the claster. Isn't that more
restrictive in real life? We don't expect regular users having ROOT access,
meaning they will unlikely to get to write their own pulse routine due to
that.
Or maybe you suggesting an approach similar to killTasks(), where an
admin check followed by a role validation? The problem here is that we don't
have a job key to extract the role for authorization. Perhaps we can change
the RPC to accept a job key instead but that would open up for a race we
don't want to see (e.g. late pulse arrives for a wrong update ID). We could
probably get away with both updateId and jobKey in the API to avoid
ambiguity, or perhpas just updateId and role? I am open to suggestions.
Kevin Sweeney wrote:
In Shiro this will look something like
subject.checkPermission(update:resume:mesos:prod:labrat);
With role update_coordinator having:
```
update:*
```
role root having:
```
*
```
and user mesos having:
```
*:*:mesos
```
So they'd all be allowed.
Or maybe you suggesting an approach similar to killTasks(), where an admin
check followed by a role validation?
Yeah, this. I'm not asking for update heartbeats to require ROOT.
The problem here is that we don't have a job key to extract the role for
authorization.
We have an association between {{updateId}} and role owning the job, right?
/**
* Fetches a read-only view of a job update.
*
* @param updateId Update ID to fetch.
* @return A read-only view of job update.
*/
OptionalIJobUpdate fetchJobUpdate(String updateId);
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/#review70655
---
On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30325/
---
(Updated Jan. 30, 2015, 5:23 p.m.)
Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner.
Bugs: AURORA-1009
https://issues.apache.org/jira/browse/AURORA-1009
Repository: aurora
Description
---
Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin
interface to support AOP capability validation.
The RB is diffed against https://reviews.apache.org/r/30225/
Diffs
-
api/src/main/thrift/org/apache/aurora/gen/api.thrift
4f603f9e7ed004e53937a45ea8edf7241e15f5cf
src/main/java/org/apache/aurora/auth/CapabilityValidator.java
45ef643ebe57c1517cdae373574331ea302a8b74
src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
8c19f3b08135eb5f3098591ebf9931b42a086318
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
03d1fba76c23570c2c4102a48daf5ce035ecaaa3
Diff: https://reviews.apache.org/r/30325/diff/
Testing
---
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70655 --- src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/30325/#comment115972 You can check whether the primitive field is set, which will distinguish between default int value and an explicitly set value. I think it makes sense to reject zero at this layer. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/30325/#comment115976 This is too restrictive. It means that _only_ the update coordinator role may call this RPC, and that a user cannot build something to pulse their own updates. You should instead do what `isAdmin` does and fall back to the coordinator role if direct auth fails. This is an unfortunate state of affairs, and hopefully the move to shiro dramatically improves all this. - Bill Farner On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Feb. 3, 2015, 12:16 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1535 https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1535 This is too restrictive. It means that _only_ the update coordinator role may call this RPC, and that a user cannot build something to pulse their own updates. You should instead do what `isAdmin` does and fall back to the coordinator role if direct auth fails. This is an unfortunate state of affairs, and hopefully the move to shiro dramatically improves all this. I don't see how it's necessarily better. Pulsing can always be done under UPDATE_COORDINATOR membership, which is specifically covering heartbeats only. The isAdmin() requires ROOT that opens up everything else, including the ability to kill anyone's tasks in the claster. Isn't that more restrictive in real life? We don't expect regular users having ROOT access, meaning they will unlikely to get to write their own pulse routine due to that. Or maybe you suggesting an approach similar to killTasks(), where an admin check followed by a role validation? The problem here is that we don't have a job key to extract the role for authorization. Perhaps we can change the RPC to accept a job key instead but that would open up for a race we don't want to see (e.g. late pulse arrives for a wrong update ID). We could probably get away with both updateId and jobKey in the API to avoid ambiguity, or perhpas just updateId and role? I am open to suggestions. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70655 --- On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70379 --- Ship it! Master (4f04a34) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Jan. 30, 2015, 5:34 p.m., David McLaughlin wrote: api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 1126 https://reviews.apache.org/r/30325/diff/3/?file=841926#file841926line1126 Have we considered adding a batch interface, to avoid having to make multiple RPCs for a collection of updates? Not sure how that will work on the caller side given different schedules updates may operate on. Adding a batch API will be quite easy given the need. I don't think it's justified yet. On Jan. 30, 2015, 5:34 p.m., David McLaughlin wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1388 https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1388 This is consistent with the message above, but we should just say 'a positive integer' here. Zero is the default accepted value. By stating non-negative in the error message we identify the acceptable range as zero or a positive integer. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70377 --- On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
On Jan. 30, 2015, 5:34 p.m., David McLaughlin wrote: api/src/main/thrift/org/apache/aurora/gen/api.thrift, line 1126 https://reviews.apache.org/r/30325/diff/3/?file=841926#file841926line1126 Have we considered adding a batch interface, to avoid having to make multiple RPCs for a collection of updates? Maxim Khutornenko wrote: Not sure how that will work on the caller side given different schedules updates may operate on. Adding a batch API will be quite easy given the need. I don't think it's justified yet. Sounds good. On Jan. 30, 2015, 5:34 p.m., David McLaughlin wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 1388 https://reviews.apache.org/r/30325/diff/3/?file=841928#file841928line1388 This is consistent with the message above, but we should just say 'a positive integer' here. Maxim Khutornenko wrote: Zero is the default accepted value. By stating non-negative in the error message we identify the acceptable range as zero or a positive integer. This field should probably have been an optional. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70377 --- On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70383 --- Ship it! Ship It! - David McLaughlin On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- Re-parenting against the right branch. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review70377 --- api/src/main/thrift/org/apache/aurora/gen/api.thrift https://reviews.apache.org/r/30325/#comment115507 Have we considered adding a batch interface, to avoid having to make multiple RPCs for a collection of updates? src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/30325/#comment115508 This is consistent with the message above, but we should just say 'a positive integer' here. - David McLaughlin On Jan. 30, 2015, 5:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:23 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 30, 2015, 5:19 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Changes --- Adding validation for blockIfNoPulsesAfterMs. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs (updated) - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java d3b30d48b76d8d7c64cda006a34f7ed3296526f2 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java a992938d4e12b20f81608be6bbdc24c0a211c3fd src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java 27a5b9026f5ac3b3bdeb32813b10435bc3dab173 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 4c827b183a87b4d97774edbfaa960bd1c3de72a5 src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java 7d0a7438b4a517e5e0d44f4e99aceb1a6d19f987 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 30325: Implementing pulseJobUpdate RPC.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/#review69910 --- Master (57a8f4f) is red with this patch. ./build-support/jenkins/build.sh ../compiler/cpp/thrift --gen html -r ../tutorial/tutorial.thrift make[4]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1/tutorial' make[3]: Entering directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make[3]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make[2]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make[1]: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift/thrift-0.9.1' make: Leaving directory `/home/jenkins/jenkins-slave/workspace/AuroraBot/build-support/thrift' :api:classesThriftNote: Some input files use unchecked or unsafe operations. Note: Recompile with -Xlint:unchecked for details. :api:checkPython :api:generateThriftEntitiesJava :api:classesThriftEntities :api:compileJava UP-TO-DATE :api:generateThriftResources :api:processResources UP-TO-DATE :api:classes :api:jar :compileJavaNote: Writing file:/home/jenkins/jenkins-slave/workspace/AuroraBot/dist/classes/main/com/twitter/common/args/apt/cmdline.arg.info.txt.2 /home/jenkins/jenkins-slave/workspace/AuroraBot/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java:1535: error: cannot find symbol JobUpdatePulseStatus result = jobUpdateController.pulse(checkNotBlank(updateId)); ^ symbol: method pulse(String) location: variable jobUpdateController of type JobUpdateController 1 error FAILED FAILURE: Build failed with an exception. * What went wrong: Execution failed for task ':compileJava'. Compilation failed; see the compiler error output for details. * Try: Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output. BUILD FAILED Total time: 1 mins 18.297 secs I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Jan. 27, 2015, 9:20 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30325/ --- (Updated Jan. 27, 2015, 9:20 p.m.) Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. Bugs: AURORA-1009 https://issues.apache.org/jira/browse/AURORA-1009 Repository: aurora Description --- Implemented the `pulseJobUpdate` RPC. Also, moved it into AuroraAdmin interface to support AOP capability validation. The RB is diffed against https://reviews.apache.org/r/30225/ Diffs - api/src/main/thrift/org/apache/aurora/gen/api.thrift 4f603f9e7ed004e53937a45ea8edf7241e15f5cf src/main/java/org/apache/aurora/auth/CapabilityValidator.java 45ef643ebe57c1517cdae373574331ea302a8b74 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 8c19f3b08135eb5f3098591ebf9931b42a086318 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 03d1fba76c23570c2c4102a48daf5ce035ecaaa3 Diff: https://reviews.apache.org/r/30325/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
