Skip to content

Conversation

Saviq
Copy link
Collaborator

@Saviq Saviq commented Apr 7, 2020

No description provided.

@multipass-ci-bot

This comment has been minimized.

Comment on lines 51 to 59
auto make_ssl_config()
{
QSslConfiguration ssl_config;
ssl_config.setLocalCertificate(QSslCertificate::fromPath("/home/ubuntu/.config/lxc/client.crt")[0]);
QFile keyfile{"/home/ubuntu/.config/lxc/client.key"};
keyfile.open(QIODevice::ReadOnly);
ssl_config.setPrivateKey(QSslKey(keyfile.readAll(), QSsl::Rsa));
ssl_config.setPeerVerifyMode(QSslSocket::VerifyNone);

return ssl_config;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed on IRC, we can set up a QSslConfiguration object in the LXDVirtualMachineFactory ctor and then pass that to each LXDVirtualMachine to avoid having to do the set up at every request.


mp::LXDVirtualMachineFactory::LXDVirtualMachineFactory(const mp::Path& data_dir)
: data_dir{data_dir},
base_url{"https://10.225.118.1:8443/1.0"},
Copy link
Contributor

Choose a reason for hiding this comment

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

We're going to have to think of how to really get the IP address the LXD daemon is listening on. When Multipass becomes a strict snap, we won't have a way to call lxc config get core.https_address to get it, nor can we read it from the lxd's config file itself.

I think in the short term, the best we can do is assume the lxd daemon is listening on 127.0.0.1 and possibly have a multipass config setting to allow the user to override that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will have to be a config option, since the LXD host may potentially be on a different host.

We'll default to localhost in the first place, switch to the socket (to which we can gain access via the lxd snap interface) and in the long run we'll have it configurable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that is basically what I had in mind as well after thinking about this. And since you mentioned it, using localhost there instead of 127.0.0.1 (as I proposed) is probably better.

{
QSslConfiguration ssl_config;
ssl_config.setLocalCertificate(QSslCertificate::fromPath("/home/ubuntu/.config/lxc/client.crt")[0]);
QFile keyfile{"/home/ubuntu/.config/lxc/client.key"};
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a placeholder to change this to use $HOME (or QStandardPaths or changes being introduced in #1467) instead of /home/ubuntu.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We actually will need to generate those and register with LXD instead of using those - we won't get access to them when strict, and they might not exist anyway…

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, that seems even better 😃


#include <fmt/format.h>

#include <QDebug>
Copy link
Contributor

Choose a reason for hiding this comment

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

Extraneous include to be removed.

#include <multipass/format.h>
#include <multipass/logging/log.h>

#include <QDebug>
Copy link
Contributor

Choose a reason for hiding this comment

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

Extraneous include to be removed.

@townsend2010
Copy link
Contributor

As discussed, building tests fails.

@Saviq Saviq force-pushed the lxd-containers-poc branch from 8b1c641 to ee3ccf2 Compare April 8, 2020 09:09
@Saviq Saviq changed the title Lxd containers poc LXD containers PoC Apr 8, 2020
@Saviq Saviq closed this Apr 8, 2020
@Saviq Saviq reopened this Apr 8, 2020
@Saviq
Copy link
Collaborator Author

Saviq commented Apr 8, 2020

Travis?

@Saviq Saviq marked this pull request as ready for review April 8, 2020 09:47
@Saviq Saviq added the no-merge label Apr 8, 2020
@Saviq Saviq force-pushed the lxd-containers-poc branch from c3d4968 to 1211603 Compare April 8, 2020 09:49
@multipass-ci-bot

This comment has been minimized.

@Saviq Saviq marked this pull request as draft April 10, 2020 08:46
@Saviq Saviq removed the no-merge label Apr 10, 2020
Comment on lines 32 to 33
class PowerShell;
class SSHKeyProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't need these 2 forward declarations.

} // namespace

mp::LXDVirtualMachineFactory::LXDVirtualMachineFactory(const mp::Path& data_dir)
: data_dir{data_dir}, base_url{"https://10.225.118.1:8443/1.0"}, manager{std::make_unique<QNetworkAccessManager>()}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the goal is to support https over Unix sockets, but for this PoC, using localhost here would make it easier to try out 😃

@multipass-ci-bot

This comment has been minimized.

@townsend2010
Copy link
Contributor

As I'm digging into this more and more, I'm finding that we made many assumptions in the daemon about there will always be an image vault along with a backing image host. Some functions such as list, info, find, and launch assume there is an image vault/image host to get image information from. Also, launch is closely tied to wanting to download the image first (if it doesn't exist) before it actual goes and creates the instance.

I've given this some thought and although I could hack around this by short circuiting the image vault/image host if LXD is being used, I don't think this is a good long term solution given that we want LXD to be the default driver on Linux at some point. That said, I think the right solution is to introduce a LXDImageHost in which we can query and do image operations via the LXD daemon. It also possible to download the image LXD uses as a separate operation apart from LXD's launch of an instance. This way, we can still keep the same logic the daemon currently uses wrt images.

That said, there are still a few unresolved issues that we need to think about.

  • How do we translate the current CustomImageHost, which includes easy ways to get the Ubuntu minimal images and Ubuntu Core images, into something LXD can use? Does LXD already have some way to get and use these images?
  • How will we support the file:// and http:// style launches from the client? Import them into LXD when a user requests such a launch?

- Remove the waiting logic from lxd_request().  Callers should handle the waiting
  as they see fit.
- Create the lxd driver specific directory.
- Increase lxd_request()'s default timeout to 30 seconds.
@townsend2010 townsend2010 force-pushed the lxd-containers-poc branch from 65e58e5 to 982e2b3 Compare May 6, 2020 12:26
@multipass-ci-bot
Copy link
Collaborator

Snap build available: snap refresh multipass --channel edge/pr1472

@Saviq
Copy link
Collaborator Author

Saviq commented May 7, 2020

As I'm digging into this more and more, I'm finding that we made many assumptions in the daemon about there will always be an image vault along with a backing image host. Some functions such as list, info, find, and launch assume there is an image vault/image host to get image information from. Also, launch is closely tied to wanting to download the image first (if it doesn't exist) before it actual goes and creates the instance.

Well, I didn't make it so that there isn't an image vault/host, did I? Rather that it doesn't download anything? I'm not saying architecturally that's the right thing, but it was the simplest way to get there? Same for launch, the download process is just NOOP, no?

I've given this some thought and although I could hack around this by short circuiting the image vault/image host if LXD is being used, I don't think this is a good long term solution given that we want LXD to be the default driver on Linux at some point.

Explain what would need to change for this short-circuiting?

That said, I think the right solution is to introduce a LXDImageHost in which we can query and do image operations via the LXD daemon. It also possible to download the image LXD uses as a separate operation apart from LXD's launch of an instance. This way, we can still keep the same logic the daemon currently uses wrt images.

That may be, but it would add maintenance for… yeah, the gain is what I'm unsure of.

That said, there are still a few unresolved issues that we need to think about.

* How do we translate the current `CustomImageHost`, which includes easy ways to get the Ubuntu minimal images and Ubuntu Core images, into something LXD can use?  Does LXD already have some way to get and use these images?

* How will we support the `file://` and `http://` style launches from the client?  Import them into LXD when a user requests such a launch?

Here's the full list of how you can ask LXD to create instances, and here's how to create images. For URL-based images, we can pass the URL directly to LXD. For file-based ones, we'll need to upload it to LXD before creating an instance.

I see the URL and file-based images as iterations over the first step.

@townsend2010
Copy link
Contributor

First of all, don't get me wrong, what you did makes it work and for a PoC, is just fine. What I was jotting down here are some things I've come across as I'm working to make this more of a product and transitioning it to virtual machines.

Well, I didn't make it so that there isn't an image vault/host, did I? Rather that it doesn't download anything? I'm not saying architecturally that's the right thing, but it was the simplest way to get there? Same for launch, the download process is just NOOP, no?

Yes, all of that is true. The problem is is that the daemon expects there to be an instance image vault in place as well and that is what is really missing. For example, if you launch a LXD instance, then stop the daemon, and then start it again, the daemon will invalidate that instance because there is no corresponding instance image database and thus, will delete that instance from Multipass's perspective (the instance is still there as far as LXD is concerned). That is just one example.

Another issue is for the download NOOP. We don't provide any user feedback if LXD has to download an image so it looks like the launch is hung. The current way this feedback is done is through the actual download before getting to the virtual machine factory or creating the virtual machine. So we either have to refactor parts to get LXD download feedback back to the client (like somehow passing grpc::ServerWriter<LaunchReply>* server all the way to LXDVirtualMachine) or we do the LXD image download as a separate step from LXD's instance creation. This is possible via similar methods used for lxc image copy ubuntu:_alias_ local:. This way, it's it own operation that we can then get the operation id for and query that for download statistics.

Explain what would need to change for this short-circuiting?

Basically what I mean here is for issues like what I mentioned above, put in if (driver == "lxd") type of logic and do whatever we need to do to make sure it works as expected.

Thinking more about the LXDImageHost solution I mentioned yesterday, that is not the right thing to do. We will still use SimpleStreams directly as we do now and use the info it provides to pass on to LXD which image to use, so UbuntuImageHost is still valid here. The problem is the DefaultVMImageVault. It (along with Daemon) assumes Multipass is in control of all of the images. DefaultVMImageVault either needs to be made completely LXD aware or create a new type of VMImageVault when LXD is being used.

Cool stuff about how to use http and file based launches. But yeah, that should be on a next iteration after introducing LXD driver support. We'll just have to remember to disable that type of launching when the LXD driver is in use by a user.

@Saviq
Copy link
Collaborator Author

Saviq commented May 7, 2020

The problem is is that the daemon expects there to be an instance image vault in place as well and that is what is really missing. For example, if you launch a LXD instance, then stop the daemon, and then start it again, the daemon will invalidate that instance because there is no corresponding instance image database and thus, will delete that instance from Multipass's perspective (the instance is still there as far as LXD is concerned). That is just one example.

Ack - I was missing where the failure case is ;)

Another issue is for the download NOOP. We don't provide any user feedback if LXD has to download an image so it looks like the launch is hung. The current way this feedback is done is through the actual download before getting to the virtual machine factory or creating the virtual machine. So we either have to refactor parts to get LXD download feedback back to the client (like somehow passing grpc::ServerWriter<LaunchReply>* server all the way to LXDVirtualMachine) or we do the LXD image download as a separate step from LXD's instance creation. This is possible via similar methods used for lxc image copy ubuntu:_alias_ local:. This way, it's it own operation that we can then get the operation id for and query that for download statistics.

There is one caveat with that, in that we would have to handle image updates - but I suppose that's exactly what we do…

Thinking more about the LXDImageHost solution I mentioned yesterday, that is not the right thing to do. We will still use SimpleStreams directly as we do now and use the info it provides to pass on to LXD which image to use, so UbuntuImageHost is still valid here. The problem is the DefaultVMImageVault. It (along with Daemon) assumes Multipass is in control of all of the images. DefaultVMImageVault either needs to be made completely LXD aware or create a new type of VMImageVault when LXD is being used.

Right, I can see how LXDImageVault would make sense. I suppose DefaultVMImageVault could also use a rename so it's clearer what it's for ;)

@townsend2010
Copy link
Contributor

There is one caveat with that, in that we would have to handle image updates - but I suppose that's exactly what we do…

I think LXD will still take care of updating the image this way. It's basically doing what LXD does at its launch when it downloads an image, but just as a separate step that we are in control of.

@Saviq
Copy link
Collaborator Author

Saviq commented Jun 17, 2020

This landed in #1533.

@Saviq Saviq closed this Jun 17, 2020
@Saviq Saviq deleted the lxd-containers-poc branch June 17, 2020 07:53
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.

3 participants