You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Ryan Dietrich <ry...@betterservers.com> on 2013/07/19 01:28:16 UTC
Review Request 12752: Async commands can inject the job id
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12752/
-----------------------------------------------------------
Review request for cloudstack and Marcus Sorensen.
Repository: cloudstack-git
Description
-------
I added "injectedjobid" to the BaseAsyncCmd class as a parameter.
If set, it will allow you to tell Cloudstack what the job id instead of it choosing one.
A basic string length test is done to verify the variable passed in is actually a UUID.
If it is not valid, it is ignored and the job generates it's own.
Diffs
-----
api/src/org/apache/cloudstack/api/BaseAsyncCmd.java 0e6f95d
server/src/com/cloud/api/ApiServer.java 95f17af
server/src/com/cloud/async/AsyncJobVO.java 41eccb4
test/integration/smoke/test_deploy_vm.py 425aeb7
tools/marvin/marvin/codegenerator.py 632b8c6
tools/marvin/marvin/integration/lib/base.py 161d03c
Diff: https://reviews.apache.org/r/12752/diff/
Testing
-------
Updated marvin, updated the deploy vm test. Ran multiple async commands manually, with and without injectedjobid present, no issues detected.
Thanks,
Ryan Dietrich
Re: Review Request 12752: Async commands can inject the job id
Posted by Marcus Sorensen <sh...@gmail.com>.
This is maybe a tangent, but does cloudstack check for uniqueness when
generating a UUID? I haven't seen it anywhere, I've always seen it
do UUID.randomUUID() and no checking. However, I guess the point is that CS
is in control of making sure it's randomly generated, with this patch CS
could be fed duplicate UUIDs by a misbehaving program.
On Jul 19, 2013 3:23 AM, "Murali Reddy" <Mu...@citrix.com> wrote:
> Ryan,
>
> You should open a bug and add details on what the use case you are trying
> to solve. Since CloudStack is not in control of UUID generation, you
> should at least check that uniqueness of passed in UUID.
>
> Also please introduce a new smoke test for 'injected job id' instead of
> piggy backing the test for deploy VM.
>
> On 19/07/13 6:02 AM, "Ryan Dietrich" <ry...@betterservers.com> wrote:
>
> >
> >
> >> On July 18, 2013, 11:34 p.m., Marcus Sorensen wrote:
> >> > server/src/com/cloud/async/AsyncJobVO.java, line 141
> >> >
> >><
> https://reviews.apache.org/r/12752/diff/1/?file=323558#file323558line141
> >>>
> >> >
> >> > I'm not immediately certain why this was pulled out.
> >
> >Nothing is using it. I did a scan to see if anyone is using it, and I
> >didn't see anything. Dead could should be removed, right? Git will
> >remember for us if we need it later.
> >
> >
> >- Ryan
> >
> >
> >-----------------------------------------------------------
> >This is an automatically generated e-mail. To reply, visit:
> >https://reviews.apache.org/r/12752/#review23457
> >-----------------------------------------------------------
> >
> >
> >On July 18, 2013, 11:28 p.m., Ryan Dietrich wrote:
> >>
> >> -----------------------------------------------------------
> >> This is an automatically generated e-mail. To reply, visit:
> >> https://reviews.apache.org/r/12752/
> >> -----------------------------------------------------------
> >>
> >> (Updated July 18, 2013, 11:28 p.m.)
> >>
> >>
> >> Review request for cloudstack and Marcus Sorensen.
> >>
> >>
> >> Repository: cloudstack-git
> >>
> >>
> >> Description
> >> -------
> >>
> >> I added "injectedjobid" to the BaseAsyncCmd class as a parameter.
> >> If set, it will allow you to tell Cloudstack what the job id instead of
> >>it choosing one.
> >> A basic string length test is done to verify the variable passed in is
> >>actually a UUID.
> >> If it is not valid, it is ignored and the job generates it's own.
> >>
> >>
> >> Diffs
> >> -----
> >>
> >> api/src/org/apache/cloudstack/api/BaseAsyncCmd.java 0e6f95d
> >> server/src/com/cloud/api/ApiServer.java 95f17af
> >> server/src/com/cloud/async/AsyncJobVO.java 41eccb4
> >> test/integration/smoke/test_deploy_vm.py 425aeb7
> >> tools/marvin/marvin/codegenerator.py 632b8c6
> >> tools/marvin/marvin/integration/lib/base.py 161d03c
> >>
> >> Diff: https://reviews.apache.org/r/12752/diff/
> >>
> >>
> >> Testing
> >> -------
> >>
> >> Updated marvin, updated the deploy vm test. Ran multiple async
> >>commands manually, with and without injectedjobid present, no issues
> >>detected.
> >>
> >>
> >> Thanks,
> >>
> >> Ryan Dietrich
> >>
> >>
> >
> >
>
>
>
Re: Review Request 12752: Async commands can inject the job id
Posted by Murali Reddy <Mu...@citrix.com>.
Ryan,
You should open a bug and add details on what the use case you are trying
to solve. Since CloudStack is not in control of UUID generation, you
should at least check that uniqueness of passed in UUID.
Also please introduce a new smoke test for 'injected job id' instead of
piggy backing the test for deploy VM.
On 19/07/13 6:02 AM, "Ryan Dietrich" <ry...@betterservers.com> wrote:
>
>
>> On July 18, 2013, 11:34 p.m., Marcus Sorensen wrote:
>> > server/src/com/cloud/async/AsyncJobVO.java, line 141
>> >
>><https://reviews.apache.org/r/12752/diff/1/?file=323558#file323558line141
>>>
>> >
>> > I'm not immediately certain why this was pulled out.
>
>Nothing is using it. I did a scan to see if anyone is using it, and I
>didn't see anything. Dead could should be removed, right? Git will
>remember for us if we need it later.
>
>
>- Ryan
>
>
>-----------------------------------------------------------
>This is an automatically generated e-mail. To reply, visit:
>https://reviews.apache.org/r/12752/#review23457
>-----------------------------------------------------------
>
>
>On July 18, 2013, 11:28 p.m., Ryan Dietrich wrote:
>>
>> -----------------------------------------------------------
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/12752/
>> -----------------------------------------------------------
>>
>> (Updated July 18, 2013, 11:28 p.m.)
>>
>>
>> Review request for cloudstack and Marcus Sorensen.
>>
>>
>> Repository: cloudstack-git
>>
>>
>> Description
>> -------
>>
>> I added "injectedjobid" to the BaseAsyncCmd class as a parameter.
>> If set, it will allow you to tell Cloudstack what the job id instead of
>>it choosing one.
>> A basic string length test is done to verify the variable passed in is
>>actually a UUID.
>> If it is not valid, it is ignored and the job generates it's own.
>>
>>
>> Diffs
>> -----
>>
>> api/src/org/apache/cloudstack/api/BaseAsyncCmd.java 0e6f95d
>> server/src/com/cloud/api/ApiServer.java 95f17af
>> server/src/com/cloud/async/AsyncJobVO.java 41eccb4
>> test/integration/smoke/test_deploy_vm.py 425aeb7
>> tools/marvin/marvin/codegenerator.py 632b8c6
>> tools/marvin/marvin/integration/lib/base.py 161d03c
>>
>> Diff: https://reviews.apache.org/r/12752/diff/
>>
>>
>> Testing
>> -------
>>
>> Updated marvin, updated the deploy vm test. Ran multiple async
>>commands manually, with and without injectedjobid present, no issues
>>detected.
>>
>>
>> Thanks,
>>
>> Ryan Dietrich
>>
>>
>
>
Re: Review Request 12752: Async commands can inject the job id
Posted by Ryan Dietrich <ry...@betterservers.com>.
> On July 18, 2013, 11:34 p.m., Marcus Sorensen wrote:
> > server/src/com/cloud/async/AsyncJobVO.java, line 141
> > <https://reviews.apache.org/r/12752/diff/1/?file=323558#file323558line141>
> >
> > I'm not immediately certain why this was pulled out.
Nothing is using it. I did a scan to see if anyone is using it, and I didn't see anything. Dead could should be removed, right? Git will remember for us if we need it later.
- Ryan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12752/#review23457
-----------------------------------------------------------
On July 18, 2013, 11:28 p.m., Ryan Dietrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12752/
> -----------------------------------------------------------
>
> (Updated July 18, 2013, 11:28 p.m.)
>
>
> Review request for cloudstack and Marcus Sorensen.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> I added "injectedjobid" to the BaseAsyncCmd class as a parameter.
> If set, it will allow you to tell Cloudstack what the job id instead of it choosing one.
> A basic string length test is done to verify the variable passed in is actually a UUID.
> If it is not valid, it is ignored and the job generates it's own.
>
>
> Diffs
> -----
>
> api/src/org/apache/cloudstack/api/BaseAsyncCmd.java 0e6f95d
> server/src/com/cloud/api/ApiServer.java 95f17af
> server/src/com/cloud/async/AsyncJobVO.java 41eccb4
> test/integration/smoke/test_deploy_vm.py 425aeb7
> tools/marvin/marvin/codegenerator.py 632b8c6
> tools/marvin/marvin/integration/lib/base.py 161d03c
>
> Diff: https://reviews.apache.org/r/12752/diff/
>
>
> Testing
> -------
>
> Updated marvin, updated the deploy vm test. Ran multiple async commands manually, with and without injectedjobid present, no issues detected.
>
>
> Thanks,
>
> Ryan Dietrich
>
>
Re: Review Request 12752: Async commands can inject the job id
Posted by Marcus Sorensen <sh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12752/#review23457
-----------------------------------------------------------
api/src/org/apache/cloudstack/api/BaseAsyncCmd.java
<https://reviews.apache.org/r/12752/#comment47362>
You could do what ApiDispatcher.java does for validating UUIDs, for consistency:
uuid.matches("^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$");
server/src/com/cloud/async/AsyncJobVO.java
<https://reviews.apache.org/r/12752/#comment47363>
I'm not immediately certain why this was pulled out.
- Marcus Sorensen
On July 18, 2013, 11:28 p.m., Ryan Dietrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12752/
> -----------------------------------------------------------
>
> (Updated July 18, 2013, 11:28 p.m.)
>
>
> Review request for cloudstack and Marcus Sorensen.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> I added "injectedjobid" to the BaseAsyncCmd class as a parameter.
> If set, it will allow you to tell Cloudstack what the job id instead of it choosing one.
> A basic string length test is done to verify the variable passed in is actually a UUID.
> If it is not valid, it is ignored and the job generates it's own.
>
>
> Diffs
> -----
>
> api/src/org/apache/cloudstack/api/BaseAsyncCmd.java 0e6f95d
> server/src/com/cloud/api/ApiServer.java 95f17af
> server/src/com/cloud/async/AsyncJobVO.java 41eccb4
> test/integration/smoke/test_deploy_vm.py 425aeb7
> tools/marvin/marvin/codegenerator.py 632b8c6
> tools/marvin/marvin/integration/lib/base.py 161d03c
>
> Diff: https://reviews.apache.org/r/12752/diff/
>
>
> Testing
> -------
>
> Updated marvin, updated the deploy vm test. Ran multiple async commands manually, with and without injectedjobid present, no issues detected.
>
>
> Thanks,
>
> Ryan Dietrich
>
>
Re: Review Request 12752: Async commands can inject the job id
Posted by Marcus Sorensen <sh...@gmail.com>.
So all that's left on this is a use case writeup and the test, right?
On Fri, Jul 19, 2013 at 11:54 AM, Ryan Dietrich <ry...@betterservers.com>wrote:
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12752/
> Review request for cloudstack and Marcus Sorensen.
> By Ryan Dietrich.
>
> *Updated July 19, 2013, 5:54 p.m.*
> Changes
>
> Thanks for the code change mod Marcus! (changed the length call to the regex, I verified in the database this was the exact format being used)
>
> *Repository: * cloudstack-git
> Description
>
> I added "injectedjobid" to the BaseAsyncCmd class as a parameter.
> If set, it will allow you to tell Cloudstack what the job id instead of it choosing one.
> A basic string length test is done to verify the variable passed in is actually a UUID.
> If it is not valid, it is ignored and the job generates it's own.
>
> Testing
>
> Updated marvin, updated the deploy vm test. Ran multiple async commands manually, with and without injectedjobid present, no issues detected.
>
> Diffs (updated)
>
> - api/src/org/apache/cloudstack/api/BaseAsyncCmd.java (0e6f95d)
> - server/src/com/cloud/api/ApiServer.java (95f17af)
> - server/src/com/cloud/async/AsyncJobVO.java (41eccb4)
> - test/integration/smoke/test_deploy_vm.py (425aeb7)
> - tools/marvin/marvin/codegenerator.py (632b8c6)
> - tools/marvin/marvin/integration/lib/base.py (161d03c)
>
> View Diff <https://reviews.apache.org/r/12752/diff/>
>
Re: Review Request 12752: Async commands can inject the job id
Posted by Ryan Dietrich <ry...@betterservers.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12752/
-----------------------------------------------------------
(Updated July 19, 2013, 5:54 p.m.)
Review request for cloudstack and Marcus Sorensen.
Changes
-------
Thanks for the code change mod Marcus! (changed the length call to the regex, I verified in the database this was the exact format being used)
Repository: cloudstack-git
Description
-------
I added "injectedjobid" to the BaseAsyncCmd class as a parameter.
If set, it will allow you to tell Cloudstack what the job id instead of it choosing one.
A basic string length test is done to verify the variable passed in is actually a UUID.
If it is not valid, it is ignored and the job generates it's own.
Diffs (updated)
-----
api/src/org/apache/cloudstack/api/BaseAsyncCmd.java 0e6f95d
server/src/com/cloud/api/ApiServer.java 95f17af
server/src/com/cloud/async/AsyncJobVO.java 41eccb4
test/integration/smoke/test_deploy_vm.py 425aeb7
tools/marvin/marvin/codegenerator.py 632b8c6
tools/marvin/marvin/integration/lib/base.py 161d03c
Diff: https://reviews.apache.org/r/12752/diff/
Testing
-------
Updated marvin, updated the deploy vm test. Ran multiple async commands manually, with and without injectedjobid present, no issues detected.
Thanks,
Ryan Dietrich