cloudfoundry / bosh-softlayer-cpi-release

An external BOSH CPI for the SoftLayer cloud written in Golang
Apache License 2.0
14 stars 20 forks source link

WIP: Move to SL official softlayer-go library #258

Closed edwardstudy closed 7 years ago

edwardstudy commented 7 years ago

The official softlayer-go code for each API data type and service method is pre-generated, using the SoftLayer API metadata endpoint as input, thus ensuring 100% coverage of the API right out of the gate. The libray use a session struct to manage all Softlayer Services. Services-relative data can be queried by calling the Mask(), Filter(), Limit() and Offset() service methods prior to invoking an API method. All non-slice method parameters are passed as pointers. Like method parameters, all non-slice members are declared as pointers in datatypes. A custom error type is returned when API services error occurs, with individual fields that can be parsed separately.

This PR is working items with features, and details are below:

  1. Use official softlayer/softlayer-go API lib.
  2. Recode SoftLayer client and relative UTs with a fake SoftLayer server.
  3. Recode a customize logger to handle softlayer-go logs.
  4. Recode CPI actions, implement snapshot_disk, delete_snapshot, get_disks CPI actions.
  5. Use https strictly to setup VPS client.
  6. Use http registry instead of file registry (user_data.json).
  7. Move network/disk configuration from cpi to bosh-agent to eliminate ssh login.

The new CPI could deploy director by bosh-deployment and deploy other apps. For future applicability, we need to keep on adding new features and test cases to enhance CPI's reliability and applicability.

Hi, @mattcui. Please help to review. The change was so huge that manual review was difficult. Thank you very much.

cf-gitbot commented 7 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/150386209

The labels on this github issue will be updated when the story is started.

cfdreddbot commented 7 years ago

Hey EdwardStudy!

Thanks for submitting this pull request!

All pull request submitters and commit authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate).

When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization.

If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions.

Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look.

mattcui commented 7 years ago

Hi @maximilien, as you know, this is a very big change, as we changed the understructure and datatype, we have to rewrite almost all of action and service code, as @EdwardStudy stated in the description, there also introduced some outstanding features (like elimination of ssh, using http registry and etc) to make our new CPI be very closed to other CPIs of the community. We are still working on all missing UTs and try to pass BATs. We can't gradually replace the existing CPI, so we didn't submit the PR to master branch, we will merge to master after we make sure it passes all testing and get your reviewed/approved. In the meantime, I hope you can help take a look at the WIP code, maybe client.go is good starting point to review. Please let us know if you have any questions/concerns. Thanks.

maximilien commented 7 years ago

I cannot see the file changes. Maybe a better strategy is many PRs... so for instance the ssh changes could be separate from the rest and many PRs would allow easier feedback.

Of course, this is up to you guys since if you pair on this maybe it's OK not to have me review.

Finally, while I realize it's a lot of changes, I'd expect things to not be very different. Lots of text and changes that are simply transformation from one to the next. You could almost write code to do those changes... I suspect this since in the end it's the same cloud we are targetting.

edwardstudy commented 7 years ago

Hi,~ @maximilien @mattcui. We updated UT cases and added some new fixes this week.

The details of fixes are as follows:

  1. Support using subnetIds to specify SoftLayer's subnets in deployment manifest. So users can specify VM's subnets or vlans right now. To make process clearly, CPI only specify VM's network components by either of the two ways.
  2. Support unordered arrangement of vlanIds/subnetIds fields in deployment manifest. CPI will check which vlan or subnet is to use the public gateway.

And we used coveralls.io to our code test coverage. And I think using this tool is enough to easily inspect our code quality. we are glad to receive your suggestions.

Please help to review. Thank you very much.