Skip to content
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

Fix list_nodes in the Azure arm driver #1850

Merged
merged 40 commits into from Jul 31, 2023

Conversation

jan-mue
Copy link
Contributor

@jan-mue jan-mue commented Jan 25, 2023

Fix the list_nodes function in the Azure ARM driver

Description

The Azure API returns an attribute nextLink for pagination that is currently not used in the Azure ARM driver.
Documentation: https://learn.microsoft.com/en-us/rest/api/compute/virtual-machines/list-all?tabs=HTTP

This PR fixes the issue where not all VMs are returned for subscriptions with a lot of VMs (#1824)

Status

ready for review

Checklist (tick everything that applies)

  • Code linting (required, can be done after the PR checks)
  • Documentation
  • Tests
  • ICLA (required for bigger changes)

@Kami
Copy link
Member

Kami commented Jan 28, 2023

Thanks for the contribution.

It would be great if you could also add a unit test for this change.

mohammad-aburadeh and others added 6 commits March 20, 2023 15:00
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
Cloud Files has no concept of an "internal" CDN so the endpoint type must be hard-coded to "external".
Sample Cloud Files service catalog listing: https://docs.rackspace.com/docs/cloud-files/v1/getting-started/authenticate#cloud-files-auth-response
@jan-mue
Copy link
Contributor Author

jan-mue commented Apr 13, 2023

@Kami sorry for the long delay, but I've added some unit tests now. Could you please have another look at the PR?

@Kami
Copy link
Member

Kami commented Jul 31, 2023

@jan-mue Thanks for adding a test case. I will review it shortly and if everything looks fine, merge changes into trunk.

]
params = {"api-version": VM_API_VERSION}
nodes = []
while True:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code is far for now, but to be on the safe side and avoid possible infinite loop, I think it would be good to add additional guard here - e.g. bail out if we are still stuck in this loop for more than X seconds.

@Kami
Copy link
Member

Kami commented Jul 31, 2023

@jan-mue Can you please also sign an ICLA when you get a chance (https://libcloud.readthedocs.io/en/latest/development.html#contributing-bigger-changes)? Thanks.

returns invalid response or similar.

Also add a test case for it.
@Kami
Copy link
Member

Kami commented Jul 31, 2023

I pushed a change so we don't end up in an infinite loop in case server returns a bad response, there is a bug in the code or similar - c49ecd0.

I picked that 60 seconds limit arbitrarily, but if needed or wanted, it can be changed (e.g. increased) - @jan-mue Can you please advise if 60 seconds is enough or the limit should be higher?

I imagine we may want a higher limit since depending on the method arguments, each _to_node() call will result in additional HTTP request(s).

For now, in case deadline is reached we simply return, but we may also want to throw or at least log a warning in such scenario.

@Kami
Copy link
Member

Kami commented Jul 31, 2023

EDIT: I went ahead and bumped that limit to 300 seconds for now. This way I can wrap up and merge this PR.

And then based on your future feedback @jan-mue, we can adjust this value.

@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2023

Codecov Report

Merging #1850 (863b88d) into trunk (d163f9b) will increase coverage by 0.01%.
Report is 30 commits behind head on trunk.
The diff coverage is 89.39%.

@@            Coverage Diff             @@
##            trunk    #1850      +/-   ##
==========================================
+ Coverage   83.11%   83.12%   +0.01%     
==========================================
  Files         353      353              
  Lines       81269    81317      +48     
  Branches     8587     8592       +5     
==========================================
+ Hits        67541    67588      +47     
- Misses      10922    10924       +2     
+ Partials     2806     2805       -1     
Files Changed Coverage Δ
libcloud/backup/drivers/dimensiondata.py 94.94% <ø> (ø)
libcloud/common/aliyun.py 74.44% <ø> (ø)
libcloud/common/azure.py 86.30% <ø> (ø)
libcloud/common/azure_arm.py 73.02% <ø> (ø)
libcloud/common/durabledns.py 99.25% <ø> (ø)
libcloud/common/google.py 71.35% <ø> (ø)
libcloud/common/gridscale.py 83.93% <ø> (ø)
libcloud/common/kubernetes.py 80.81% <ø> (ø)
libcloud/common/nttcis.py 73.59% <ø> (ø)
libcloud/common/vultr.py 84.93% <ø> (ø)
... and 69 more

@asfgit asfgit merged commit 863b88d into apache:trunk Jul 31, 2023
20 checks passed
@Kami Kami added this to the v3.8.0 milestone Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants