cleanbuild: use pylxd instead of lxd command line #904

kalikiana · 2016-11-15T09:51:43Z

https://bugs.launchpad.net/snapcraft/+bug/1641520

sergiusens

Hi, thanks for working on this, before diving into it, do you mind fixing the tests?

There are some errors in the code like @patch('snapcraft.internal.lxd.Cleanbuilder._container_run')

where it probably is supposed to be @mock.patch or some sort.

I am also interested in knowing what the details are wrt the differences in pylxd between xenial, yakkety and zesty.

Thanks again

kyrofa · 2016-12-08T16:59:02Z

debian/control

@@ -17,6 +17,7 @@ Build-Depends: bash-completion,
               python3-petname,
               python3-pkg-resources,
               python3-progressbar,
+               python3-pylxd,


Does this also need to be in the binary package below? Please also add it to the requirements.txt so our tests can obtain it (seems to be the reason for the failures).

kyrofa · 2016-12-08T17:20:01Z

snapcraft/internal/lxd.py

+import pylxd
+from six.moves.urllib import parse
+from ws4py.client import WebSocketBaseClient
+from ws4py.manager import WebSocketManager


Does this require python3-ws4py?

Ah, I see that's a dependency of pylxd. Should we depend on it directly?

kyrofa · 2016-12-08T17:23:07Z

snapcraft/internal/lxd.py

 from time import sleep
+import pylxd
+from six.moves.urllib import parse


Seeing six in python3-only code made me double-take. Would urllib.parse be better here? If we need six, we should consider making it a direct dependency.

kyrofa · 2016-12-08T17:27:32Z

snapcraft/internal/lxd.py

-        check_call(['lxc', 'exec', self._container_name, '--'] + cmd)
+        if not self._container:
+            raise CalledProcessError(-1, cmd)
+        print('Executing {}'.format(cmd))


Should this be using a logger?

kyrofa · 2016-12-08T17:29:16Z

snapcraft/internal/lxd.py

+        self._pipe(sys.stdin), self._pipe(sys.stdout), self._pipe(sys.stderr)
+        self._manager.start()
+        while len(self._manager) > 0:
+            sleep(.1)


Is there a possibility this will wait forever? Perhaps we should implement a timeout.

We wait til the pipes close, same as pylxd would out of the box. Different builds can take any amount of time, so a hard timeout would to my mind effectively prevent you from snapping certain things.

Fair enough.

kyrofa · 2016-12-08T17:54:26Z

snapcraft/internal/lxd.py


    def _container_run(self, cmd):
-        check_call(['lxc', 'exec', self._container_name, '--'] + cmd)
+        if not self._container:
+            raise CalledProcessError(-1, cmd)


This isn't really an error from subprocess-- someone's trying to run a command without a container, right? Could we provide a more helpful error?

Where "somebody" is test cases which want a reliable error. As a user of snapcraft you won't get this.

kyrofa · 2016-12-08T17:56:42Z

snapcraft/internal/lxd.py

+        if self._io == sys.stdin:
+            return
+
+        if len(message.data) == 0:


A simple if not message.data would likely be more pythonic.

kyrofa · 2016-12-08T18:00:30Z

snapcraft/internal/lxd.py

+                'ephemeral': True,
+                'source': {
+                    'type': 'image',
+                    'fingerprint': fingerprint,


It seems the API can handle 'image': 'ubuntu/xenial'. Can it not also handle the arch? Or is there another benefit to using the fingerprint instead?

I don't see where the API takes a name... fingerprint only takes a fingerprint.

Perhaps I misunderstand, but this is from the docs:

config = {'name': 'my-container', 'source': {'type': 'image', 'image': 'ubuntu/trusty'}} container = client.containers.create(config, wait=True)

Isn't ubuntu/trusty the name, there?

Those docs must be outdated. If you try that you'll get "Must specify one of alias, fingerprint or properties for init from image" (https://github.com/lxc/lxd/blob/master/lxd/containers_post.go#L122).

Huh, indeed! Alright, sorry for the noise.

curl -s --unix-socket /var/snap/lxd/common/lxd/unix.socket lxd/1.0/containers -X POST -d '{"name": "blah", "ephemeral": true, "source": {"type": "image", "mode": "pull", "server": "https://cloud-images.ubuntu.com/releases", "protocol": "simplestreams", "alias": "xenial"}}'

kyrofa · 2016-12-08T18:18:06Z

snapcraft/internal/lxd.py

+        if not self._container:
+            raise CalledProcessError(-1, cmd)
+        print('Executing {}'.format(cmd))
+        # API call because self._container.execute doesn't expose IO streams


Can you explain this a little more? execute() blocks, so this makes me think that you simply want to be able to show scrolling output while the command executes within the container rather than blocking, but then you also pipe stdin. How is that used?

Afair stdin needs to be added to the manager so it can be closed. That's also what pylxd does normally

Okay, thanks for the explanation.

kyrofa · 2016-12-08T18:19:56Z

snapcraft/internal/lxd.py

+        if exit_status is not 0:
+            raise CalledProcessError(exit_status, cmd)
+
+    def _pipe(self, io):


Perhaps this should be called _connect_pipe()? Its return value is never used.

kyrofa · 2016-12-08T18:21:05Z

snapcraft/internal/lxd.py

+        self._parsed = parse.urlparse(
+            self._client.api.operations[operation_id].websocket._api_endpoint)
+        self._manager = WebSocketManager()
+        self._pipe(sys.stdin), self._pipe(sys.stdout), self._pipe(sys.stderr)


Please put these statements on their own lines.

kyrofa · 2016-12-08T18:31:04Z

snapcraft/internal/lxd.py

+            image_name = '{}:{}/{}'.format(
+                props['os'], props['release'], props['architecture'])
+            if name == image_name:
+                return fingerprint


I'm curious why the API is being used directly here instead of iterating over client.images.all() and using its properties, architecture, and fingerprint attributes?

Because that only returns the fingerprints - but we need to pick an image by name.

Correct me if I'm wrong, but does this not do the same thing?

def _get_fingerprint_by_name(self, name): for image in client.images.all(): image_name = '{}:{}/{}'.format( image.properties['os'], image.properties['release'], image.architecture) if image_name == name: return image.fingerprint

Actually that does work. Pushed a slightly modified change. Thanks!

(And in case you're wondering: .architecture and properties['architecture'] are not the same so I couldn't use the former)

Ah okay, makes sense.

kyrofa · 2016-12-13T18:31:58Z

This is looking good to me, but there's a problem in the tests:

Collecting python3-pylxd==2.0.5 (from -r requirements.txt (line 7))

  Could not find a version that satisfies the requirement python3-pylxd==2.0.5 (from -r requirements.txt (line 7)) (from versions: )

No matching distribution found for python3-pylxd==2.0.5 (from -r requirements.txt (line 7))

kyrofa · 2016-12-14T17:17:17Z

Hmm, looks like this needs libssl-dev to build.

This reverts commit 48d49e0.

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

sergiusens · 2017-01-04T21:39:41Z

This is what I get when I want to cleanbuild:

sergiusens@snapcraft:~/snapcraft/demos/godd$ snapcraft cleanbuild --debug
"grade" property not specified: defaulting to "stable"
Setting up part 'godd' with plugin 'go' and properties {'source': 'https://github.com/mvo5/godd', 'go-importpath': 'github.com/mvo5/godd', 'build-packages': ['gcc', 'libgudev-1.0-dev'], 'plugin': 'go', 'snap': [], 'stage': [], 'source-type': 'git', 'prime': []}.
"GET /1.0 HTTP/1.1" 200 None
"GET /1.0/images HTTP/1.1" 200 67
Creating container snapcraft-inadvertently-anthracitic-kip with fingerprint N/A
"POST /1.0/containers HTTP/1.1" 202 463
"GET /1.0/operations/d0a7bdbc-cb8b-4309-a55b-8c2a360b70c0 HTTP/1.1" 200 394
"GET /1.0/operations/d0a7bdbc-cb8b-4309-a55b-8c2a360b70c0/wait HTTP/1.1" 200 394
Stopping snapcraft-inadvertently-anthracitic-kip
Traceback (most recent call last):
  File "/home/sergiusens/snapcraft/bin/snapcraft", line 31, in <module>
    snapcraft.main.main()
  File "/home/sergiusens/snapcraft/snapcraft/main.py", line 236, in main
    return run(args, project_options)
  File "/home/sergiusens/snapcraft/snapcraft/main.py", line 278, in run
    lifecycle.cleanbuild(project_options),
  File "/home/sergiusens/snapcraft/snapcraft/internal/lifecycle.py", line 288, in cleanbuild
    lxd.Cleanbuilder(snap_filename, tar_filename, project_options).execute()
  File "/home/sergiusens/snapcraft/snapcraft/internal/lxd.py", line 171, in execute
    with self._create_container():
  File "/usr/lib/python3.5/contextlib.py", line 59, in __enter__
    return next(self.gen)
  File "/home/sergiusens/snapcraft/snapcraft/internal/lxd.py", line 161, in _create_container
    }, wait=True)
  File "/home/sergiusens/.local/lib/python3.5/site-packages/pylxd/models/container.py", line 132, in create
    client.operations.wait_for_operation(response.json()['operation'])
  File "/home/sergiusens/.local/lib/python3.5/site-packages/pylxd/models/operation.py", line 29, in wait_for_operation
    operation.wait()
  File "/home/sergiusens/.local/lib/python3.5/site-packages/pylxd/models/operation.py", line 51, in wait
    raise exceptions.LXDAPIException(response)
pylxd.exceptions.LXDAPIException: not found

sergiusens · 2017-01-04T21:42:34Z

Apparently I have no fingerprints available

sergiusens · 2017-02-06T15:19:14Z

closing due to lack of activity. Also pylxd in xenial differs greatly from the on in latter series of ubuntu

kalikiana · 2017-02-06T15:36:54Z

Unfortunate to see this being closed, I had been waiting for some review/ help with the tests - the code has worked well for me.
Not sure how pylxd xenial/others is relevant, as far as I'm aware Xenial is the only relevant target for stable snaps anyway.

sergiusens changed the title ~~WIP: Use pylxd instead of lxd command line (LP: #1641520)~~ WIP: Use pylxd instead of lxd command line Nov 15, 2016

sergiusens changed the title ~~WIP: Use pylxd instead of lxd command line~~ WIP: cleanbuild: use pylxd instead of lxd command line Nov 15, 2016

kalikiana changed the title ~~WIP: cleanbuild: use pylxd instead of lxd command line~~ cleanbuild: use pylxd instead of lxd command line Nov 25, 2016

sergiusens reviewed Nov 30, 2016

View reviewed changes

kyrofa reviewed Dec 8, 2016

View reviewed changes

sergiusens closed this Dec 12, 2016

sergiusens reopened this Dec 12, 2016

kalikiana added 11 commits December 14, 2016 10:28

Use pylxd instead of lxd command line

403c3b4

Custom exec implementation and image name lookup

d0ba67b

Drop unused imports from lxd.py

bdc526b

Handle exit status and and adapt check_call-based tests

e956fcf

Update one more test to remove check_call mock

70645ee

Fix flake8 assertions

4fc473b

Depend on pylxd and ws4py

f6f4e45

Avoid helper function to create pipes

3352295

Don't use six but urllib

053a60f

Use client.images.all() to find the image

c51eac9

Avoid obstructing exceptions creating a container

15b29b6

kalikiana added 2 commits December 14, 2016 10:28

Use newer pylxd 2.1.1 in test requirements

9a08762

Use latest pylxd and ws4py from pip

5e59aca

kalikiana force-pushed the pylxd branch from 333b82e to 5e59aca Compare December 14, 2016 09:30

kalikiana added 2 commits December 16, 2016 12:58

Depend on libssl-dev

48d49e0

Log errors while trying LXD sockets

9278566

This was referenced Dec 23, 2016

Snap snapcraft (based on pylxd branch) #929

Closed

Cloudbuild (based on pylxd branch) #1013

Closed

sergiusens added 3 commits January 4, 2017 21:13

Revert "Depend on libssl-dev"

9b7b7d9

This reverts commit 48d49e0.

Fix libssl-dev dependency

73ce6cb

Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

Merge remote-tracking branch 'origin/master' into pylxd

3cdd0e4

sergiusens closed this Feb 6, 2017

cleanbuild: use pylxd instead of lxd command line #904

cleanbuild: use pylxd instead of lxd command line #904

Conversation

kalikiana commented Nov 15, 2016

sergiusens left a comment

Choose a reason for hiding this comment

kyrofa Dec 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Dec 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa Dec 8, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa commented Dec 13, 2016

kyrofa commented Dec 14, 2016

sergiusens commented Jan 4, 2017

sergiusens commented Jan 4, 2017

sergiusens commented Feb 6, 2017

kalikiana commented Feb 6, 2017 • edited

kyrofa Dec 8, 2016 •

edited

kyrofa Dec 8, 2016 •

edited

kyrofa Dec 8, 2016 •

edited

kalikiana commented Feb 6, 2017 •

edited