New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LIBCLOUD 770 wait for state #631

Closed
wants to merge 19 commits into
base: trunk
from

Conversation

Projects
None yet
5 participants
@tonybaloney
Contributor

tonybaloney commented Nov 12, 2015

Support for waiting for a request to reach a target state (i.e. provisioned)

Show outdated Hide outdated libcloud/common/dimensiondata.py
response = func(*args, **kwargs)
if response.status is state or response.status in state:
break
sleep(2)

This comment has been minimized.

@Kami

Kami Nov 12, 2015

Member

It might be a good idea to make the sleep interval configurable with a function argument.

@Kami

Kami Nov 12, 2015

Member

It might be a good idea to make the sleep interval configurable with a function argument.

This comment has been minimized.

@Kami

Kami Nov 12, 2015

Member

if we make this a bit more generic and safe (timeout, etc.) we could also consider it adding to the base NodeDriver class.

@Kami

Kami Nov 12, 2015

Member

if we make this a bit more generic and safe (timeout, etc.) we could also consider it adding to the base NodeDriver class.

Show outdated Hide outdated libcloud/common/dimensiondata.py
while(True):
response = func(*args, **kwargs)
if response.status is state or response.status in state:
break

This comment has been minimized.

@Kami

Kami Nov 12, 2015

Member

Probably a good idea to return original function result (node / instance) when state transition happens?

@Kami

Kami Nov 12, 2015

Member

Probably a good idea to return original function result (node / instance) when state transition happens?

Show outdated Hide outdated libcloud/common/dimensiondata.py
:param kwargs: The arguments for func
:type kwargs: Keyword arguments
"""
while(True):

This comment has been minimized.

@Kami

Kami Nov 12, 2015

Member

Minor style thing - no parenthesis needed in Python :)

@Kami

Kami Nov 12, 2015

Member

Minor style thing - no parenthesis needed in Python :)

This comment has been minimized.

@Kami

Kami Nov 12, 2015

Member

Might also be a good idea to add some kind of timeout functionality?

Since now it could potentially block forever...

@Kami

Kami Nov 12, 2015

Member

Might also be a good idea to add some kind of timeout functionality?

Since now it could potentially block forever...

Show outdated Hide outdated libcloud/compute/drivers/dimensiondata.py
:param kwargs: The arguments for func
:type kwargs: Keyword arguments
"""
self.connection.wait_for_state(state, func, *args, **kwargs)

This comment has been minimized.

@Kami

Kami Nov 12, 2015

Member

Same here - if you update the original function to return the result you can also update this one.

@Kami

Kami Nov 12, 2015

Member

Same here - if you update the original function to return the result you can also update this one.

take optional parameters for the timeout and polling interval, defaul…
…ted on the public methods, raise error on timeout to stop infinite loop
@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Nov 12, 2015

Contributor

Updated code to reflect comments. Agree on adding it to nodedriver, I've seen similar helper methods in libraries consuming libcloud, would need to research whether all of the drivers use the same field name (status).

Contributor

tonybaloney commented Nov 12, 2015

Updated code to reflect comments. Agree on adding it to nodedriver, I've seen similar helper methods in libraries consuming libcloud, would need to research whether all of the drivers use the same field name (status).

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Nov 12, 2015

Contributor

all done

Contributor

tonybaloney commented Nov 12, 2015

all done

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Nov 12, 2015

Member

I have trouble applying the patch directly onto trunk.

Can you please sync this branch with latest trunk and squash the commits?

Member

Kami commented Nov 12, 2015

I have trouble applying the patch directly onto trunk.

Can you please sync this branch with latest trunk and squash the commits?

tonybaloney added some commits Nov 12, 2015

Provide a way for users to wait for an asset to complete (+4 squashed…
… commit)

Squashed commit:

[37abdea] return a DD exception instead of a standard error

[a41b98a] take optional parameters for the timeout and polling interval, defaulted on the public methods, raise error on timeout to stop infinite loop

[9be032b] VLAN has an attribute for the owning network domain, since you really would want to know what that is!

[fa3a6b2] Added tests and updated arguement options (+2 squashed commit)

Squashed commit:

[9be032b] VLAN has an attribute for the owning network domain, since you really would want to know what that is!

[fa3a6b2] Added tests and updated arguement options
Merge branch 'LIBCLOUD-770_Wait_for_state' of github.com:DimensionDat…
…aCBUSydney/libcloud into LIBCLOUD-770_Wait_for_state
@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Nov 12, 2015

Contributor

DimensionDataCBUSydney@609fa85
squashed. I resynced but there weren't any differences with trunk. see if this is mergable

Contributor

tonybaloney commented Nov 12, 2015

DimensionDataCBUSydney@609fa85
squashed. I resynced but there weren't any differences with trunk. see if this is mergable

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Nov 12, 2015

Member

I'm still having issues, I will manually apply it tomorrow by directly merging your branch locally :)

kami ~/w/lc/libcloud (git:trunk)$ git am --signoff 609fa8541188fb3daae7653e253c77c0dd422c2f.patch
Applying: Provide a way for users to wait for an asset to complete (+4 squashed commit)
error: patch failed: libcloud/common/dimensiondata.py:218
error: libcloud/common/dimensiondata.py: patch does not apply
error: patch failed: libcloud/compute/drivers/dimensiondata.py:815
error: libcloud/compute/drivers/dimensiondata.py: patch does not apply
error: patch failed: libcloud/loadbalancer/drivers/dimensiondata.py:823
error: libcloud/loadbalancer/drivers/dimensiondata.py: patch does not apply
Patch failed at 0001 Provide a way for users to wait for an asset to complete (+4 squashed commit)
The copy of the patch that failed is found in:
   /home/kami/w/lc/libcloud/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Member

Kami commented Nov 12, 2015

I'm still having issues, I will manually apply it tomorrow by directly merging your branch locally :)

kami ~/w/lc/libcloud (git:trunk)$ git am --signoff 609fa8541188fb3daae7653e253c77c0dd422c2f.patch
Applying: Provide a way for users to wait for an asset to complete (+4 squashed commit)
error: patch failed: libcloud/common/dimensiondata.py:218
error: libcloud/common/dimensiondata.py: patch does not apply
error: patch failed: libcloud/compute/drivers/dimensiondata.py:815
error: libcloud/compute/drivers/dimensiondata.py: patch does not apply
error: patch failed: libcloud/loadbalancer/drivers/dimensiondata.py:823
error: libcloud/loadbalancer/drivers/dimensiondata.py: patch does not apply
Patch failed at 0001 Provide a way for users to wait for an asset to complete (+4 squashed commit)
The copy of the patch that failed is found in:
   /home/kami/w/lc/libcloud/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Kami added a commit to Kami/libcloud that referenced this pull request Nov 12, 2015

Added tests and updated arguement options
Closes #631

Signed-off-by: Tomaz Muraus <tomaz@apache.org>

Kami added a commit to Kami/libcloud that referenced this pull request Nov 12, 2015

VLAN has an attribute for the owning network domain, since you really…
… would want to know what that is!

Closes #631

Signed-off-by: Tomaz Muraus <tomaz@apache.org>

Kami added a commit to Kami/libcloud that referenced this pull request Nov 12, 2015

take optional parameters for the timeout and polling interval, defaul…
…ted on the public methods, raise error on timeout to stop infinite loop

Closes #631

Signed-off-by: Tomaz Muraus <tomaz@apache.org>

Kami added a commit to Kami/libcloud that referenced this pull request Nov 12, 2015

return a DD exception instead of a standard error
Closes #631

Signed-off-by: Tomaz Muraus <tomaz@apache.org>

Kami added a commit to Kami/libcloud that referenced this pull request Nov 12, 2015

pylint errors
Closes #631

Signed-off-by: Tomaz Muraus <tomaz@apache.org>

@asfgit asfgit closed this in 84501da Nov 12, 2015

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Nov 12, 2015

Member

Alright, I merged the changes. Thanks.

Here is what I did:

git fetch dd
git checkout LIBCLOUD-770_Wait_for_state
git rebase trunk  -> that's the important part since it replayed your changes on top of trunk so the changes were now in the mergable order
git format-patch trunk --stdout > 1.patch
git checkout trunk
git am --signoff 1.patch

I also made a small change - I supplied default value for poll_interval and timeout argument and changed exception message to also include response body - 4a74243. Let me know if the response.body change looks OK.

Member

Kami commented Nov 12, 2015

Alright, I merged the changes. Thanks.

Here is what I did:

git fetch dd
git checkout LIBCLOUD-770_Wait_for_state
git rebase trunk  -> that's the important part since it replayed your changes on top of trunk so the changes were now in the mergable order
git format-patch trunk --stdout > 1.patch
git checkout trunk
git am --signoff 1.patch

I also made a small change - I supplied default value for poll_interval and timeout argument and changed exception message to also include response body - 4a74243. Let me know if the response.body change looks OK.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Nov 12, 2015

Contributor

would response.body have any value?

here's an example of me using it>

so ex_get_vlan returns a DimensionDataVlan object instance, which doensn't
have a body attribute.

driver.ex_wait_for_state('NORMAL', driver.ex_get_vlan, result['id'])

On Fri, Nov 13, 2015 at 10:06 AM, Tomaz Muraus notifications@github.com
wrote:

Alright, I merged the changes. Thanks.

Here is what I did:

git fetch dd
git checkout LIBCLOUD-770_Wait_for_state
git rebase trunk
git format-patch trunk --stdout > 1.patch
git checkout trunk
git am --signoff 1.patch

I also made a small change - I supplied default value for poll_interval
and timeout argument and changed exception message to also include response
body - 4a74243
4a74243.
Let me know if the response.body change looks OK.


Reply to this email directly or view it on GitHub
#631 (comment).

Contributor

tonybaloney commented Nov 12, 2015

would response.body have any value?

here's an example of me using it>

so ex_get_vlan returns a DimensionDataVlan object instance, which doensn't
have a body attribute.

driver.ex_wait_for_state('NORMAL', driver.ex_get_vlan, result['id'])

On Fri, Nov 13, 2015 at 10:06 AM, Tomaz Muraus notifications@github.com
wrote:

Alright, I merged the changes. Thanks.

Here is what I did:

git fetch dd
git checkout LIBCLOUD-770_Wait_for_state
git rebase trunk
git format-patch trunk --stdout > 1.patch
git checkout trunk
git am --signoff 1.patch

I also made a small change - I supplied default value for poll_interval
and timeout argument and changed exception message to also include response
body - 4a74243
4a74243.
Let me know if the response.body change looks OK.


Reply to this email directly or view it on GitHub
#631 (comment).

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Nov 12, 2015

Member

@tonybaloney Ah, you are correct.

I was confused for a second since the method is on a connection class, but it actually returns an object not the raw response from the connection class (it returns result from a calling method).

I will revert that change and change variable name from response to result to make it a bit less confusing (response in connection class usually refers to an instance of Libcloud HTTP response class).

Member

Kami commented Nov 12, 2015

@tonybaloney Ah, you are correct.

I was confused for a second since the method is on a connection class, but it actually returns an object not the raw response from the connection class (it returns result from a calling method).

I will revert that change and change variable name from response to result to make it a bit less confusing (response in connection class usually refers to an instance of Libcloud HTTP response class).

@Kami

This comment has been minimized.

Show comment
Hide comment
@Kami

Kami Nov 12, 2015

Member

Alright, I've pushed a fix in - 232f949.

Another minor feature improvement would be to change the method signature to (state, poll_interval, timeout, func, *args, **kargs). This way you can partially apply the function, e.g.:

wait_for_vlan = functools.partial(self.wait_for_state, state=foo, func=self.ex_get_vlan)
vlan = wait_for_vlan(...)

Or you could even modify the method to return an already partially applied function which user can use directly.

Member

Kami commented Nov 12, 2015

Alright, I've pushed a fix in - 232f949.

Another minor feature improvement would be to change the method signature to (state, poll_interval, timeout, func, *args, **kargs). This way you can partially apply the function, e.g.:

wait_for_vlan = functools.partial(self.wait_for_state, state=foo, func=self.ex_get_vlan)
vlan = wait_for_vlan(...)

Or you could even modify the method to return an already partially applied function which user can use directly.

@tonybaloney

This comment has been minimized.

Show comment
Hide comment
@tonybaloney

tonybaloney Nov 12, 2015

Contributor

my python knowledge doesn't extend that far. I was only just up to
functools.wrap
maybe soon I'll know what you're talking about!

On Fri, Nov 13, 2015 at 10:35 AM, Tomaz Muraus notifications@github.com
wrote:

Alright, I've pushed a fix in - 232f949
232f949
.

Another minor feature improvement would be to change the method signature
to (state, poll_interval, timeout, func, _args, *_kargs). This way you
can partially apply the function, e.g.:

wait_for_vlan = functools.partial(self.wait_for_state, state=foo, func=self.ex_get_vlan)
result = wait_for_vlan(...)

Or you could even modify the method to return an already partially applied
function which user can use directly.


Reply to this email directly or view it on GitHub
#631 (comment).

Contributor

tonybaloney commented Nov 12, 2015

my python knowledge doesn't extend that far. I was only just up to
functools.wrap
maybe soon I'll know what you're talking about!

On Fri, Nov 13, 2015 at 10:35 AM, Tomaz Muraus notifications@github.com
wrote:

Alright, I've pushed a fix in - 232f949
232f949
.

Another minor feature improvement would be to change the method signature
to (state, poll_interval, timeout, func, _args, *_kargs). This way you
can partially apply the function, e.g.:

wait_for_vlan = functools.partial(self.wait_for_state, state=foo, func=self.ex_get_vlan)
result = wait_for_vlan(...)

Or you could even modify the method to return an already partially applied
function which user can use directly.


Reply to this email directly or view it on GitHub
#631 (comment).

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 17, 2017

Coverage Status

Changes Unknown when pulling 55f24e3 on DimensionDataCBUSydney:LIBCLOUD-770_Wait_for_state into ** on apache:trunk**.

coveralls commented Sep 17, 2017

Coverage Status

Changes Unknown when pulling 55f24e3 on DimensionDataCBUSydney:LIBCLOUD-770_Wait_for_state into ** on apache:trunk**.

@pquentin

This comment has been minimized.

Show comment
Hide comment
@pquentin

pquentin Sep 18, 2017

Contributor

If possible, it would be nice to disable coveralls who keeps posting random comments in old pull requests.

Contributor

pquentin commented Sep 18, 2017

If possible, it would be nice to disable coveralls who keeps posting random comments in old pull requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment