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

[LIBCLOUD-973] Support disk_size parameter for boot disk when creating instance. #1162

Conversation

Rahul-CSI
Copy link
Contributor

Support specifying the disk size for boot disk when we create (one/more) google compute cloud instances.

Description

This is fix for LIBCLOUD-973 bug.
Link to bug: https://issues.apache.org/jira/browse/LIBCLOUD-973
The ex_create_multiple_nodes API call in gce.py compute driver
does not handle boot disk size. Hence it creates a boot disk of size 10GB every time.
Added disk_size keyword parameter to ex_create_multiple_nodes API call,
It passes the parameter through internal method calls and adds the disk size to the POST request.

  • API call sequence:
  1. ex_create_multiple_nodes
  2. _multi_create_node
  3. _create_node_req
  4. _create_instance_properties
  5. _build_disk_gce_struct

Status

  • done, 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)

@Rahul-CSI
Copy link
Contributor Author

The failure does not looks related to the PR.

The failure is due to:
pip install graphviz gcc libvirt-bin

@Rahul-CSI
Copy link
Contributor Author

@Kami @erjohnso @supertom
Hi,

I created a PR and the build failed.
The failure is due to:
pip install graphviz gcc libvirt-bin

I am not sure if the failure is caused due to my code changes.
Could you please help me here.
Also could you please review the code changes.

@pquentin
Copy link
Contributor

pquentin commented Jan 9, 2018

@Rahul-CSI it's not related, indeed - Travis appears to be a bit broken these days

@pquentin
Copy link
Contributor

@Rahul-CSI Sorry, the failure was on our end. It's fixed now. Can you rebase your pull request on top of latest trunk and add tests? Please ask if you don't know how to do that. Thanks.

@Rahul-CSI
Copy link
Contributor Author

Thanks @pquentin
I will rebase my PR on trunk,
Please let me know how I can add tests.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Add tests

…ng instances in ex_create_multiple_nodes API call.
@Rahul-CSI Rahul-CSI force-pushed the LIBCLOUD-973_Support_disk_size branch from 81b58ae to a661f5c Compare January 23, 2018 19:14
@Rahul-CSI
Copy link
Contributor Author

@pquentin
I rebased my branch on upstream trunk,

@BTaskaya
Added test into "libcloud/test/compute/test_gce.py" itself as there is a small change of disk_size,
When I ran test with PYTHONPATH=. python libcloud/test/compute/test_gce.py
I am observing attribute errors for all tests like below:
AttributeError: 'GCEMockHttp' object has no attribute '_projects_project_name_zones_us_central1_a_operations_operation_zones_us_central1_a_instances_post'

Ran 178 tests in 15.032s
FAILED (errors=104)

Please let me know if I am missing something.

@codecov-io
Copy link

codecov-io commented Jan 23, 2018

Codecov Report

Merging #1162 into trunk will increase coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@           Coverage Diff            @@
##           trunk   #1162      +/-   ##
========================================
+ Coverage   86.1%   86.1%   +<.01%     
========================================
  Files        348     348              
  Lines      66814   66842      +28     
  Branches    5935    5937       +2     
========================================
+ Hits       57532   57557      +25     
- Misses      6855    6857       +2     
- Partials    2427    2428       +1
Impacted Files Coverage Δ
libcloud/compute/drivers/gce.py 76.33% <0%> (+0.06%) ⬆️
libcloud/test/compute/test_gce.py 97.65% <100%> (ø) ⬆️
libcloud/common/cloudstack.py 86.59% <0%> (-1.83%) ⬇️
libcloud/test/compute/test_ec2.py 97.77% <0%> (-0.16%) ⬇️
libcloud/test/compute/test_cloudstack.py 99.51% <0%> (ø) ⬆️
libcloud/compute/drivers/cloudstack.py 75.59% <0%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd11628...0107f8a. Read the comment docs.

@Rahul-CSI
Copy link
Contributor Author

libcloud_gce_test.log

@BTaskaya
The "libcloud/test/compute/test_gce.py" fails without any of my test changes.
Please find attached output file.

Command:
PYTHONPATH=. python libcloud/test/compute/test_gce.py

@pquentin
Copy link
Contributor

@Rahul-CSI do they fail without any of your changes?

To avoid doing real network operations, libcloud uses fixtures and mocks. This should documented, but for now #1157 (comment) might help you understand what's going on.

Please tell me if that helps!

@Rahul-CSI
Copy link
Contributor Author

@pquentin Yes the failure is without any of my changes.
I even tried it on trunk branch, but it is still failing.

Can you point to me detailed test documentation please.
I just know about this

I think I have somewhat understood #1157 (comment)

When testing libcloud/test/compute/test_ecs.py
in ECSDriverTestCase class methods that start with test_ are considered as tests,
Similarly in my case,
when testing libcloud/test/compute/test_gce.py
in GCENodeDriverTest class, methods that start with test_ are considered as tests,
test_gce.py is also using GCEMockHttp, and GCEMockHttp is subclass of MockHttp.
It seems there is some issue with libcloud/test/compute/test_gce.py

@pquentin
Copy link
Contributor

pquentin commented Jan 24, 2018 via email

@Rahul-CSI
Copy link
Contributor Author

@pquentin
Thanks, I have opened defect LIBCLOUD-976 for this.

@pquentin
Copy link
Contributor

pquentin commented Jan 25, 2018

Okay, I understand now! As I said on LIBCLOUD-976, If you simply use cp libcloud/test/secrets.py-dist libcloud/test/secrets.py without providing your secrets, the tests should run fine: they don't need to access to the actual Google servers. I guess the bug is that providing secrets is breaking the tests.

@Rahul-CSI
Copy link
Contributor Author

Thanks @pquentin
That really helped.
I was putting secrets into libcloud/test/secrets.py hence it was failing.
Closed : https://issues.apache.org/jira/browse/LIBCLOUD-976

I had to struggle a bit though to know that we need to modify the fixtures json files as the request does not directly go to google.
Finally figured it out and added a test.

@Rahul-CSI
Copy link
Contributor Author

@BTaskaya
I have added tests.
Travis build is in progress.

Copy link
Contributor

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good. I requested a small formatting change.

Do you think it would be easy to add disk_size support to create_node instead of only ex_create_multiple_nodes?

Copy link

@rambleraptor rambleraptor left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@asfgit asfgit closed this in 68ba179 Mar 16, 2018
@pquentin
Copy link
Contributor

Thanks! Merged in trunk. ✨

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants