Merge lp:~citrix-openstack/nova/xenapi-netinject-prop into lp:~hudson-openstack/nova/trunk

[Andy Southgate]

This is basic network injection for XenServer, and includes:

o Modification of the /etc/network/interfaces file within the image using code taken from and now shared with libvirt_conn. This is for compatibility with legacy Linux images without a guest agent.

o Setting of xenstore keys before instance boot, intended for the XenServer Windows agent. The agent will use these to configure the network at boot-time.

This change does not implement live reconfiguration, which is on another blueprint:

https://blueprints.launchpad.net/nova/+spec/xs-inject-networking

It does include template code to detect the presence of agents and avoid modifying the filesystem if they are injection-capable.

[Jay Pipes (jaypipes)]

Hi Andy!

Before going any further, please set this merge proposal status to Work In Progress and resolve the merge conflict in nova/virt/xenapi/vm_utils.py.

Lemme know if you're uncertain about the process of resolving merge conflicts.

Cheers,
jay

[Andy Southgate (andy-southgate)]

Hi,

Thanks Jay. It looks like lp:nova has moved on since I rebased this
branch. Is it sufficient to rebase to trunk again?

Cheers,

Andy S.

On 02/16/11 18:48, Jay Pipes wrote:
> Hi Andy!
>
> Before going any further, please set this merge proposal status to Work In Progress and resolve the merge conflict in nova/virt/xenapi/vm_utils.py.
>
> Lemme know if you're uncertain about the process of resolving merge conflicts.
>
> Cheers,
> jay
>

[Jay Pipes (jaypipes)]

On Wed, Feb 16, 2011 at 2:01 PM, Andy Southgate
<email address hidden> wrote:
> Thanks Jay.  It looks like lp:nova has moved on since I rebased this
> branch.  Is it sufficient to rebase to trunk again?

It's good practice to have a local copy of the lp:nova trunk, and
then, when you are ready to bzr push your local topic branch to
Launchpad, simply do a bzr merge ../trunk and resolve any conflicts
that might pop up. After you resolve the conflicts in the files (if
any), do a bzr resolve --all and then bzr push your local topic branch
to Launchpad.

As far as "rebasing", not sure you need to do that. bzr has a rebase
plugin, but it's not what you need in this case.

So, in short, the recommended dev process is like so:

# Assume ~/repos/nova is where you have your bzr branches for nova...
cd ~/repos/nova
bzr branch lp:nova trunk
bzr branch trunk topic # Where "topic" is the name you give the branch
you work on (say, xs-netinject-prop)
cd topic
# do work in topic branch...
bzr commit -m "Some comment about your changes"
bzr push lp:~citrix-openstack/nova/topic
# On Launchpad, propose your branch for merging, as you did...
# If review come back as some changes are needed, set the merge
proposal status to Work In Progress
# then, in your local branch, do more work
bzr commit -m "More changes"
cd ../trunk
bzr pull # This pulls all changes merged into lp:nova trunk from the
rest of the Nova contributors into your local branch
cd ../topic
bzr merge ../trunk
# If any conflicts, resolve them and then do bzr resolve --all
bzr commit -m "Merge trunk"
bzr push
# The set your merge proposal back to "Needs Review"

All set :)

Don't worry, once you do it a couple times, it becomes second nature ;)

-jay

[Andy Southgate (andy-southgate)]

Thanks, that was a bit quicker than a rebase!

Andy

[Andy Southgate (andy-southgate)]

Salvatore (salvatore-orlando) is taking over this merge BTW as I'm moving on to other things, so please send question to him if you have them.

[Ewan Mellor (ewanmellor)]

I have merged with trunk again. This branch is ready for review. I have requested a review from Trey Morris, since this branch is related to his xs-inject-networking work. An additional nova-core review would be appreciated.

Thanks!

[Trey Morris (tr3buchet)]

Great tests!

I have some concerns, not the least of which is that you added new functions which write networking data to the xenstore param list and call them just before spawn writes to the xenstore param list. If written to the same location, spawn will blow away all of your changes. It doesn't appear that your function writes them to the same path though so we'd end up with some nasty config duplication. Your function also writes to different paths than spawn does. Those paths work with the agent. I'm not sure at this point what the agent would do with data at other paths (possibly ignore it).

Mounting the vdi also concerns me. We use VHDs and aren't going to be mounting them because we'll be building not only base images but also customer images where they could be using who knows which file systems or even encryption. So there will need to be some sort of branching put in place to determine if you do or do not want to mount the vdi.

There are functions in place such as "write_to_param_xenstore()" in vmops you should use instead of making the calls directly to follow convention set when Ed refactored.

Also in the next hour I'll be proposing merge for the 2nd half of my network_injection branch, it refactors what takes place in spawn and allows network reconfiguration/injection in post spawn situations. I think it makes sense to get this in and then place your branch on top of it and fix any conflicts instead of doing it twice.

Let me know if you want to discuss anything, I'm glad to help.

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

> Great tests!
>
> I have some concerns, not the least of which is that you added new functions
> which write networking data to the xenstore param list and call them just
> before spawn writes to the xenstore param list. If written to the same
> location, spawn will blow away all of your changes. It doesn't appear that
> your function writes them to the same path though so we'd end up with some
> nasty config duplication. Your function also writes to different paths than
> spawn does. Those paths work with the agent. I'm not sure at this point what
> the agent would do with data at other paths (possibly ignore it).
>

I'm not entirely sure whether this duplication is intended or just the result of some previous merge with trunk (I'm thinking of multi-nic-support). However, I agree the fact that they write at different path is a bit weird. I'll investigate this issue.

> Mounting the vdi also concerns me. We use VHDs and aren't going to be mounting
> them because we'll be building not only base images but also customer images
> where they could be using who knows which file systems or even encryption. So
> there will need to be some sort of branching put in place to determine if you
> do or do not want to mount the vdi.
>

Mounting the image for injecting data is a technique which has been used in nova since its early days. In the xenapi implementation, we first stream it into a VDI, and then attach the VDI to the VM in which nova-compute is running. Your point on VHDs and encrypted file systems is more than valid, though. For the same reason we believe the guest agent should be the 'preferred' way for getting network/key configuration into the instance. We are providing injection for 'compatibility' with the libvirt implementation.

Also the way in which is currently done will probably not work even in more trival cases, such as root partition not starting at byte 0 or /etc being mounted on a partition other than the first. As already stated, injection is intended only for 'basic' scenarios.

In the proposed implementation if _mounted_processing fails to mount the image into the vdi, an error message is logged but this does not cause the spawn process to fail. However, I agree we probably need a better mechanism for understanding whether injection should be performed or not. I don't think proposing changes either to the API or instance_types would be a good idea at all. Now that glance is in place we can probably use metadata to this purpose. For instance image_type==simple_machine would mean network injection should occur, whereas image_type==agent_machine would mean the xensore param list should be populated. But probably this is not the rigth place for this discussion!

> There are functions in place such as "write_to_param_xenstore()" in vmops you
> should use instead of making the calls directly to follow convention set when
> Ed refactored.
>

Good point. I will take care of this.

> Also in the next hour I'll be proposing merge for the 2nd half of my
> network_injection branch, it refactors what takes place in spawn and allows
> network reconfiguration/injection in post spawn situations. I think it makes
> sense to get this i...

[Trey Morris (tr3buchet)]

> I'm not entirely sure whether this duplication is intended or just the result of some previous merge
> with trunk (I'm thinking of multi-nic-support). However, I agree the fact that they write at different
> path is a bit weird. I'll investigate this issue.

The paths I used are what the agent is expecting for instance the mac address being lowercase with no punctuation vs all uppercase and full of '_'. That's what I'm referring to. I'm not sure of a reason these should be changed, so I'm all ears if you've got one.

> (mounting the vdi)

First I'm glad it fails quietly. Second, I understand the reason for doing so. It's a great reason. It's something that for sure should be implemented; however, we will never use it in any circumstance, relying on the agent instead. For this reason, I suggest a configuration option. example: --mount-vdi=false. I don't see the point in having more metadata if it will always be true or always be false. Of course, if there are people who want to mount some vdi at spawn and twiddle with the file system, but not others, then that's a different story. I'm happy with either way as long as there is a way to disable it completely.

At this point I feel confident saying that injection of network configuration into the xenstore/xenstore-param-list and the agent call to reset networking is finished. Perhaps preconfigure_xenstore() isn't needed. I'm not nearly as knowledgeable of xenstore as you guys surely are so if I've forgotten something please let me know. I only chose my paths as such because of the agent.

There will be a lot of changes to these areas as i start working on multinic, especially in the inject_network_info() function. Perhaps we can agree to leave writing injection into the xenstore to me and you handle the rest (mounting of vdi's, configuration etc). This way I can proceed with multi-nic without worry that I'll just have to rewrite it.

Also, ff there is anything you'd like me to change about the way I've implemented network injection, please let me know. I've written it in a way that works perfectly for us. I'd like for it to work for you as well.

-tr3buchet

[Salvatore Orlando (salvatore-orlando)]

Acknowledging Trey's review and reverting status to work in progress to ease reviewer's work.

@Trey: I will reply to your latest comments shortly!

[Salvatore Orlando (salvatore-orlando)]

> > I'm not entirely sure whether this duplication is intended or just the
> result of some previous merge
> > with trunk (I'm thinking of multi-nic-support). However, I agree the fact
> that they write at different
> > path is a bit weird. I'll investigate this issue.
>
> The paths I used are what the agent is expecting for instance the mac address
> being lowercase with no punctuation vs all uppercase and full of '_'. That's
> what I'm referring to. I'm not sure of a reason these should be changed, so
> I'm all ears if you've got one.
>

I also don't see any reason for which we should use different paths. The only reason I see for the different paths is that these two branches have evolved independently even though the functionalities they implement are quite overlapping.

> > (mounting the vdi)
>
> First I'm glad it fails quietly. Second, I understand the reason for doing so.
> It's a great reason. It's something that for sure should be implemented;
> however, we will never use it in any circumstance, relying on the agent
> instead. For this reason, I suggest a configuration option. example: --mount-
> vdi=false. I don't see the point in having more metadata if it will always be
> true or always be false. Of course, if there are people who want to mount some
> vdi at spawn and twiddle with the file system, but not others, then that's a
> different story. I'm happy with either way as long as there is a way to
> disable it completely.
>

I was suggesting metadata assuming a scenario in which there might be images with and without agent. However, I agree this scenario does not make a lot of sense after all. As you said, either you use the agent or you don't. So the flag idea is maybe more effective, and easier to implement as well :-)

> At this point I feel confident saying that injection of network configuration
> into the xenstore/xenstore-param-list and the agent call to reset networking
> is finished. Perhaps preconfigure_xenstore() isn't needed. I'm not nearly as
> knowledgeable of xenstore as you guys surely are so if I've forgotten
> something please let me know. I only chose my paths as such because of the
> agent.

I tentatively agree with you that preconfigure_xenstore is not required anymore. I will however double-check the code before taking a decision in this direction.

> There will be a lot of changes to these areas as i start working on multinic,
> especially in the inject_network_info() function. Perhaps we can agree to
> leave writing injection into the xenstore to me and you handle the rest
> (mounting of vdi's, configuration etc). This way I can proceed with multi-nic
> without worry that I'll just have to rewrite it.
>

I see your branch has now been merged. I will update my branch accordingly.
I agree on your approach: maybe if there is anything in preconfigure_xenstore I can you ask you to add it to inject_network_info?

> Also, ff there is anything you'd like me to change about the way I've
> implemented network injection, please let me know. I've written it in a way
> that works perfectly for us. I'd like for it to work for you as well.
>
> -tr3buchet

[Trey Morris (tr3buchet)]

 > I agree on your approach: maybe if there is anything in preconfigure_xenstore I can you ask you to add it to inject_network_info?

Absolutely! Just let me know. First thing I came across was not injecting broadcast address where you guys did. Come up with a list so I can implement them all at once.

-tr3buchet

[Salvatore Orlando (salvatore-orlando)]

State changed to work in progress due to conflicts with current trunk

[Salvatore Orlando (salvatore-orlando)]

Conflicts resolved.
Branch ready for review again.

[Salvatore Orlando (salvatore-orlando)]

Reverting to work in progress to resolve conflicts and integrate IPv6 injection into branch

[Salvatore Orlando (salvatore-orlando)]

Branch updated and again ready for review.

[Rick Harris (rconradharris)]

Nice work, Andy-- really digging the added tests here.

Overall this looks good, just have a few small suggestions that might improve
readability:

> 87 + def fake_network_get_all_by_instance(context, instance_id):
> 88 + l = []
> 89 + l.append(FakeModel(network_fields))
> 90 + return l
> 91 +

Pep8: "Never use the characters `l' (lowercase letter el)". Might be better
as:

    return [FakeModel(network_fields)]

> 153 +def fake_execute_get_log():
> 154 + global _fake_execute_log
> 155 + return _fake_execute_log

`global` isn't needed here since you're not mutating the value.

> +def fake_execute(*cmd, **kwargs):

Since `cmd` is a list, it might be clearer as `cmd_parts` (or something
similar)

> 185 + cmd_map = map(str, cmd)
> 186 + cmd_str = ' '.join(cmd_map)

Going forward, comprehensions are preferred to map/reduce/filter. So, might be
better as:

    cmd_str = ' '.join(str(part) for part in cmd_parts)

> 199 + if isinstance(reply_handler, types.StringTypes):

I think `basestring` is preferred to StringTypes:

    if isinstance(reply_handler, basestring):

> 213 + LOG.debug(_("Reply to faked command is stdout='%(0)s' stderr='%(1)s'") %
> 214 + {'0': reply[0], '1': reply[1]})

Rather than using a position-style, since a dict is being passed, might be
better to attach meaningful names. For example:

    stdout = reply[0]
    stderr = reply[1]
    LOG.debug(_("Reply to faked command is stdout='%(stdout)s'"
                "stderr='%(stderr)s'") % locals())

> 264 + #db_fakes.stub_out_db_network_api(self.stubs)

Vestigial code?

> 389 def _test_spawn(self, image_id, kernel_id, ramdisk_id,
> 394 + instance_type="m1.large", os_type="linux", check_injection=False):

Should probably be lined up with the paren, something like:

    def _test_spawn(self, image_id, kernel_id, ramdisk_id,
                    instance_type="m1.large", os_type="linux",
                    check_injection=False):

> 580 + def f_1(self, interval, now=True):
> 581 + self.f(*self.args, **self.kw)
> 582 + stubs.Set(utils.LoopingCall, 'start', f_1)

Could use a more descriptive name than `f_1`. Perhaps, `fake_start`.

> 640 + t = __import__('Cheetah.Template', globals(), locals(), ['Template'],
> 641 + -1)
> 642 + template = t.Template

Is there a reason you're using __import__ here--if so, it might benefit from a
NOTE(andy):

Otherwise, might be better to do:

    if not template:
        from Cheetah import Template
        template = Template

> 643 + #load template file if necessary

Not a super-useful comment (didn't add anything for me, at least)

> 808 + LOG.debug(_('Raising Failure'))

Not a particularly useful logging line, especially since we raise immediately
afterwards. I'd consider scrapping this.

> 804 + if (ref in _db_content[cls]):
> 805 + if (field in _db_content[cls][ref]):
> 806 + return _db_content[cls][ref][field]
> 807 + else:
> 808 + LOG.debug(_('Raising Failure'))
> 809 + raise Failure(['HANDLE_INVALID', cls, ref])

Might be c...

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

Hi Rick,

thanks for taking time for doing such a detailed review!
Your tips will be very useful to provide higher-quality code in the future.

I will update the branch in the few hours in order to give reviewers a full day before feature freeze to review it.

There are only a few bits which probably deserve some discussion:
>
> Is there a reason you're using __import__ here--if so, it might benefit from a
> NOTE(andy):
>
> Otherwise, might be better to do:
>
> if not template:
> from Cheetah import Template
> template = Template
>

I agrre the syntax you are proposing is much neater, but wouldn't this put a requirement on Cheetah for the whole nova project (e.g.: nova services, unit tests, etc. failing if Cheetah is not installed)?

> > 990 + LOG.debug("ENTERING WAIT FOR BOOT!")

This is vestigial code. I love to use them for debugging code, and I usually throw a pep8 violation in them in order to make sure I remove them before push. Unfortunately I forgot to do the pep8 violation for this one!

[Rick Harris (rconradharris)]

> wouldn't this put a requirement on Cheetah

Since you're importing Cheetah in function and not in the module namespace, we'll only end up importing Cheetah if it's actually needed. So, I think the "from x import y", in this case, works just as well as __import__, just a bit cleaner IMHO.

> and I usually throw a pep8 violation in them in order to make sure I remove them before push

Interesting. I've been meaning to add a SPIKE(author) tag that would cause Hudson to reject patches that include this. You could wrap extra debugs with a # SPIKE comment and be sure it never gets committed.

[Salvatore Orlando (salvatore-orlando)]

All Rick's comments, but one, have been addressed.

The one not addressed is the following:

> 804 + if (ref in _db_content[cls]):
> 805 + if (field in _db_content[cls][ref]):
> 806 + return _db_content[cls][ref][field]
> 807 + else:
> 808 + LOG.debug(_('Raising Failure'))
> 809 + raise Failure(['HANDLE_INVALID', cls, ref])

Might be clearer with a EAFP style, like:

    try:
        return _db_content[cls][ref][field]
    except KeyError:
        raise Failure(['HANDLE_INVALID', cls, ref])

In the fake xenapi driver we need to distinguish between the following cases:
- entity not found (ref not in _db_content[cls]), in which case we need to raise a HANDLE_INVALID falure
- entity found, but attribute not found, in which case we need to raised a NOT_IMPLEMENTED failure.

Nevertheless, Rick's suggestion is very valid and this calls for a wider refactoring of the fake xenapi driver, starting with the _getter routine. However, I think it would be better to do this after-cactus.

[Sandy Walsh (sandy-walsh)]

Getting XenAPIMigrateInstance (test_xenapi.py) asking for passwords during tests:

XenAPIMigrateInstance
    test_finish_resize [sudo] password for swalsh:
                                [sudo] password for swalsh:
OK

Otherwise, impressive branch ... nothing serious to say.

641 + # it the caller does not provide template object and data

"If the caller ..."

But largely I'm going to trust Trey on ensuring the approach is sound (as he's closer to the problem than I)

Once he approves, I will too.

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

> Getting XenAPIMigrateInstance (test_xenapi.py) asking for passwords during
> tests:
>
> XenAPIMigrateInstance
> test_finish_resize [sudo] password for swalsh:
> [sudo] password for swalsh:
> OK
>

Thanks for spotting this Sandy! Finish_resize calls _create_vm which in turn calls preconfigure_instance (which writes /etc/network/interfaces). We were not stubbing out utils.execute for the migrate test case.

I already pushed updated code, pleasee see if the tests run fine now.
Another way of doing this is disabling the flag xenapi_inject_image for the migrate use case. If you think that would make more sense, I'll do that.

> Otherwise, impressive branch ... nothing serious to say.
>
> 641 + # it the caller does not provide template object and data
>
> "If the caller ..."
>
> But largely I'm going to trust Trey on ensuring the approach is sound (as he's
> closer to the problem than I)
>

get_injectables is a routine 'shared' between libvirt and nova. In the libvirt backend Cheetah.Template is loaded with a dynamic import as a global variable, and the template file is read when the LibVirtConnection instance is initialized.

We don't do that in XenAPI and I'm a bit wary of using global variables, so I put the template class and the template file as optional variables and then dynamically set them if they are not set from the caller (xenapi backend in this case).
> Once he approves, I will too.

[Trey Morris (tr3buchet)]

--- disk.py ----
get_injectables needs a docstring
why is get_injectables in disk? seems like an odd place..

get_injectables troubles me because one of my goals with multi-nic is to untether the network and virt layers by passing all of the data required for network configuration into the virt layer from compute so no network related db lookups are required at the virt layer. In the mean time (before nova-multi-nic), get_network_info() in vmops.py is performing this task. There will be a similar get_network_info() function for each libvirt/xenapi/hyperV. You can see the format of the data compute will pass to the virt layer spawn process by looking at get_network_info() in vmops.py. Is there something you need that this function isn't handling? I'm not sure I fully grasp the goal of get_injectables().

This should also apply to the fake calls for network info in tests/db/fakes.py. The xenapi/libvirt/hyperV tests shouldn't be using those (but this will surely have to figured out later).

--- vm_utils.py ---
_find_guest_agent needs a docstring

--- vmops.py ---
yay!!

--- xenapi_conn.py ---
1058 + ' network configuration if not injected into the image'
should probably be
1058 + ' network configuration is not injected into the image'
unless i misinterpreted.

The rest is great! I like the way you've implemented mounting vdi as a flaggable option and I also like how if a proper agent is present you preference using it over adding to the mounted filesystem.

review: Needs Fixing

[Salvatore Orlando (salvatore-orlando)]

Hi Trey,
see my comments inline

> --- disk.py ----
> get_injectables needs a docstring
> why is get_injectables in disk? seems like an odd place..
>
> get_injectables troubles me because one of my goals with multi-nic is to
> untether the network and virt layers by passing all of the data required for
> network configuration into the virt layer from compute so no network related
> db lookups are required at the virt layer. In the mean time (before nova-
> multi-nic), get_network_info() in vmops.py is performing this task. There will
> be a similar get_network_info() function for each libvirt/xenapi/hyperV. You
> can see the format of the data compute will pass to the virt layer spawn
> process by looking at get_network_info() in vmops.py. Is there something you
> need that this function isn't handling? I'm not sure I fully grasp the goal of
> get_injectables().
>
> This should also apply to the fake calls for network info in
> tests/db/fakes.py. The xenapi/libvirt/hyperV tests shouldn't be using those
> (but this will surely have to figured out later).

When I noted your network_info I realized there was some overlap between get_injectables and it.
get_injectables is in disk.py since it is used by both libvirt and xenapi (and possibly hyperv); it seemed the best place for it because it performs operations related to the disk image.

From what you said, every backend is going to have a _get_network_info, at least until multi_nic hits trunk. In this case I don't see anymore a reason for having that common bit of code, and I will therefore remove it. However, I should still use the interface.template file and populate it with Chetaah. To do so, the best thing I can do is to map the dict created by _get_network_info onto the dict required by the Cheetah template (the dict already contains all the info I need).

Another thing to note is that the Cheetah template does not support multiple IP addresses. For this reason I think it might be worth to use only the first element from the ip/ipv6 lists in the dict created by network_info.

> --- vm_utils.py ---
> _find_guest_agent needs a docstring
>

Will be done

> --- vmops.py ---
> yay!!
>
> --- xenapi_conn.py ---
> 1058 + ' network configuration if not injected into the
> image'
> should probably be
> 1058 + ' network configuration is not injected into the
> image'
> unless i misinterpreted.
>

Typo. Thanks for spotting this!

> The rest is great! I like the way you've implemented mounting vdi as a
> flaggable option and I also like how if a proper agent is present you
> preference using it over adding to the mounted filesystem.

[Salvatore Orlando (salvatore-orlando)]

Hi reviewers!

Updating the branch took a bit longer than expect as in the meanwhile another branch changing the way in which network injection is performed for libvirt landed in trunk!

I adapted my branch following Trey's comments. No more _get_injectables! Now I also use the value returned by _get_network_info. Unlike the libvirt impl, I decided to use the 'info' field in the tuple rather than network_ref. This way, if the db changes in the future we will have to update only _get_network_info accordingly.

Another tangential issue was the rescue feature. In the unit tests spawn_rescue was stubbed out with a nice 'pass', leading to a failure on a later operation on the xenapi fake driver (which has been improved to be less 'fake'). By the way, if spawn_rescue has been stubbed out, what's the point of having a unit test for rescue? Code coverage is not complete at all, and I think the unit test is quite unreliable.

Thanks again for spending so much time on this branch. I hope it is now ready to rest in trunk forever!

[lp:~citrix-openstack/nova/xenapi-netinject-prop]

721. By Salvatore Orlando on 2011-03-25

merge trunk

[Rick Harris (rconradharris)]

As with Sandy, I really can't speak to whether this is the right approach. I'll leave that to Trey.

But from a code-quality perspective, this looks really good. Thanks for the fix-ups!

One small nit (not a deal-breaker):

> 97 + l = []
> 98 + l.append(FakeModel(fixed_ip_fields))
> 99 + return l

Looks like one was missed. I'm okay letting this slide since we can get this later; better to get this merged sooner. That said, if you can slip in a fix here, that'd be great.

review: Approve

[lp:~citrix-openstack/nova/xenapi-netinject-prop]

722. By Salvatore Orlando on 2011-03-25

minor pep8 fix in db/fakes.py

[Salvatore Orlando (salvatore-orlando)]

> As with Sandy, I really can't speak to whether this is the right approach.
> I'll leave that to Trey.
>
> But from a code-quality perspective, this looks really good. Thanks for the
> fix-ups!
>
> One small nit (not a deal-breaker):
>
> > 97 + l = []
> > 98 + l.append(FakeModel(fixed_ip_fields))
> > 99 + return l
>
> Looks like one was missed. I'm okay letting this slide since we can get this
> later; better to get this merged sooner. That said, if you can slip in a fix
> here, that'd be great.

As for the approach, I've think I've addresses Trey's concerns.
Thanks for spotting the missed fix! Updated branch pushed.

[Salvatore Orlando (salvatore-orlando)]

Requesting FF exception for this branch, as it has already 1 approve vote and all discussion points (major and minor) have been addressed according to reviewers' suggestions.

The branch only touches the part of the XenAPI backend for the compute service which deals with spawning VMs. It performs delicate operations such as mounting VDIs, but the routine we use for these operations are already widely used in the XenAPI backend.
Two unit tests, carefully prepared to reduce stubbing-out to a minimum, have been provided.

Benefits:
- this branch will enable support for IP injection in disk images for XenAPI, in the same way in which is already done in libvirt. It indeed uses the same template file. Altough XenAPI already supports agent-based injection, this way of doing injection can be very useful in situations where guest images do not have an agent.
- this branch slightly improves unit test infrastructure for xenapi backend, increasing code coverage.

Risk assessment:
- this branch has a quite difficult life. It was first proposed more than a month ago, but was pushed back due to overlaps with other network-related branches dealing with XenAPI backend. More problems arised when the interfaces.template file changed yesterday to reflect multiple interface. However, now the branch is completely in sync with the current trunk, and since we are in FF period we can expect minimum to no interferences with other branches.

Risk assessment:
- No risk for network manager, as its code has not been touched at all. The xenapi network driver will be used only by the compute node for setting up VLANs on xen hosts. This also means that there cannot be any impact on other capabilities of the Network Manager, such as VPN access or Floating IPs.
- Since this is a new feature, and the associated code is executed only when a compute node uses XenAPI and the VLan manager, there is no risk of breaking libvirt, hyperv, or mware installations, or XenAPI installations which use the flat manager

[Sandy Walsh (sandy-walsh)]

Heh, nearly had a heart attack when I saw the tests failing. Then I saw it was only 'suds' missing. Should have looked in pip-requires.

Nice work!we Hopefully this sneaks it in?

review: Approve

[Salvatore Orlando (salvatore-orlando)]

> Heh, nearly had a heart attack when I saw the tests failing. Then I saw it was
> only 'suds' missing. Should have looked in pip-requires.
>
> Nice work!we Hopefully this sneaks it in?

Hi Sandy,

Are you sure you're not talking about VMware-vSphere? I believe that was the branch with the test failing due to suds...
In this branch your tests were failing because I was not stubbing out utils.execute in the migrate test case (on my dev machines they were not failing because all the commands were in the sudoers file :-)
Anyway, that has been fixed!

[lp:~citrix-openstack/nova/xenapi-netinject-prop]

723. By Salvatore Orlando on 2011-03-25

merge trunk

[Thierry Carrez (ttx)]

This is a bit too wide for my taste but it seems very close to the goal. FFe granted, provided this gets merged very soon (before end of day Monday)

review: Approve (ffe)

[Sandy Walsh (sandy-walsh)]

> Are you sure you're not talking about VMware-vSphere? I believe that was the
> branch with the test failing due to suds...

Could have been just part of the trunk merges. It was definitely this branch. All good now though.

[Trey Morris (tr3buchet)]

great :)

review: Approve

=== modified file 'Authors'
--- Authors	2011-03-24 22:47:36 +0000
+++ Authors	2011-03-25 13:43:59 +0000
@@ -1,4 +1,5 @@
 Andy Smith <[e-mail]>
+Andy Southgate <[e-mail]>
 Anne Gentle <[e-mail]>
 Anthony Young <[e-mail]>
 Antony Messerli <[e-mail]>
 
=== modified file 'nova/tests/db/fakes.py'
--- nova/tests/db/fakes.py	2011-03-17 02:20:18 +0000
+++ nova/tests/db/fakes.py	2011-03-25 13:43:59 +0000
@@ -24,7 +24,7 @@
 from nova import utils
 
 
-def stub_out_db_instance_api(stubs):
+def stub_out_db_instance_api(stubs, injected=True):
     """ Stubs out the db API for creating Instances """
 
     INSTANCE_TYPES = {
@@ -56,6 +56,25 @@
                  flavorid=5,
                  rxtx_cap=5)}
 
+    network_fields = {
+        'id': 'test',
+        'bridge': 'xenbr0',
+        'label': 'test_network',
+        'netmask': '255.255.255.0',
+        'cidr_v6': 'fe80::a00:0/120',
+        'netmask_v6': '120',
+        'gateway': '10.0.0.1',
+        'gateway_v6': 'fe80::a00:1',
+        'broadcast': '10.0.0.255',
+        'dns': '10.0.0.2',
+        'ra_server': None,
+        'injected': injected}
+
+    fixed_ip_fields = {
+        'address': '10.0.0.3',
+        'address_v6': 'fe80::a00:3',
+        'network_id': 'test'}
+
     class FakeModel(object):
         """ Stubs out for model """
         def __init__(self, values):
@@ -76,38 +95,29 @@
     def fake_instance_type_get_by_name(context, name):
         return INSTANCE_TYPES[name]
 
-    def fake_instance_create(values):
-        """ Stubs out the db.instance_create method """
-
-        type_data = INSTANCE_TYPES[values['instance_type']]
-
-        base_options = {
-            'name': values['name'],
-            'id': values['id'],
-            'reservation_id': utils.generate_uid('r'),
-            'image_id': values['image_id'],
-            'kernel_id': values['kernel_id'],
-            'ramdisk_id': values['ramdisk_id'],
-            'state_description': 'scheduling',
-            'user_id': values['user_id'],
-            'project_id': values['project_id'],
-            'launch_time': time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime()),
-            'instance_type': values['instance_type'],
-            'memory_mb': type_data['memory_mb'],
-            'mac_address': values['mac_address'],
-            'vcpus': type_data['vcpus'],
-            'local_gb': type_data['local_gb'],
-            'os_type': values['os_type']}
-
-        return FakeModel(base_options)
-
     def fake_network_get_by_instance(context, instance_id):
