[nova] Per-instance serial number implementation question
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value. The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2]. After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec: image: hw_guest_serial=unique flavor: hw:guest_serial=unique If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today. I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans. [1] https://review.openstack.org/#/c/612531/2/specs/stein/approved/per-instance-... [2] https://review.openstack.org/#/c/619953/7/nova/virt/libvirt/driver.py@4894 -- Thanks, Matt
Yes, enum sounds much clearer to me. It is similar to hw:cpu_policy or hw_cpu_policy. So why not :) From Fan’s plastic iPhone
在 2019年1月24日，23:13，Matt Riedemann <mriedemos@gmail.com> 写道：
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
[1] https://review.openstack.org/#/c/612531/2/specs/stein/approved/per-instance-... [2] https://review.openstack.org/#/c/619953/7/nova/virt/libvirt/driver.py@4894
--
Thanks,
Matt
On Thu, 24 Jan 2019 09:09:07 -0600, Matt Riedemann <mriedemos@gmail.com> wrote:
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
I think use of the enum makes sense and it happens to make it easier to reason about in conjunction with the [libvirt]/sysinfo_serial config option, at least for me. -melanie
On 01/24/2019 09:20 PM, melanie witt wrote:
On Thu, 24 Jan 2019 09:09:07 -0600, Matt Riedemann <mriedemos@gmail.com> wrote:
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
I think use of the enum makes sense and it happens to make it easier to reason about in conjunction with the [libvirt]/sysinfo_serial config option, at least for me.
+1 Though as I've noted on the spec, I have no idea why we even need yet another extra-spec/image metadata item (particularly a libvirt-specific one leaking out of the API. Why can't we just always set the libvirt sysinfo_serial number to the instance's UUID and be done with it? Best, -jay
On Thu, 2019-01-24 at 09:09 -0600, Matt Riedemann wrote:
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
Makes sense - we do that for 'hw_cpu_policy', for example (we have 'dedicated' and 'shared', but they're essentially booleans). However, the reason we added 'hw:cpu_policy=shared' (the flavor extra spec) was to provide a way for operators to prevent users requesting pinned CPUs via images metadata if they didn't want them in their cloud. At the risk of rehasing what's in the spec, if there aren't cases where users would want 'hw_unique_serial=False' then what is the upside of ever supporting non-unique serials going forward? Could we not just default to unique serials, avoiding these knobs? Stephen
[1] https://review.openstack.org/#/c/612531/2/specs/stein/approved/per-instance-... [2] https://review.openstack.org/#/c/619953/7/nova/virt/libvirt/driver.py@4894
On 1/25/2019 9:03 AM, Stephen Finucane wrote:
At the risk of rehasing what's in the spec, if there aren't cases where users would want 'hw_unique_serial=False' then what is the upside of ever supporting non-unique serials going forward? Could we not just default to unique serials, avoiding these knobs?
The answer to this, and Jay's same question elsewhere in this thread, is likely going to need to come from danpb who I think originally added the serial stuff in the libvirt driver. I could not find discussions about why the serial was per-host rather than per-guest in the original code changes. So besides "this is the way it's always been" and I'm risk averse to backward incompatible changes, I don't have a good answer. -- Thanks, Matt
On Fri, 2019-01-25 at 10:09 -0600, Matt Riedemann wrote:
On 1/25/2019 9:03 AM, Stephen Finucane wrote:
At the risk of rehasing what's in the spec, if there aren't cases where users would want 'hw_unique_serial=False' then what is the upside of ever supporting non-unique serials going forward? Could we not just default to unique serials, avoiding these knobs?
The answer to this, and Jay's same question elsewhere in this thread, is likely going to need to come from danpb who I think originally added the serial stuff in the libvirt driver. I could not find discussions about why the serial was per-host rather than per-guest in the original code changes. So besides "this is the way it's always been" and I'm risk averse to backward incompatible changes, I don't have a good answer.
Chatted with Dan about this. In summary, this value is up to apps (i.e. nova) to populate. That said, as noted in the spec, nova currently populates this using the value of the host OS' '/etc/machine-id' file. It is possible that operators/users are using this to determine if two guests are co-located. He noted that one would be a valid point in claiming the host OS identity should have been reported in 'chassis.serial' instead of 'system.serial' in the first place [1] but changing it now is definitely not zero risk. My personal take on that is that we can avoid the configurable option and it might be good to start reporting 'chassis.serial' in case anyone was doing the above (assuming we care about that possible use case). We'd just need to make sure the change in behaviour was fully documented by way of an 'upgrade' reno. That's just my take though. Stephen [1] https://libvirt.org/formatdomain.html#elementsSysinfo
On Fri, 2019-01-25 at 16:35 +0000, Stephen Finucane wrote:
On Fri, 2019-01-25 at 10:09 -0600, Matt Riedemann wrote:
On 1/25/2019 9:03 AM, Stephen Finucane wrote:
At the risk of rehasing what's in the spec, if there aren't cases where users would want 'hw_unique_serial=False' then what is the upside of ever supporting non-unique serials going forward? Could we not just default to unique serials, avoiding these knobs?
The answer to this, and Jay's same question elsewhere in this thread, is likely going to need to come from danpb who I think originally added the serial stuff in the libvirt driver. I could not find discussions about why the serial was per-host rather than per-guest in the original code changes. So besides "this is the way it's always been" and I'm risk averse to backward incompatible changes, I don't have a good answer.
Chatted with Dan about this. In summary, this value is up to apps (i.e. nova) to populate. That said, as noted in the spec, nova currently populates this using the value of the host OS' '/etc/machine-id' file. It is possible that operators/users are using this to determine if two guests are co-located. you can get the hashed host-id form the instance metadata to determin if two instance are on the same host from a tenant perspective in a hypervior indepent way so i think that usecase would still be supported.
He noted that one would be a valid point in claiming the host OS identity should have been reported in 'chassis.serial' instead of 'system.serial' in the first place [1] but changing it now is definitely not zero risk.
My personal take on that is that we can avoid the configurable option and it might be good to start reporting 'chassis.serial' in case anyone was doing the above (assuming we care about that possible use case). We'd just need to make sure the change in behaviour was fully documented by way of an 'upgrade' reno. That's just my take though.
Stephen
On 1/25/2019 10:47 AM, Sean Mooney wrote:
you can get the hashed host-id form the instance metadata to determin if two instance are on the same host from a tenant perspective in a hypervior indepent way so i think that usecase would still be supported.
I was thinking the same thing. Not even just instance metadata (API) / config drive, it's right in the REST API for the server resource (the hostId field). -- Thanks, Matt
On 1/25/2019 10:35 AM, Stephen Finucane wrote:
My personal take on that is that we can avoid the configurable option and it might be good to start reporting 'chassis.serial' in case anyone was doing the above (assuming we care about that possible use case). We'd just need to make sure the change in behaviour was fully documented by way of an 'upgrade' reno. That's just my take though.
I'm on this page as well. No need for flags, just do it. Chris
On 1/25/2019 10:35 AM, Stephen Finucane wrote:
He noted that one would be a valid point in claiming the host OS identity should have been reported in 'chassis.serial' instead of 'system.serial' in the first place [1] but changing it now is definitely not zero risk.
If I'm reading those docs correctly, chassis.serial was new in libvirt 4.1.0 which is quite a bit newer than our minimum libvirt version support. -- Thanks, Matt
On Fri, 2019-01-25 at 18:52 -0600, Matt Riedemann wrote:
On 1/25/2019 10:35 AM, Stephen Finucane wrote:
He noted that one would be a valid point in claiming the host OS identity should have been reported in 'chassis.serial' instead of 'system.serial' in the first place [1] but changing it now is definitely not zero risk.
If I'm reading those docs correctly, chassis.serial was new in libvirt 4.1.0 which is quite a bit newer than our minimum libvirt version support.
Good point. Guess it doesn't matter though if we have the two alternatives you and Sean have suggested for figuring this stuff out? The important thing is that release note. Setting 'chassis.serial' would be a nice TODO if we have 4.1.0. Stephen
Thanks alot for bring this up, if we decided to make unique serial the only choice, I guess we have to sort on what curcumstances it will change the serial of instances that already exists. Should we have a way to preserve the serial for exisiting instances in order to not cause any workload failue for our customers as changing the serial may cause some problem. On Sat, Jan 26, 2019 at 7:30 PM Stephen Finucane <sfinucan@redhat.com> wrote:
On Fri, 2019-01-25 at 18:52 -0600, Matt Riedemann wrote:
On 1/25/2019 10:35 AM, Stephen Finucane wrote:
He noted that one would be a valid point in claiming the host OS identity should have been reported in 'chassis.serial' instead of 'system.serial' in the first place [1] but changing it now is definitely not zero risk.
If I'm reading those docs correctly, chassis.serial was new in libvirt 4.1.0 which is quite a bit newer than our minimum libvirt version support.
Good point. Guess it doesn't matter though if we have the two alternatives you and Sean have suggested for figuring this stuff out? The important thing is that release note. Setting 'chassis.serial' would be a nice TODO if we have 4.1.0.
Stephen
On Thu, 2019-01-31 at 19:31 +0800, Zhenyu Zheng wrote:
Thanks alot for bring this up, if we decided to make unique serial the only choice, I guess we have to sort on what curcumstances it willchange the serial of instances that already exists. Should we have a way to preserve the serial for exisiting instances in order to not cause any workload failue for our customers as changing the serial may cause some problem.
I think all that's necessary here is to add a reno calling out this change in behavior (along with the alternatives put forth by Sean and Matt) and, ideally, start setting 'chassis.serial' if libvirt > 4.1.0? Stephen
On Sat, Jan 26, 2019 at 7:30 PM Stephen Finucane <sfinucan@redhat.com
wrote: On Fri, 2019-01-25 at 18:52 -0600, Matt Riedemann wrote:
On 1/25/2019 10:35 AM, Stephen Finucane wrote:
He noted that one would be a valid point in
claiming the host OS identity should have been reported in
'chassis.serial' instead of 'system.serial' in the first place [1] but
changing it now is definitely not zero risk.
If I'm reading those docs correctly, chassis.serial was new in libvirt
4.1.0 which is quite a bit newer than our minimum libvirt version support.
Good point. Guess it doesn't matter though if we have the two
alternatives you and Sean have suggested for figuring this stuff out?
The important thing is that release note. Setting 'chassis.serial'
would be a nice TODO if we have 4.1.0.
Stephen
On Thu, 2019-01-31 at 11:55 +0000, Stephen Finucane wrote:
On Thu, 2019-01-31 at 19:31 +0800, Zhenyu Zheng wrote:
Thanks alot for bring this up, if we decided to make unique serial the only choice, I guess we have to sort on what curcumstances it will change the serial of instances that already exists. Should we have a way to preserve the serial for exisiting instances in order to not cause any workload failue for our customers as changing the serial may cause some problem.
I think all that's necessary here is to add a reno calling out this change in behavior (along with the alternatives put forth by Sean and Matt) and, ideally, start setting 'chassis.serial' if libvirt > 4.1.0? since the workloads would already have to tolerate the serial changingn after a migration anyway i think we should be fine with jsut the reno as stephen says.
Stephen
On Sat, Jan 26, 2019 at 7:30 PM Stephen Finucane <sfinucan@redhat.com> wrote:
On Fri, 2019-01-25 at 18:52 -0600, Matt Riedemann wrote:
On 1/25/2019 10:35 AM, Stephen Finucane wrote:
He noted that one would be a valid point in claiming the host OS identity should have been reported in 'chassis.serial' instead of 'system.serial' in the first place [1] but changing it now is definitely not zero risk.
If I'm reading those docs correctly, chassis.serial was new in libvirt 4.1.0 which is quite a bit newer than our minimum libvirt version support.
Good point. Guess it doesn't matter though if we have the two alternatives you and Sean have suggested for figuring this stuff out? The important thing is that release note. Setting 'chassis.serial' would be a nice TODO if we have 4.1.0.
Stephen
On 01/25/2019 10:03 AM, Stephen Finucane wrote:
On Thu, 2019-01-24 at 09:09 -0600, Matt Riedemann wrote:
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
Makes sense - we do that for 'hw_cpu_policy', for example (we have 'dedicated' and 'shared', but they're essentially booleans). However, the reason we added 'hw:cpu_policy=shared' (the flavor extra spec) was to provide a way for operators to prevent users requesting pinned CPUs via images metadata if they didn't want them in their cloud. At the risk of rehasing what's in the spec, if there aren't cases where users would want 'hw_unique_serial=False' then what is the upside of ever supporting non-unique serials going forward? Could we not just default to unique serials, avoiding these knobs?
Right, which is precisely what I commented on the spec. :) I see no valid reason to not just always set the sysinfo_serial to the instance UUID and be done with it. -jay
I'm going to top post and try to summarize where we are on this thread since I have it on today's nova meeting agenda under the "stuck reviews" section. * The email started as a proposal to change the proposed image property and flavor extra spec from a confusing boolean to an enum. * The question was raised why even have any other option than a unique serial number for all instances based on the instance UUID. * Stephen asked Daniel Berrange (danpb) about the history of the [libvirt]/sysinfo_serial configuration option and it sounds like it was mostly added as a way to determine guests running on the same host, which can already be determined using the hostId parameter in the REST API (hostId is the hashed instance.host + instance.project_id so it's not exactly the same since it's unique per host and project, not just host). However, the hostId is not exposed to the guest in the metadata API / config drive - so that could be a regression for applications that used this somehow to calculate affinity within the guest based on the serial (note that mnaser has a patch to expose hostId in the metadata API / config drive [1]). * danpb said the system.serial we set today should really be chassis.serial but that's only available in libvirt >= 4.1.0 and our current minimum required version of libvirt is 1.3.1 so setting chassis.serial would have to be conditional on the running version of libvirt (this is common in that driver). * Applications that depend on the serial number within the guest were not guaranteed it would be unique or not change because migrating the guest to another host would change the serial number anyway (that's the point of the blueprint - to keep the serial unchanged for each guest), so if we just changed to always using unique serial numbers everywhere it should probably be OK (and tolerated/expected by guest applications). * Clearly we would have a release note if we change this behavior but keep in mind that end users are not reading release notes, and none of this is documented today anyway outside of the [libvirt]/sysinfo_serial config option. So a release note would really only help an operator or support personal if they get a ticket due to the change in behavior (which we probably wouldn't hear about upstream for 2+ years given how slow openstack deployments upgrade). So where are we? If we want the minimal amount of behavior change as possible then we just add the new image property / flavor extra spec / config option choice, but that arguably adds technical debt and virt-driver specific behavior to the API (again, that's not uncommon though). If we want to simplify, we don't add the image property / flavor extra spec. But what do we do about the existing config option? Do we add the 'unique' choice, make it the default, and then deprecate the option to at least signal the change is coming in Train? Or do we just deprecate the option in Stein and completely ignore it, always setting the unique serial number as the instance.uuid (and set the host serial in chassis.serial if libvirt>=4.1.0)? In addition, do we expose hostId in the metadata API / config drive via [1] so there is a true alternative *from within the guest* to determine guest affinity on the same host? I'm personally OK with [1] if there is some user documentation around it (as noted in the review). If we are not going to add the new image property / extra spec, my personal choice would be to: - Add the 'unique' choice to the [libvirt]/sysinfo_serial config option and make it the default for new deployments. - Deprecate the sysinfo_serial config option in Stein and remove it in Train. This would give at least some window of time for transition and/or raising a stink if someone thinks we should leave the old per-host behavior. - Merge mnaser's patch to expose hostId in the metadata API and config drive so users still have a way within the guest to determine that affinity for servers in the same project on the same host. What do others think? [1] https://review.openstack.org/#/c/577933/ On 1/24/2019 9:09 AM, Matt Riedemann wrote:
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
[1] https://review.openstack.org/#/c/612531/2/specs/stein/approved/per-instance-...
[2] https://review.openstack.org/#/c/619953/7/nova/virt/libvirt/driver.py@4894
-- Thanks, Matt
I'm going to top post and try to summarize where we are on this thread since I have it on today's nova meeting agenda under the "stuck reviews" section.
* The email started as a proposal to change the proposed image property and flavor extra spec from a confusing boolean to an enum.
* The question was raised why even have any other option than a unique serial number for all instances based on the instance UUID.
* Stephen asked Daniel Berrange (danpb) about the history of the [libvirt]/sysinfo_serial configuration option and it sounds like it was mostly added as a way to determine guests running on the same host, which can already be determined using the hostId parameter in the REST API (hostId is the hashed instance.host + instance.project_id so it's not exactly the same since it's unique per host and project, not just host). However, the hostId is not exposed to the guest in the metadata API / config drive - so that could be a regression for applications that used this somehow to calculate affinity within the guest based on the serial (note that mnaser has a patch to expose hostId in the metadata API / config drive [1]).
* danpb said the system.serial we set today should really be chassis.serial but that's only available in libvirt >= 4.1.0 and our current minimum required version of libvirt is 1.3.1 so setting chassis.serial would have to be conditional on the running version of libvirt (this is common in that driver).
* Applications that depend on the serial number within the guest were not guaranteed it would be unique or not change because migrating the guest to another host would change the serial number anyway (that's the point of the blueprint - to keep the serial unchanged for each guest), so if we just changed to always using unique serial numbers everywhere it should probably be OK (and tolerated/expected by guest applications).
* Clearly we would have a release note if we change this behavior but keep in mind that end users are not reading release notes, and none of this is documented today anyway outside of the [libvirt]/sysinfo_serial config option. So a release note would really only help an operator or support personal if they get a ticket due to the change in behavior (which we probably wouldn't hear about upstream for 2+ years given how slow openstack deployments upgrade).
So where are we? If we want the minimal amount of behavior change as possible then we just add the new image property / flavor extra spec / config option choice, but that arguably adds technical debt and virt-driver specific behavior to the API (again, that's not uncommon though).
If we want to simplify, we don't add the image property / flavor extra spec. But what do we do about the existing config option?
Do we add the 'unique' choice, make it the default, and then deprecate the option to at least signal the change is coming in Train?
Or do we just deprecate the option in Stein and completely ignore it, always setting the unique serial number as the instance.uuid (and set the host serial in chassis.serial if libvirt>=4.1.0)?
On Thu, 2019-01-31 at 09:00 -0600, Matt Riedemann wrote: personally i would do ^ assuming we also do v
In addition, do we expose hostId in the metadata API / config drive via [1] so there is a true alternative *from within the guest* to determine guest affinity on the same host? I'm personally OK with [1] if there is some user documentation around it (as noted in the review).
If we are not going to add the new image property / extra spec, my personal choice would be to:
- Add the 'unique' choice to the [libvirt]/sysinfo_serial config option and make it the default for new deployments. - Deprecate the sysinfo_serial config option in Stein and remove it in Train. This would give at least some window of time for transition and/or raising a stink if someone thinks we should leave the old per-host behavior. - Merge mnaser's patch to expose hostId in the metadata API and config drive so users still have a way within the guest to determine that affinity for servers in the same project on the same host.
What do others think?
yes i think your personal choice above makes sense too so i would be +1 on that too as it gives a cycle for people to move if they care about the serials.
[1] https://review.openstack.org/#/c/577933/
On 1/24/2019 9:09 AM, Matt Riedemann wrote:
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
[1] https://review.openstack.org/#/c/612531/2/specs/stein/approved/per-instance-...
[2] https://review.openstack.org/#/c/619953/7/nova/virt/libvirt/driver.py@4894
On Thu, Jan 31, 2019 at 10:05 AM Matt Riedemann <mriedemos@gmail.com> wrote:
I'm going to top post and try to summarize where we are on this thread since I have it on today's nova meeting agenda under the "stuck reviews" section.
* The email started as a proposal to change the proposed image property and flavor extra spec from a confusing boolean to an enum.
* The question was raised why even have any other option than a unique serial number for all instances based on the instance UUID.
* Stephen asked Daniel Berrange (danpb) about the history of the [libvirt]/sysinfo_serial configuration option and it sounds like it was mostly added as a way to determine guests running on the same host, which can already be determined using the hostId parameter in the REST API (hostId is the hashed instance.host + instance.project_id so it's not exactly the same since it's unique per host and project, not just host). However, the hostId is not exposed to the guest in the metadata API / config drive - so that could be a regression for applications that used this somehow to calculate affinity within the guest based on the serial (note that mnaser has a patch to expose hostId in the metadata API / config drive [1]).
* danpb said the system.serial we set today should really be chassis.serial but that's only available in libvirt >= 4.1.0 and our current minimum required version of libvirt is 1.3.1 so setting chassis.serial would have to be conditional on the running version of libvirt (this is common in that driver).
* Applications that depend on the serial number within the guest were not guaranteed it would be unique or not change because migrating the guest to another host would change the serial number anyway (that's the point of the blueprint - to keep the serial unchanged for each guest), so if we just changed to always using unique serial numbers everywhere it should probably be OK (and tolerated/expected by guest applications).
* Clearly we would have a release note if we change this behavior but keep in mind that end users are not reading release notes, and none of this is documented today anyway outside of the [libvirt]/sysinfo_serial config option. So a release note would really only help an operator or support personal if they get a ticket due to the change in behavior (which we probably wouldn't hear about upstream for 2+ years given how slow openstack deployments upgrade).
So where are we? If we want the minimal amount of behavior change as possible then we just add the new image property / flavor extra spec / config option choice, but that arguably adds technical debt and virt-driver specific behavior to the API (again, that's not uncommon though).
If we want to simplify, we don't add the image property / flavor extra spec. But what do we do about the existing config option?
Do we add the 'unique' choice, make it the default, and then deprecate the option to at least signal the change is coming in Train?
Or do we just deprecate the option in Stein and completely ignore it, always setting the unique serial number as the instance.uuid (and set the host serial in chassis.serial if libvirt>=4.1.0)?
In addition, do we expose hostId in the metadata API / config drive via [1] so there is a true alternative *from within the guest* to determine guest affinity on the same host? I'm personally OK with [1] if there is some user documentation around it (as noted in the review).
If we are not going to add the new image property / extra spec, my personal choice would be to:
- Add the 'unique' choice to the [libvirt]/sysinfo_serial config option and make it the default for new deployments. - Deprecate the sysinfo_serial config option in Stein and remove it in Train. This would give at least some window of time for transition and/or raising a stink if someone thinks we should leave the old per-host behavior. - Merge mnaser's patch to expose hostId in the metadata API and config drive so users still have a way within the guest to determine that affinity for servers in the same project on the same host.
I agree with this for a few reasons Assuming that a system serial means that it is colocated with another machine seems just taking advantage of a bug in the first place. That is not *documented* behaviour and serials should inherently be unique, it also exposes information about the host which should not be necessary, Matt has pointed me to an OSSN about this too: https://wiki.openstack.org/wiki/OSSN/OSSN-0028 I think we should indeed provide a unique serials (only, ideally) to avoid having the user shooting themselves in the foot by exposing information they didn't know they were exposing. The patch that I supplied was really meant to make that information available in a controllable way, it also provides a much more secure way of exposing that information because hostId is actually hashed with the tenant ID which means that one VM from one tenant can't know that it's hosted on the same VM as another one by usnig the hostId (and with all of the recent processor issues, this is a big plus in security).
What do others think?
[1] https://review.openstack.org/#/c/577933/
On 1/24/2019 9:09 AM, Matt Riedemann wrote:
The proposal from the spec for this feature was to add an image property (hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
[1] https://review.openstack.org/#/c/612531/2/specs/stein/approved/per-instance-...
[2] https://review.openstack.org/#/c/619953/7/nova/virt/libvirt/driver.py@4894
--
Thanks,
Matt
-- Mohammed Naser — vexxhost ----------------------------------------------------- D. 514-316-8872 D. 800-910-1726 ext. 200 E. mnaser@vexxhost.com W. http://vexxhost.com
- Add the 'unique' choice to the [libvirt]/sysinfo_serial config option and make it the default for new deployments. - Deprecate the sysinfo_serial config option in Stein and remove it in Train. This would give at least some window of time for transition and/or raising a stink if someone thinks we should leave the old per-host behavior. - Merge mnaser's patch to expose hostId in the metadata API and config drive so users still have a way within the guest to determine that affinity for servers in the same project on the same host.
So can I assume this is the decision now? I will update the patch according to this and since I have 3 more days before Chinese new year, I will update again if something new happens On Thu, Jan 31, 2019 at 11:38 PM Mohammed Naser <mnaser@vexxhost.com> wrote:
On Thu, Jan 31, 2019 at 10:05 AM Matt Riedemann <mriedemos@gmail.com> wrote:
I'm going to top post and try to summarize where we are on this thread since I have it on today's nova meeting agenda under the "stuck reviews" section.
* The email started as a proposal to change the proposed image property and flavor extra spec from a confusing boolean to an enum.
* The question was raised why even have any other option than a unique serial number for all instances based on the instance UUID.
* Stephen asked Daniel Berrange (danpb) about the history of the [libvirt]/sysinfo_serial configuration option and it sounds like it was mostly added as a way to determine guests running on the same host, which can already be determined using the hostId parameter in the REST API (hostId is the hashed instance.host + instance.project_id so it's not exactly the same since it's unique per host and project, not just host). However, the hostId is not exposed to the guest in the metadata API / config drive - so that could be a regression for applications that used this somehow to calculate affinity within the guest based on the serial (note that mnaser has a patch to expose hostId in the metadata API / config drive [1]).
* danpb said the system.serial we set today should really be chassis.serial but that's only available in libvirt >= 4.1.0 and our current minimum required version of libvirt is 1.3.1 so setting chassis.serial would have to be conditional on the running version of libvirt (this is common in that driver).
* Applications that depend on the serial number within the guest were not guaranteed it would be unique or not change because migrating the guest to another host would change the serial number anyway (that's the point of the blueprint - to keep the serial unchanged for each guest), so if we just changed to always using unique serial numbers everywhere it should probably be OK (and tolerated/expected by guest applications).
* Clearly we would have a release note if we change this behavior but keep in mind that end users are not reading release notes, and none of this is documented today anyway outside of the [libvirt]/sysinfo_serial config option. So a release note would really only help an operator or support personal if they get a ticket due to the change in behavior (which we probably wouldn't hear about upstream for 2+ years given how slow openstack deployments upgrade).
So where are we? If we want the minimal amount of behavior change as possible then we just add the new image property / flavor extra spec / config option choice, but that arguably adds technical debt and virt-driver specific behavior to the API (again, that's not uncommon though).
If we want to simplify, we don't add the image property / flavor extra spec. But what do we do about the existing config option?
Do we add the 'unique' choice, make it the default, and then deprecate the option to at least signal the change is coming in Train?
Or do we just deprecate the option in Stein and completely ignore it, always setting the unique serial number as the instance.uuid (and set the host serial in chassis.serial if libvirt>=4.1.0)?
In addition, do we expose hostId in the metadata API / config drive via [1] so there is a true alternative *from within the guest* to determine guest affinity on the same host? I'm personally OK with [1] if there is some user documentation around it (as noted in the review).
If we are not going to add the new image property / extra spec, my personal choice would be to:
- Add the 'unique' choice to the [libvirt]/sysinfo_serial config option and make it the default for new deployments. - Deprecate the sysinfo_serial config option in Stein and remove it in Train. This would give at least some window of time for transition and/or raising a stink if someone thinks we should leave the old per-host behavior. - Merge mnaser's patch to expose hostId in the metadata API and config drive so users still have a way within the guest to determine that affinity for servers in the same project on the same host.
I agree with this for a few reasons
Assuming that a system serial means that it is colocated with another machine seems just taking advantage of a bug in the first place. That is not *documented* behaviour and serials should inherently be unique, it also exposes information about the host which should not be necessary, Matt has pointed me to an OSSN about this too:
https://wiki.openstack.org/wiki/OSSN/OSSN-0028
I think we should indeed provide a unique serials (only, ideally) to avoid having the user shooting themselves in the foot by exposing information they didn't know they were exposing.
The patch that I supplied was really meant to make that information available in a controllable way, it also provides a much more secure way of exposing that information because hostId is actually hashed with the tenant ID which means that one VM from one tenant can't know that it's hosted on the same VM as another one by usnig the hostId (and with all of the recent processor issues, this is a big plus in security).
What do others think?
[1] https://review.openstack.org/#/c/577933/
The proposal from the spec for this feature was to add an image
On 1/24/2019 9:09 AM, Matt Riedemann wrote: property
(hw_unique_serial), flavor extra spec (hw:unique_serial) and new "unique" choice to the [libvirt]/sysinfo_serial config option. The image property and extra spec would be booleans but really only True values make sense and False would be more or less ignored. There were no plans to enforce strict checking of a boolean value, e.g. if the image property was True but the flavor extra spec was False, we would not raise an exception for incompatible values, we would just use OR logic and take the image property True value.
The boolean usage proposed is a bit confusing, as can be seen from comments in the spec [1] and the proposed code change [2].
After thinking about this a bit, I'm now thinking maybe we should just use a single-value enum for the image property and flavor extra spec:
image: hw_guest_serial=unique flavor: hw:guest_serial=unique
If either are set, then we use a unique serial number for the guest. If neither are set, then the serial number is based on the host configuration as it is today.
I think that's more clear usage, do others agree? Alex does. I can't think of any cases where users would want hw_unique_serial=False, so this removes that ability and confusion over whether or not to enforce mismatching booleans.
[1]
https://review.openstack.org/#/c/612531/2/specs/stein/approved/per-instance-...
[2]
https://review.openstack.org/#/c/619953/7/nova/virt/libvirt/driver.py@4894
--
Thanks,
Matt
-- Mohammed Naser — vexxhost ----------------------------------------------------- D. 514-316-8872 D. 800-910-1726 ext. 200 E. mnaser@vexxhost.com W. http://vexxhost.com
participants (9)
-
Chris Friesen
-
Jay Pipes
-
Matt Riedemann
-
melanie witt
-
Mohammed Naser
-
Sean Mooney
-
Stephen Finucane
-
Zhang Fan
-
Zhenyu Zheng