All of Allards' comments have been resolved in #976 https://github.com/apache/libcloud/pull/976.
With one exception. The hash_algo still handles strings (or unicode), iterators and stream objects (with context managers). It's pretty smelly but it does the job for now. I'll come back to it another time when I feel safer about the tests. It was clear from Quentin that we were missing some in the storage drivers because I get a pass but his simple script was failing. I need to write up the changes into the docs and then generate a release package ready for vote. This will be a 2.0 release candidate. Thanks for your help everyone. Anthony On Wed, Jan 11, 2017 at 7:15 PM, anthony shaw <anthony.p.s...@gmail.com> wrote: > Thanks, > > it's also worth noting that the new connection design leverages the > requests session objects, which reuse TCP sessions between requests. > http://docs.python-requests.org/en/master/user/advanced/#session-objects > > If you're making a lot of calls, e.g. polling or paging node data, > there will be a significant performance improvement on 1.5.0< > > The HTTP mocking functionality is hugely improved. Requests has an > adapter interface, so you can consume a driver, connection class > without having to mock out ANYTHING except the underlying HTTP calls. > This is available as a context manager in requests_mock, which I use > for testing a lot of other API projects (it rocks). > > + conn = Connection(host='mock.com', port=80, secure=False) > + conn.connect() > + > + with requests_mock.Mocker() as m: > + m.register_uri('GET', 'http://mock.com/raw_data', text=TEST_DATA, > + headers={'test': 'value'}) > + response = conn.request('/raw_data') > + data = response.text > + self.assertEqual(data, TEST_DATA) > > Ant > > On Tue, Jan 10, 2017 at 8:28 PM, Allard Hoeve <allardho...@gmail.com> wrote: >> Hey, >> >> I reviewed a little (but important) part of the PR. >> >> I'm planning to run the entire branch against an OpenStack provider, >> Digital Ocean and AWS EC2 soon, when I have the time. >> >> Best, >> >> Allard >> >> On Tue, Jan 10, 2017 at 9:46 AM anthony shaw <anthony.p.s...@gmail.com> >> wrote: >> >>> I decided the best thing was to use this : >>> >>> https://github.com/apache/libcloud/pull/970 >>> >>> To verify the changes. It's going to be a complete driver, both >>> compute and storage using a real API. No Mocking, No funny-business. >>> Uses authentication, checks custom headers, file uploads and downloads >>> etc. We can use this in our release process. >>> >>> I've already found 3 bugs using it, including one nasty one. The >>> RawResponse class actually exposed the underlying httplib.HTTPResponse >>> class as a class property. I'm just writing a proxy class for it now. >>> >>> On Tue, Jan 10, 2017 at 7:41 PM, anthony shaw <anthony.p.s...@gmail.com> >>> wrote: >>> > The regenerated provider tables? I had a merge conflict (well, many >>> > merge conflicts) on the way and re-running the table generator is the >>> > safest way to resolve those ones. >>> > >>> > They need running again, since the AWS regions are a dictionary, the >>> > ordering is random. >>> > >>> > On Tue, Jan 10, 2017 at 7:37 PM, Allard Hoeve <allardho...@gmail.com> >>> wrote: >>> >> Hey Anthony, >>> >> >>> >> Wow, that is a lot of work :) Thanks for that. >>> >> >>> >> I started reviewing the branch, but it's 3k+, 3k- and I immediately ran >>> >> into the problem of "reviewing a patch that's too big". For example >>> >> < >>> https://github.com/apache/libcloud/pull/923/files#diff-f9b7105b8fba7a7f1a0ddec6ef14c8feR413 >>> >: >>> >> why is that added in? Seems unrelated, but you must've done that for a >>> >> reason. >>> >> >>> >> So before the review devolves into a bunch of nitpicks, how do you >>> suppose >>> >> we'd review most efficiently? >>> >> >>> >> Maybe you can go through the PR and comment yourself on why certain >>> things >>> >> are as they are? >>> >> >>> >> Best, >>> >> >>> >> Allard >>> >> >>> >> >>> >> On Mon, Jan 9, 2017 at 5:40 AM anthony shaw <anthony.p.s...@gmail.com> >>> >> wrote: >>> >> >>> >>> This pull-request is FINALLY finished! It's taken me the best part of >>> >>> a year to complete it. >>> >>> >>> >>> https://github.com/apache/libcloud/pull/923 >>> >>> >>> >>> This last round of changes I had to completely rewrite the Storage API >>> >>> base classes. I've been testing using my GCP account and uploading and >>> >>> downloading files and it all looks good now. I've gone back and tested >>> >>> a bunch of random other drivers like GoDaddy and Dimension Data for >>> >>> which I have accounts for. >>> >>> >>> >>> I'm going to merge it and raise another PR this week with an >>> >>> integration suite module. This will have a driver and a FLASK web app >>> >>> with a tox definition to test the libcloud library from end to end. >>> >>> The way the httplib_ssl module is mocked out in our unit tests leaves >>> >>> a lot of room for mistakes. >>> >>> >>> >>> Things that visually don't make much sense but I don't have an account >>> to >>> >>> test >>> >>> - The code for Azure Blob leases looked fragile. This really needs >>> >>> testing properly >>> >>> - Multipart uploads should work for storage APIs, S3 has a custom API >>> >>> that is now disabled >>> >>> >>> http://docs.aws.amazon.com/AmazonS3/latest/dev/UsingRESTAPImpUpload.html >>> >>> - The Aliyun OSS driver had some really visible bugs in it, I doubt >>> >>> the existing driver works. We need integration testers for it. >>> >>> >>> >>> >>> >>> >>> >>> On Fri, Jan 6, 2017 at 8:05 PM, Samia, Michel <msa...@netsuite.com> >>> wrote: >>> >>> > <html><bodyFor multipart upload you can use >>> >>> https://pypi.python.org/pypi/requests-toolbelt >>> >>> > >>> >>> > -----Original Message----- >>> >>> > From: anthony shaw [mailto:anthony.p.s...@gmail.com] >>> >>> > Sent: Friday, January 6, 2017 6:07 AM >>> >>> > To: dev <dev@libcloud.apache.org> >>> >>> > Subject: Re: [dev] [DISCUSS] Using requests instead of httplib >>> >>> > >>> >>> > Good news is I figured out a way of implementing the upload >>> >>> functionality using the requests package. >>> >>> > >>> >>> > >>> >>> >>> https://github.com/apache/libcloud/pull/923/commits/5e04dbce554830eca3f9812272076a2fbdbe7cdc >>> >>> > >>> >>> > I've tested it against the Google Cloud Storage account, downloaded >>> and >>> >>> uploaded a file using both the direct file_path option and the option >>> >>> passing a context manager (IOStream or ByteStream). >>> >>> > >>> >>> > The bad news is : >>> >>> > - The S3 multipart upload I've removed. I don't have time right now >>> to >>> >>> implement this feature from scratch >>> >>> > - The unit tests are all coupled to the private methods, the call >>> back >>> >>> system and a bunch of other bad coupling practices, so they are broken >>> BUT >>> >>> it does actually work >>> >>> > >>> >>> > It's nearly there. >>> >>> > >>> >>> > Ant >>> >>> > >>> >>> > On Fri, Jan 6, 2017 at 7:28 AM, anthony shaw < >>> anthony.p.s...@gmail.com> >>> >>> wrote: >>> >>> >> it's more of an existential question :-) >>> >>> >> >>> >>> >> The _upload_object method inside the libcloud.storage.base submodule >>> >>> >> makes a 'raw' call to the LibcloudConnection, which will send the >>> top >>> >>> >> part of the HTTP request then some headers and leave the connection >>> >>> >> open (i.e. not read the response). >>> >>> >> >>> >>> >> Then, depending on the driver, the file and other things, it will >>> >>> >> callback one of the methods like _stream_data, which writes directly >>> >>> >> to the HTTP session using the `send()` method, which is only >>> available >>> >>> >> in httplib. >>> >>> >> >>> >>> >> httplib is a very low level library, requests is very high level. >>> You >>> >>> >> don't get access to the HTTP session directly in requests. >>> >>> >> >>> >>> >> That means that I would have to throw away the code we already have >>> >>> >> (which I am definitely in favour of in the long term since it is >>> >>> >> fragile) and replace it with requests' APIs for doing chunked >>> uploads >>> >>> >> using file streams. >>> >>> >> >>> >>> >> It would probably take me another day or two to finish that >>> >>> implementation. >>> >>> >> >>> >>> >> I always preached that you should change 1 thing at a thing, in >>> small >>> >>> >> amount, and keep testing. So far this has been more like pulling a >>> >>> >> thread on a sweater, I've touched every single file in the code base >>> >>> >> practically! >>> >>> >> The odds are, I will have missed something. So 2.0.0rc1 (if we do >>> call >>> >>> >> it that), despite my best intentions will introduce a new bug just >>> >>> >> based on the number of things I have changed. >>> >>> >> >>> >>> >> On Fri, Jan 6, 2017 at 1:11 AM, Tom Melendez <t...@supertom.com> >>> wrote: >>> >>> >>> Hi Anthony, >>> >>> >>> >>> >>> >>> Nice job getting this going! >>> >>> >>> >>> >>> >>> Would you mind elaborating on this point? >>> >>> >>> "The raw connection still uses httplib. I decided it was too risky >>> to >>> >>> >>> swap that for requests' method of uploading files." >>> >>> >>> >>> >>> >>> Since you're going through the trouble, it would be ideal to go to >>> >>> >>> Requests completely. What's blocking us on the upload code >>> >>> >>> (Admittedly, I haven't studied the upload code)? Anything the >>> >>> community can do to help? >>> >>> >>> >>> >>> >>> Thanks, >>> >>> >>> >>> >>> >>> Tom >>> >>> >>> >>> >>> >>> >>> >>> >>> On Thu, Jan 5, 2017 at 3:43 AM, Chris DeRamus >>> >>> >>> <chris.dera...@gmail.com> >>> >>> >>> wrote: >>> >>> >>> >>> >>> >>>> For what it's worth my company (DivvyCloud) has been using the >>> good >>> >>> >>>> work you've done now for almost six months. We had to make a few >>> >>> >>>> tweaks, but the core code contributed has worked flawlessly across >>> >>> >>>> AWS, Azure, OpenStack, Google, VMware and more. The only issue I >>> >>> >>>> believe that still stands which I've seen is an issue when >>> >>> >>>> LIBCLOUD_DEBUG is set to true. Logging doesn't appear to function >>> >>> >>>> properly, but that may have been addressed since your initial >>> >>> submission last year. >>> >>> >>>> >>> >>> >>>> Nice work on this and we sincerely appreciate the contribution. >>> >>> >>>> >>> >>> >>>> On Thu, Jan 5, 2017 at 12:21 AM, anthony shaw >>> >>> >>>> <anthony.p.s...@gmail.com> >>> >>> >>>> wrote: >>> >>> >>>> >>> >>> >>>> > That package had a dumb error in it, I've since fixed it and >>> >>> >>>> > against a live API (GoDaddy). I've tested the following >>> scenarios >>> >>> >>>> > >>> >>> >>>> > - Applying a custom proxy via the environment variable >>> >>> >>>> > - Using libcloud.security to disable SSL verification >>> >>> >>>> > - Using libcloud.security to set a custom CA certificate >>> >>> >>>> > - Combining all of those scenarios >>> >>> >>>> > - Verification of custom headers applied by the driver using >>> >>> >>>> > Charles Proxy and inspecting the HTTP messages manually >>> >>> >>>> > - Decoding JSON messages - although this still uses the existing >>> >>> >>>> > methods, not the requests own json() decoder. >>> >>> >>>> > >>> >>> >>>> > IMO, this is ready to merge. I would like to test the raw >>> >>> >>>> > connections and file uploads if anyone has an account on one of >>> >>> those providers? >>> >>> >>>> > >>> >>> >>>> > On Thu, Jan 5, 2017 at 8:30 AM, Tomaz Muraus <to...@apache.org> >>> >>> wrote: >>> >>> >>>> > > Thanks for working on this again! >>> >>> >>>> > > >>> >>> >>>> > > Once we get a green light from people testing those changes, I >>> >>> >>>> > > propose >>> >>> >>>> to >>> >>> >>>> > > first roll out v2.0.0-rc1 and eventually after we are happy >>> with >>> >>> >>>> > > the stability call it v2.0.0. >>> >>> >>>> > > >>> >>> >>>> > > On Wed, Jan 4, 2017 at 6:20 AM, anthony shaw >>> >>> >>>> > > <anthony.p.s...@gmail.com >>> >>> >>>> > >>> >>> >>>> > > wrote: >>> >>> >>>> > > >>> >>> >>>> > >> Hi, >>> >>> >>>> > >> >>> >>> >>>> > >> I tried doing this a year ago but put it in the 'too hard' >>> >>> bucket. >>> >>> >>>> > >> I've opened a PR (again) replacing the use of httplib with >>> the >>> >>> >>>> > >> requests package. >>> >>> >>>> > >> >>> >>> >>>> > >> The consequences are: >>> >>> >>>> > >> - Connection.conn_classes is no longer a tuple, it is >>> >>> >>>> > >> Connection.conn_class. There is no separation between a HTTP >>> >>> >>>> > >> and HTTPS connection. I could have just hacked around this >>> but >>> >>> >>>> > >> I updated all the tests instead >>> >>> >>>> > >> - Mock implementations no longer use the tuple as above >>> >>> >>>> > >> - We cannot support Python 3.2 officially anymore. Requests >>> >>> >>>> > >> does not support 3.2 >>> >>> >>>> > >> - The raw connection still uses httplib. I decided it was too >>> >>> >>>> > >> risky to swap that for requests' method of uploading files. >>> >>> >>>> > >> >>> >>> >>>> > >> >>> https://github.com/apache/libcloud/pull/923#partial-pull-mergin >>> >>> >>>> > >> g >>> >>> >>>> > >> >>> >>> >>>> > >> Please remote fetch this branch and test it out on some >>> working >>> >>> >>>> > >> code talking to real APIs. Mocks can only go so far. >>> >>> >>>> > >> >>> >>> >>>> > >> I've uploaded the package here - >>> >>> >>>> > >> http://people.apache.org/~anthonyshaw/libcloud/1.5.0.post0/ >>> >>> >>>> > >> >>> >>> >>>> > >> I would like to get this merged but would like some >>> additional >>> >>> >>>> > >> nods before it gets merged into trunk. >>> >>> >>>> > >> >>> >>> >>>> > >> Ant >>> >>> >>>> > >> >>> >>> >>>> > >>> >>> >>>> >>> >>> > >>> >>> > >>> >>> > NOTICE: This email and any attachments may contain confidential and >>> >>> proprietary information of NetSuite Inc. and is for the sole use of the >>> >>> intended recipient for the stated purpose. Any improper use or >>> distribution >>> >>> is prohibited. If you are not the intended recipient, please notify the >>> >>> sender; do not review, copy or distribute; and promptly delete or >>> destroy >>> >>> all transmitted information. Please note that all communications and >>> >>> information transmitted through this email system may be monitored by >>> >>> NetSuite or its agents and that all incoming email is automatically >>> scanned >>> >>> by a third party spam and filtering service >>> >>> > >>> >>> > </body></html> >>> >>> >>>