-        fields = {
-            'bridge': 'xenbr0',
-            }
-        return FakeModel(fields)
-
-    stubs.Set(db, 'instance_create', fake_instance_create)
+        return FakeModel(network_fields)
+
+    def fake_network_get_all_by_instance(context, instance_id):
+        return [FakeModel(network_fields)]
+
+    def fake_instance_get_fixed_address(context, instance_id):
+        return FakeModel(fixed_ip_fields).address
+
+    def fake_instance_get_fixed_address_v6(context, instance_id):
+        return FakeModel(fixed_ip_fields).address
+
+    def fake_fixed_ip_get_all_by_instance(context, instance_id):
+        return [FakeModel(fixed_ip_fields)]
+
     stubs.Set(db, 'network_get_by_instance', fake_network_get_by_instance)
     stubs.Set(db, 'instance_type_get_all', fake_instance_type_get_all)
     stubs.Set(db, 'instance_type_get_by_name', fake_instance_type_get_by_name)
+    stubs.Set(db, 'instance_get_fixed_address',
+        fake_instance_get_fixed_address)
+    stubs.Set(db, 'instance_get_fixed_address_v6',
+        fake_instance_get_fixed_address_v6)
+    stubs.Set(db, 'network_get_all_by_instance',
+        fake_network_get_all_by_instance)
+    stubs.Set(db, 'fixed_ip_get_all_by_instance',
+        fake_fixed_ip_get_all_by_instance)
 
=== added file 'nova/tests/fake_utils.py'
--- nova/tests/fake_utils.py	1970-01-01 00:00:00 +0000
+++ nova/tests/fake_utils.py	2011-03-25 13:43:59 +0000
@@ -0,0 +1,106 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright (c) 2011 Citrix Systems, Inc.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""This modules stubs out functions in nova.utils
+"""
+
+import re
+import types
+
+from eventlet import greenthread
+
+from nova import exception
+from nova import log as logging
+from nova import utils
+
+LOG = logging.getLogger('nova.tests.fake_utils')
+
+_fake_execute_repliers = []
+_fake_execute_log = []
+
+
+def fake_execute_get_log():
+    return _fake_execute_log
+
+
+def fake_execute_clear_log():
+    global _fake_execute_log
+    _fake_execute_log = []
+
+
+def fake_execute_set_repliers(repliers):
+    """Allows the client to configure replies to commands"""
+    global _fake_execute_repliers
+    _fake_execute_repliers = repliers
+
+
+def fake_execute_default_reply_handler(*ignore_args, **ignore_kwargs):
+    """A reply handler for commands that haven't been added to the reply
+    list.  Returns empty strings for stdout and stderr
+    """
+    return '', ''
+
+
+def fake_execute(*cmd_parts, **kwargs):
+    """This function stubs out execute, optionally executing
+    a preconfigued function to return expected data
+    """
+    global _fake_execute_repliers
+
+    process_input = kwargs.get('process_input', None)
+    addl_env = kwargs.get('addl_env', None)
+    check_exit_code = kwargs.get('check_exit_code', 0)
+    cmd_str = ' '.join(str(part) for part in cmd_parts)
+
+    LOG.debug(_("Faking execution of cmd (subprocess): %s"), cmd_str)
+    _fake_execute_log.append(cmd_str)
+
+    reply_handler = fake_execute_default_reply_handler
+
+    for fake_replier in _fake_execute_repliers:
+        if re.match(fake_replier[0], cmd_str):
+            reply_handler = fake_replier[1]
+            LOG.debug(_('Faked command matched %s') % fake_replier[0])
+            break
+
+    if isinstance(reply_handler, basestring):
+        # If the reply handler is a string, return it as stdout
+        reply = reply_handler, ''
+    else:
+        try:
+            # Alternative is a function, so call it
+            reply = reply_handler(cmd_parts,
+                                  process_input=process_input,
+                                  addl_env=addl_env,
+                                  check_exit_code=check_exit_code)
+        except exception.ProcessExecutionError as e:
+            LOG.debug(_('Faked command raised an exception %s' % str(e)))
+            raise
+
+    stdout = reply[0]
+    stderr = reply[1]
+    LOG.debug(_("Reply to faked command is stdout='%(stdout)s' "
+                "stderr='%(stderr)s'") % locals())
+
+    # Replicate the sleep call in the real function
+    greenthread.sleep(0)
+    return reply
+
+
+def stub_out_utils_execute(stubs):
+    fake_execute_set_repliers([])
+    fake_execute_clear_log()
+    stubs.Set(utils, 'execute', fake_execute)
 
=== modified file 'nova/tests/test_xenapi.py'
--- nova/tests/test_xenapi.py	2011-03-21 18:21:26 +0000
+++ nova/tests/test_xenapi.py	2011-03-25 13:43:59 +0000
@@ -19,11 +19,15 @@
 """
 
 import functools
+import os
+import re
 import stubout
+import ast
 
 from nova import db
 from nova import context
 from nova import flags
+from nova import log as logging
 from nova import test
 from nova import utils
 from nova.auth import manager
@@ -38,6 +42,9 @@
 from nova.tests.db import fakes as db_fakes
 from nova.tests.xenapi import stubs
 from nova.tests.glance import stubs as glance_stubs
+from nova.tests import fake_utils
+
+LOG = logging.getLogger('nova.tests.test_xenapi')
 
 FLAGS = flags.FLAGS
 
@@ -64,13 +71,14 @@
     def setUp(self):
         super(XenAPIVolumeTestCase, self).setUp()
         self.stubs = stubout.StubOutForTesting()
+        self.context = context.RequestContext('fake', 'fake', False)
         FLAGS.target_host = '127.0.0.1'
         FLAGS.xenapi_connection_url = 'test_url'
         FLAGS.xenapi_connection_password = 'test_pass'
         db_fakes.stub_out_db_instance_api(self.stubs)
         stubs.stub_out_get_target(self.stubs)
         xenapi_fake.reset()
-        self.values = {'name': 1, 'id': 1,
+        self.values = {'id': 1,
                   'project_id': 'fake',
                   'user_id': 'fake',
                   'image_id': 1,
@@ -90,7 +98,7 @@
         vol['availability_zone'] = FLAGS.storage_availability_zone
         vol['status'] = "creating"
         vol['attach_status'] = "detached"
-        return db.volume_create(context.get_admin_context(), vol)
+        return db.volume_create(self.context, vol)
 
     def test_create_iscsi_storage(self):
         """ This shows how to test helper classes' methods """
@@ -126,7 +134,7 @@
         stubs.stubout_session(self.stubs, stubs.FakeSessionForVolumeTests)
         conn = xenapi_conn.get_connection(False)
         volume = self._create_volume()
-        instance = db.instance_create(self.values)
+        instance = db.instance_create(self.context, self.values)
         vm = xenapi_fake.create_vm(instance.name, 'Running')
         result = conn.attach_volume(instance.name, volume['id'], '/dev/sdc')
 
@@ -146,7 +154,7 @@
                               stubs.FakeSessionForVolumeFailedTests)
         conn = xenapi_conn.get_connection(False)
         volume = self._create_volume()
-        instance = db.instance_create(self.values)
+        instance = db.instance_create(self.context, self.values)
         xenapi_fake.create_vm(instance.name, 'Running')
         self.assertRaises(Exception,
                           conn.attach_volume,
@@ -175,8 +183,9 @@
         self.project = self.manager.create_project('fake', 'fake', 'fake')
         self.network = utils.import_object(FLAGS.network_manager)
         self.stubs = stubout.StubOutForTesting()
-        FLAGS.xenapi_connection_url = 'test_url'
-        FLAGS.xenapi_connection_password = 'test_pass'
+        self.flags(xenapi_connection_url='test_url',
+                   xenapi_connection_password='test_pass',
+                   instance_name_template='%d')
         xenapi_fake.reset()
         xenapi_fake.create_local_srs()
         db_fakes.stub_out_db_instance_api(self.stubs)
@@ -189,6 +198,8 @@
         stubs.stub_out_vm_methods(self.stubs)
         glance_stubs.stubout_glance_client(self.stubs,
                                            glance_stubs.FakeGlance)
+        fake_utils.stub_out_utils_execute(self.stubs)
+        self.context = context.RequestContext('fake', 'fake', False)
         self.conn = xenapi_conn.get_connection(False)
 
     def test_list_instances_0(self):
@@ -213,7 +224,7 @@
                 if not vm_rec["is_control_domain"]:
                     vm_labels.append(vm_rec["name_label"])
 
-            self.assertEquals(vm_labels, [1])
+            self.assertEquals(vm_labels, ['1'])
 
         def ensure_vbd_was_torn_down():
             vbd_labels = []
@@ -221,7 +232,7 @@
                 vbd_rec = xenapi_fake.get_record('VBD', vbd_ref)
                 vbd_labels.append(vbd_rec["vm_name_label"])
 
-            self.assertEquals(vbd_labels, [1])
+            self.assertEquals(vbd_labels, ['1'])
 
         def ensure_vdi_was_torn_down():
             for vdi_ref in xenapi_fake.get_all('VDI'):
@@ -238,11 +249,10 @@
 
     def create_vm_record(self, conn, os_type):
         instances = conn.list_instances()
-        self.assertEquals(instances, [1])
+        self.assertEquals(instances, ['1'])
 
         # Get Nova record for VM
         vm_info = conn.get_info(1)
-
         # Get XenAPI record for VM
         vms = [rec for ref, rec
                in xenapi_fake.get_all_records('VM').iteritems()
@@ -251,7 +261,7 @@
         self.vm_info = vm_info
         self.vm = vm
 
-    def check_vm_record(self, conn):
+    def check_vm_record(self, conn, check_injection=False):
         # Check that m1.large above turned into the right thing.
         instance_type = db.instance_type_get_by_name(conn, 'm1.large')
         mem_kib = long(instance_type['memory_mb']) << 10
@@ -271,6 +281,25 @@
         # Check that the VM is running according to XenAPI.
         self.assertEquals(self.vm['power_state'], 'Running')
 
+        if check_injection:
+            xenstore_data = self.vm['xenstore_data']
+            key = 'vm-data/networking/aabbccddeeff'
+            xenstore_value = xenstore_data[key]
+            tcpip_data = ast.literal_eval(xenstore_value)
+            self.assertEquals(tcpip_data, {
+                'label': 'test_network',
+                'broadcast': '10.0.0.255',
+                'ips': [{'ip': '10.0.0.3',
+                         'netmask':'255.255.255.0',
+                         'enabled':'1'}],
+                'ip6s': [{'ip': 'fe80::a8bb:ccff:fedd:eeff',
+                          'netmask': '120',
+                          'enabled': '1',
+                          'gateway': 'fe80::a00:1'}],
+                'mac': 'aa:bb:cc:dd:ee:ff',
+                'dns': ['10.0.0.2'],
+                'gateway': '10.0.0.1'})
+
     def check_vm_params_for_windows(self):
         self.assertEquals(self.vm['platform']['nx'], 'true')
         self.assertEquals(self.vm['HVM_boot_params'], {'order': 'dc'})
@@ -304,10 +333,10 @@
         self.assertEquals(self.vm['HVM_boot_policy'], '')
 
     def _test_spawn(self, image_id, kernel_id, ramdisk_id,
-                    instance_type="m1.large", os_type="linux"):
-        stubs.stubout_session(self.stubs, stubs.FakeSessionForVMTests)
-        values = {'name': 1,
-                  'id': 1,
+                    instance_type="m1.large", os_type="linux",
+                    check_injection=False):
+        stubs.stubout_loopingcall_start(self.stubs)
+        values = {'id': 1,
                   'project_id': self.project.id,
                   'user_id': self.user.id,
                   'image_id': image_id,
@@ -316,12 +345,10 @@
                   'instance_type': instance_type,
                   'mac_address': 'aa:bb:cc:dd:ee:ff',
                   'os_type': os_type}
-
-        conn = xenapi_conn.get_connection(False)
-        instance = db.instance_create(values)
-        conn.spawn(instance)
-        self.create_vm_record(conn, os_type)
-        self.check_vm_record(conn)
+        instance = db.instance_create(self.context, values)
+        self.conn.spawn(instance)
+        self.create_vm_record(self.conn, os_type)
+        self.check_vm_record(self.conn, check_injection)
 
     def test_spawn_not_enough_memory(self):
         FLAGS.xenapi_image_service = 'glance'
@@ -362,6 +389,85 @@
                          glance_stubs.FakeGlance.IMAGE_RAMDISK)
         self.check_vm_params_for_linux_with_external_kernel()
 
+    def test_spawn_netinject_file(self):
+        FLAGS.xenapi_image_service = 'glance'
+        db_fakes.stub_out_db_instance_api(self.stubs, injected=True)
+
+        self._tee_executed = False
+
+        def _tee_handler(cmd, **kwargs):
+            input = kwargs.get('process_input', None)
+            self.assertNotEqual(input, None)
+            config = [line.strip() for line in input.split("\n")]
+            # Find the start of eth0 configuration and check it
+            index = config.index('auto eth0')
+            self.assertEquals(config[index + 1:index + 8], [
+                'iface eth0 inet static',
+                'address 10.0.0.3',
+                'netmask 255.255.255.0',
+                'broadcast 10.0.0.255',
+                'gateway 10.0.0.1',
+                'dns-nameservers 10.0.0.2',
+                ''])
+            self._tee_executed = True
+            return '', ''
+
+        fake_utils.fake_execute_set_repliers([
+            # Capture the sudo tee .../etc/network/interfaces command
+            (r'(sudo\s+)?tee.*interfaces', _tee_handler),
+        ])
+        FLAGS.xenapi_image_service = 'glance'
+        self._test_spawn(glance_stubs.FakeGlance.IMAGE_MACHINE,
+                         glance_stubs.FakeGlance.IMAGE_KERNEL,
+                         glance_stubs.FakeGlance.IMAGE_RAMDISK,
+                         check_injection=True)
+        self.assertTrue(self._tee_executed)
+
+    def test_spawn_netinject_xenstore(self):
+        FLAGS.xenapi_image_service = 'glance'
+        db_fakes.stub_out_db_instance_api(self.stubs, injected=True)
+
+        self._tee_executed = False
+
+        def _mount_handler(cmd, *ignore_args, **ignore_kwargs):
+            # When mounting, create real files under the mountpoint to simulate
+            # files in the mounted filesystem
+
+            # mount point will be the last item of the command list
+            self._tmpdir = cmd[len(cmd) - 1]
+            LOG.debug(_('Creating files in %s to simulate guest agent' %
+                self._tmpdir))
+            os.makedirs(os.path.join(self._tmpdir, 'usr', 'sbin'))
+            # Touch the file using open
+            open(os.path.join(self._tmpdir, 'usr', 'sbin',
+                'xe-update-networking'), 'w').close()
+            return '', ''
+
+        def _umount_handler(cmd, *ignore_args, **ignore_kwargs):
+            # Umount would normall make files in the m,ounted filesystem
+            # disappear, so do that here
+            LOG.debug(_('Removing simulated guest agent files in %s' %
+                self._tmpdir))
+            os.remove(os.path.join(self._tmpdir, 'usr', 'sbin',
+                'xe-update-networking'))
+            os.rmdir(os.path.join(self._tmpdir, 'usr', 'sbin'))
+            os.rmdir(os.path.join(self._tmpdir, 'usr'))
+            return '', ''
+
+        def _tee_handler(cmd, *ignore_args, **ignore_kwargs):
+            self._tee_executed = True
+            return '', ''
+
+        fake_utils.fake_execute_set_repliers([
+            (r'(sudo\s+)?mount', _mount_handler),
+            (r'(sudo\s+)?umount', _umount_handler),
+            (r'(sudo\s+)?tee.*interfaces', _tee_handler)])
+        self._test_spawn(1, 2, 3, check_injection=True)
+
+        # tee must not run in this case, where an injection-capable
+        # guest agent is detected
+        self.assertFalse(self._tee_executed)
+
     def test_spawn_with_network_qos(self):
         self._create_instance()
         for vif_ref in xenapi_fake.get_all('VIF'):
@@ -371,6 +477,7 @@
                               str(4 * 1024))
 
     def test_rescue(self):
+        self.flags(xenapi_inject_image=False)
         instance = self._create_instance()
         conn = xenapi_conn.get_connection(False)
         conn.rescue(instance, None)
@@ -391,8 +498,8 @@
 
     def _create_instance(self):
         """Creates and spawns a test instance"""
+        stubs.stubout_loopingcall_start(self.stubs)
         values = {
-            'name': 1,
             'id': 1,
             'project_id': self.project.id,
             'user_id': self.user.id,
@@ -402,7 +509,7 @@
             'instance_type': 'm1.large',
             'mac_address': 'aa:bb:cc:dd:ee:ff',
             'os_type': 'linux'}
-        instance = db.instance_create(values)
+        instance = db.instance_create(self.context, values)
         self.conn.spawn(instance)
         return instance
 
@@ -447,21 +554,26 @@
         db_fakes.stub_out_db_instance_api(self.stubs)
         stubs.stub_out_get_target(self.stubs)
         xenapi_fake.reset()
+        xenapi_fake.create_network('fake', FLAGS.flat_network_bridge)
         self.manager = manager.AuthManager()
         self.user = self.manager.create_user('fake', 'fake', 'fake',
                                              admin=True)
         self.project = self.manager.create_project('fake', 'fake', 'fake')
-        self.values = {'name': 1, 'id': 1,
+        self.context = context.RequestContext('fake', 'fake', False)
+        self.values = {'id': 1,
                   'project_id': self.project.id,
                   'user_id': self.user.id,
                   'image_id': 1,
                   'kernel_id': None,
                   'ramdisk_id': None,
+                  'local_gb': 5,
                   'instance_type': 'm1.large',
                   'mac_address': 'aa:bb:cc:dd:ee:ff',
                   'os_type': 'linux'}
 
+        fake_utils.stub_out_utils_execute(self.stubs)
         stubs.stub_out_migration_methods(self.stubs)
+        stubs.stubout_get_this_vm_uuid(self.stubs)
         glance_stubs.stubout_glance_client(self.stubs,
                                            glance_stubs.FakeGlance)
 
@@ -472,14 +584,15 @@
         self.stubs.UnsetAll()
 
     def test_migrate_disk_and_power_off(self):
-        instance = db.instance_create(self.values)
+        instance = db.instance_create(self.context, self.values)
         stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests)
         conn = xenapi_conn.get_connection(False)
         conn.migrate_disk_and_power_off(instance, '127.0.0.1')
 
     def test_finish_resize(self):
-        instance = db.instance_create(self.values)
+        instance = db.instance_create(self.context, self.values)
         stubs.stubout_session(self.stubs, stubs.FakeSessionForMigrationTests)
+        stubs.stubout_loopingcall_start(self.stubs)
         conn = xenapi_conn.get_connection(False)
         conn.finish_resize(instance, dict(base_copy='hurr', cow='durr'))
 
 
=== modified file 'nova/tests/xenapi/stubs.py'
--- nova/tests/xenapi/stubs.py	2011-03-24 04:09:51 +0000
+++ nova/tests/xenapi/stubs.py	2011-03-25 13:43:59 +0000
@@ -21,6 +21,7 @@
 from nova.virt.xenapi import volume_utils
 from nova.virt.xenapi import vm_utils
 from nova.virt.xenapi import vmops
+from nova import utils
 
 
 def stubout_instance_snapshot(stubs):
@@ -137,14 +138,17 @@
     stubs.Set(vm_utils, '_is_vdi_pv', f)
 
 
+def stubout_loopingcall_start(stubs):
+    def fake_start(self, interval, now=True):
+        self.f(*self.args, **self.kw)
+    stubs.Set(utils.LoopingCall, 'start', fake_start)
+
+
 class FakeSessionForVMTests(fake.SessionBase):
     """ Stubs out a XenAPISession for VM tests """
     def __init__(self, uri):
         super(FakeSessionForVMTests, self).__init__(uri)
 
-    def network_get_all_records_where(self, _1, _2):
-        return self.xenapi.network.get_all_records()
-
     def host_call_plugin(self, _1, _2, _3, _4, _5):
         sr_ref = fake.get_all('SR')[0]
         vdi_ref = fake.create_vdi('', False, sr_ref, False)
@@ -196,7 +200,7 @@
         pass
 
     def fake_spawn_rescue(self, inst):
-        pass
+        inst._rescue = False
 
     stubs.Set(vmops.VMOps, "_shutdown", fake_shutdown)
     stubs.Set(vmops.VMOps, "_acquire_bootlock", fake_acquire_bootlock)
 
=== modified file 'nova/virt/disk.py'
--- nova/virt/disk.py	2011-03-14 17:59:41 +0000
+++ nova/virt/disk.py	2011-03-25 13:43:59 +0000
@@ -26,6 +26,8 @@
 import tempfile
 import time
 
+from nova import context
+from nova import db
 from nova import exception
 from nova import flags
 from nova import log as logging
@@ -38,6 +40,9 @@
                      'minimum size in bytes of root partition')
 flags.DEFINE_integer('block_size', 1024 * 1024 * 256,
                      'block_size to use for dd')
+flags.DEFINE_string('injected_network_template',
+                    utils.abspath('virt/interfaces.template'),
+                    'Template file for injected network')
 flags.DEFINE_integer('timeout_nbd', 10,
                      'time to wait for a NBD device coming up')
 flags.DEFINE_integer('max_nbd_devices', 16,
@@ -97,11 +102,7 @@
                                       % err)
 
             try:
-                if key:
-                    # inject key file
-                    _inject_key_into_fs(key, tmpdir)
-                if net:
-                    _inject_net_into_fs(net, tmpdir)
+                inject_data_into_fs(tmpdir, key, net, utils.execute)
             finally:
                 # unmount device
                 utils.execute('sudo', 'umount', mapped_device)
@@ -164,7 +165,18 @@
     _DEVICES.append(device)
 
 
-def _inject_key_into_fs(key, fs):
+def inject_data_into_fs(fs, key, net, execute):
+    """Injects data into a filesystem already mounted by the caller.
+    Virt connections can call this directly if they mount their fs
+    in a different way to inject_data
+    """
+    if key:
+        _inject_key_into_fs(key, fs, execute=execute)
+    if net:
+        _inject_net_into_fs(net, fs, execute=execute)
+
+
+def _inject_key_into_fs(key, fs, execute=None):
     """Add the given public ssh key to root's authorized_keys.
 
     key is an ssh key string.
@@ -179,7 +191,7 @@
             process_input='\n' + key.strip() + '\n')
 
 
-def _inject_net_into_fs(net, fs):
+def _inject_net_into_fs(net, fs, execute=None):
     """Inject /etc/network/interfaces into the filesystem rooted at fs.
 
     net is the contents of /etc/network/interfaces.
 
=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-03-24 21:26:11 +0000
+++ nova/virt/libvirt_conn.py	2011-03-25 13:43:59 +0000
@@ -76,9 +76,7 @@
 flags.DEFINE_string('rescue_image_id', 'ami-rescue', 'Rescue ami image')
 flags.DEFINE_string('rescue_kernel_id', 'aki-rescue', 'Rescue aki image')
 flags.DEFINE_string('rescue_ramdisk_id', 'ari-rescue', 'Rescue ari image')
-flags.DEFINE_string('injected_network_template',
-                    utils.abspath('virt/interfaces.template'),
-                    'Template file for injected network')
+
 flags.DEFINE_string('libvirt_xml_template',
                     utils.abspath('virt/libvirt.xml.template'),
                     'Libvirt XML Template')
 
=== modified file 'nova/virt/xenapi/fake.py'
--- nova/virt/xenapi/fake.py	2011-03-03 19:13:15 +0000
+++ nova/virt/xenapi/fake.py	2011-03-25 13:43:59 +0000
@@ -162,6 +162,12 @@
     vbd_rec['vm_name_label'] = vm_name_label
 
 
+def after_VM_create(vm_ref, vm_rec):
+    """Create read-only fields in the VM record."""
+    if 'is_control_domain' not in vm_rec:
+        vm_rec['is_control_domain'] = False
+
+
 def create_pbd(config, host_ref, sr_ref, attached):
     return _create_object('PBD', {
         'device-config': config,
@@ -286,6 +292,25 @@
         rec['currently_attached'] = False
         rec['device'] = ''
 
+    def VM_get_xenstore_data(self, _1, vm_ref):
+        return _db_content['VM'][vm_ref].get('xenstore_data', '')
+
+    def VM_remove_from_xenstore_data(self, _1, vm_ref, key):
+        db_ref = _db_content['VM'][vm_ref]
+        if not 'xenstore_data' in db_ref:
+            return
+        db_ref['xenstore_data'][key] = None
+
+    def network_get_all_records_where(self, _1, _2):
+        # TODO (salvatore-orlando):filter table on _2
+        return _db_content['network']
+
+    def VM_add_to_xenstore_data(self, _1, vm_ref, key, value):
+        db_ref = _db_content['VM'][vm_ref]
+        if not 'xenstore_data' in db_ref:
+            db_ref['xenstore_data'] = {}
+        db_ref['xenstore_data'][key] = value
+
     def host_compute_free_memory(self, _1, ref):
         #Always return 12GB available
         return 12 * 1024 * 1024 * 1024
@@ -376,7 +401,6 @@
     def _getter(self, name, params):
         self._check_session(params)
         (cls, func) = name.split('.')
-
         if func == 'get_all':
             self._check_arg_count(params, 1)
             return get_all(cls)
@@ -399,10 +423,11 @@
         if len(params) == 2:
             field = func[len('get_'):]
             ref = params[1]
-
-            if (ref in _db_content[cls] and
-                field in _db_content[cls][ref]):
-                return _db_content[cls][ref][field]
+            if (ref in _db_content[cls]):
+                if (field in _db_content[cls][ref]):
+                    return _db_content[cls][ref][field]
+            else:
+                raise Failure(['HANDLE_INVALID', cls, ref])
 
         LOG.debug(_('Raising NotImplemented'))
         raise NotImplementedError(
@@ -476,7 +501,7 @@
     def _check_session(self, params):
         if (self._session is None or
             self._session not in _db_content['session']):
-            raise Failure(['HANDLE_INVALID', 'session', self._session])
+                raise Failure(['HANDLE_INVALID', 'session', self._session])
         if len(params) == 0 or params[0] != self._session:
             LOG.debug(_('Raising NotImplemented'))
             raise NotImplementedError('Call to XenAPI without using .xenapi')
 
=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2011-03-24 22:31:39 +0000
+++ nova/virt/xenapi/vm_utils.py	2011-03-25 13:43:59 +0000
@@ -22,6 +22,7 @@
 import os
 import pickle
 import re
+import tempfile
 import time
 import urllib
 import uuid
@@ -29,6 +30,8 @@
 
 from eventlet import event
 import glance.client
+from nova import context
+from nova import db
 from nova import exception
 from nova import flags
 from nova import log as logging
@@ -36,6 +39,7 @@
 from nova.auth.manager import AuthManager
 from nova.compute import instance_types
 from nova.compute import power_state
+from nova.virt import disk
 from nova.virt import images
 from nova.virt.xenapi import HelperBase
 from nova.virt.xenapi.volume_utils import StorageError
@@ -670,6 +674,23 @@
                 return None
 
     @classmethod
+    def preconfigure_instance(cls, session, instance, vdi_ref, network_info):
+        """Makes alterations to the image before launching as part of spawn.
+        """
+
+        # As mounting the image VDI is expensive, we only want do do it once,
+        # if at all, so determine whether it's required first, and then do
+        # everything
+        mount_required = False
+        key, net = _prepare_injectables(instance, network_info)
+        mount_required = key or net
+        if not mount_required:
+            return
+
+        with_vdi_attached_here(session, vdi_ref, False,
+                               lambda dev: _mounted_processing(dev, key, net))
+
+    @classmethod
     def lookup_kernel_ramdisk(cls, session, vm):
         vm_rec = session.get_xenapi().VM.get_record(vm)
         if 'PV_kernel' in vm_rec and 'PV_ramdisk' in vm_rec:
@@ -927,6 +948,7 @@
                 e.details[0] == 'DEVICE_DETACH_REJECTED'):
                 LOG.debug(_('VBD.unplug rejected: retrying...'))
                 time.sleep(1)
+                LOG.debug(_('Not sleeping anymore!'))
             elif (len(e.details) > 0 and
                   e.details[0] == 'DEVICE_ALREADY_DETACHED'):
                 LOG.debug(_('VBD.unplug successful eventually.'))
@@ -1002,3 +1024,114 @@
 def get_name_label_for_image(image):
     # TODO(sirp): This should eventually be the URI for the Glance image
     return _('Glance image %s') % image
+
+
+def _mount_filesystem(dev_path, dir):
+    """mounts the device specified by dev_path in dir"""
+    try:
+        out, err = utils.execute('sudo', 'mount',
+                                 '-t', 'ext2,ext3',
+                                 dev_path, dir)
+    except exception.ProcessExecutionError as e:
+        err = str(e)
+    return err
+
+
+def _find_guest_agent(base_dir, agent_rel_path):
+    """
+    tries to locate a guest agent at the path
+    specificed by agent_rel_path
+    """
+    agent_path = os.path.join(base_dir, agent_rel_path)
+    if os.path.isfile(agent_path):
+        # The presence of the guest agent
+        # file indicates that this instance can
+        # reconfigure the network from xenstore data,
+        # so manipulation of files in /etc is not
+        # required
+        LOG.info(_('XenServer tools installed in this '
+                'image are capable of network injection.  '
+                'Networking files will not be'
+                'manipulated'))
+        return True
+    xe_daemon_filename = os.path.join(base_dir,
+        'usr', 'sbin', 'xe-daemon')
+    if os.path.isfile(xe_daemon_filename):
+        LOG.info(_('XenServer tools are present '
+                'in this image but are not capable '
+                'of network injection'))
+    else:
+        LOG.info(_('XenServer tools are not '
+                'installed in this image'))
+    return False
+
+
+def _mounted_processing(device, key, net):
+    """Callback which runs with the image VDI attached"""
+
+    dev_path = '/dev/' + device + '1'  # NB: Partition 1 hardcoded
+    tmpdir = tempfile.mkdtemp()
+    try:
+        # Mount only Linux filesystems, to avoid disturbing NTFS images
+        err = _mount_filesystem(dev_path, tmpdir)
+        if not err:
+            try:
+                # This try block ensures that the umount occurs
+                if not _find_guest_agent(tmpdir, FLAGS.xenapi_agent_path):
+                    LOG.info(_('Manipulating interface files '
+                            'directly'))
+                    disk.inject_data_into_fs(tmpdir, key, net,
+                        utils.execute)
+            finally:
+                utils.execute('sudo', 'umount', dev_path)
+        else:
+            LOG.info(_('Failed to mount filesystem (expected for '
+                'non-linux instances): %s') % err)
+    finally:
+        # remove temporary directory
+        os.rmdir(tmpdir)
+
+
+def _prepare_injectables(inst, networks_info):
+    """
+    prepares the ssh key and the network configuration file to be
+    injected into the disk image
+    """
+    #do the import here - Cheetah.Template will be loaded
+    #only if injection is performed
+    from Cheetah import Template as t
+    template = t.Template
+    template_data = open(FLAGS.injected_network_template).read()
+
+    key = str(inst['key_data'])
+    net = None
+    if networks_info:
+        ifc_num = -1
+        interfaces_info = []
+        for (network_ref, info) in networks_info:
+            ifc_num += 1
+            if not network_ref['injected']:
+                continue
+
+            ip_v4 = ip_v6 = None
+            if 'ips' in info and len(info['ips']) > 0:
+                ip_v4 = info['ips'][0]
+            if 'ip6s' in info and len(info['ip6s']) > 0:
+                ip_v6 = info['ip6s'][0]
+            if len(info['dns']) > 0:
+                dns = info['dns'][0]
+            interface_info = {'name': 'eth%d' % ifc_num,
+                              'address': ip_v4 and ip_v4['ip'] or '',
+                              'netmask': ip_v4 and ip_v4['netmask'] or '',
+                              'gateway': info['gateway'],
+                              'broadcast': info['broadcast'],
+                              'dns': dns,
+                              'address_v6': ip_v6 and ip_v6['ip'] or '',
+                              'netmask_v6': ip_v6 and ip_v6['netmask'] or '',
+                              'gateway_v6': ip_v6 and ip_v6['gateway'] or '',
+                              'use_ipv6': FLAGS.use_ipv6}
+            interfaces_info.append(interface_info)
+        net = str(template(template_data,
+                        searchList=[{'interfaces': interfaces_info,
+                                     'use_ipv6': FLAGS.use_ipv6}]))
+    return key, net
 
=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-03-24 21:11:48 +0000
+++ nova/virt/xenapi/vmops.py	2011-03-25 13:43:59 +0000
@@ -33,6 +33,7 @@
 from nova import log as logging
 from nova import exception
 from nova import utils
+from nova import flags
 
 from nova.auth.manager import AuthManager
 from nova.compute import power_state
@@ -43,6 +44,7 @@
 
 XenAPI = None
 LOG = logging.getLogger("nova.virt.xenapi.vmops")
+FLAGS = flags.FLAGS
 
 
 class VMOps(object):
@@ -53,7 +55,6 @@
         self.XenAPI = session.get_imported_xenapi()
         self._session = session
         self.poll_rescue_last_ran = None
-
         VMHelper.XenAPI = self.XenAPI
 
     def list_instances(self):
@@ -168,6 +169,12 @@
         # create it now. This goes away once nova-multi-nic hits.
         if network_info is None:
             network_info = self._get_network_info(instance)
+
+        # Alter the image before VM start for, e.g. network injection
+        if FLAGS.xenapi_inject_image:
+            VMHelper.preconfigure_instance(self._session, instance,
+                                           vdi_ref, network_info)
+
         self.create_vifs(vm_ref, network_info)
         self.inject_network_info(instance, vm_ref, network_info)
         return vm_ref
@@ -237,26 +244,17 @@
             obj = None
             try:
                 # check for opaque ref
-                obj = self._session.get_xenapi().VM.get_record(instance_or_vm)
+                obj = self._session.get_xenapi().VM.get_uuid(instance_or_vm)
                 return instance_or_vm
             except self.XenAPI.Failure:
-                # wasn't an opaque ref, must be an instance name
+                # wasn't an opaque ref, can be an instance name
                 instance_name = instance_or_vm
 
         # if instance_or_vm is an int/long it must be instance id
         elif isinstance(instance_or_vm, (int, long)):
             ctx = context.get_admin_context()
-            try:
-                instance_obj = db.instance_get(ctx, instance_or_vm)
-                instance_name = instance_obj.name
-            except exception.NotFound:
-                # The unit tests screw this up, as they use an integer for
-                # the vm name. I'd fix that up, but that's a matter for
-                # another bug report. So for now, just try with the passed
-                # value
-                instance_name = instance_or_vm
-
-        # otherwise instance_or_vm is an instance object
+            instance_obj = db.instance_get(ctx, instance_or_vm)
+            instance_name = instance_obj.name
         else:
             instance_name = instance_or_vm.name
         vm_ref = VMHelper.lookup(self._session, instance_name)
@@ -692,7 +690,6 @@
         vm_ref = VMHelper.lookup(self._session, instance.name)
         self._shutdown(instance, vm_ref)
         self._acquire_bootlock(vm_ref)
-
         instance._rescue = True
         self.spawn_rescue(instance)
         rescue_vm_ref = VMHelper.lookup(self._session, instance.name)
@@ -816,6 +813,7 @@
             info = {
                 'label': network['label'],
                 'gateway': network['gateway'],
+                'broadcast': network['broadcast'],
                 'mac': instance.mac_address,
                 'rxtx_cap': flavor['rxtx_cap'],
                 'dns': [network['dns']],
 
=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2011-03-24 08:56:06 +0000
+++ nova/virt/xenapi_conn.py	2011-03-25 13:43:59 +0000
@@ -107,8 +107,22 @@
                      5,
                      'Max number of times to poll for VHD to coalesce.'
                      '  Used only if connection_type=xenapi.')
+flags.DEFINE_bool('xenapi_inject_image',
+                  True,
+                  'Specifies whether an attempt to inject network/key'
+                  '  data into the disk image should be made.'
+                  '  Used only if connection_type=xenapi.')
+flags.DEFINE_string('xenapi_agent_path',
+                    'usr/sbin/xe-update-networking',
+                    'Specifies the path in which the xenapi guest agent'
+                    '  should be located. If the agent is present,'
+                    '  network configuration is not injected into the image'
+                    '  Used only if connection_type=xenapi.'
+                    '  and xenapi_inject_image=True')
+
 flags.DEFINE_string('xenapi_sr_base_path', '/var/run/sr-mount',
                     'Base path to the storage repository')
+
 flags.DEFINE_string('target_host',
                     None,
                     'iSCSI Target Host')