)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a68cbbe86aad3cb3041ce004f64ef763fb77eae2","unresolved":true,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When a user requests a vTPM but does not specify the desired secret"},{"line_number":10,"context_line":"security, the config [libvirt]default_tpm_secret_security will be used"},{"line_number":11,"context_line":"to set the secret security in the instance system metadata."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Because the secret security is not included in the flavor nor image in"},{"line_number":14,"context_line":"this case, scheduling will not consider the secret security properly."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"eead36c8_a6878bc8","line":11,"updated":"2025-08-18 14:42:22.000000000","message":"I think this should be rewritten as this isn\u0027t really related to what\u0027s going on here and sort of sent me off on a goose chase when I first read it. I think the point here is that \"for existing instances, the existing vtpm policy is stored only in system_metadata and thus, won\u0027t be seen by scheduling.\" The way this is written currently makes it sound like this code (through `hardware.*` or whatever) is checking the libvirt config, which would be, of course, not cool for conductor. It\u0027s not (even though it\u0027s delving too far into compute-related things IMHO, as I note in the next file) so it would be better to avoid the confusion I think.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"16b1fbf855644b8ea5c4cfdbbf94176cc2ddc419","unresolved":true,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"When a user requests a vTPM but does not specify the desired secret"},{"line_number":10,"context_line":"security, the config [libvirt]default_tpm_secret_security will be used"},{"line_number":11,"context_line":"to set the secret security in the instance system metadata."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Because the secret security is not included in the flavor nor image in"},{"line_number":14,"context_line":"this case, scheduling will not consider the secret security properly."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"adef5970_d8e99cc6","line":11,"in_reply_to":"eead36c8_a6878bc8","updated":"2025-08-18 17:59:43.000000000","message":"Sorry it caused confusion, I will rewrite with your suggestion.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a68cbbe86aad3cb3041ce004f64ef763fb77eae2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"17f14d3c_5094bd9c","updated":"2025-08-18 14:42:22.000000000","message":"Reviewing this makes me realize that our use of `get_vtpm_constraint()` in other parts of the api/control-plane are probably not ideal. I\u0027ve been papering over that method in my head as \"determine if the flavor or image specifies a TPM\" but it\u0027s actually more complicated than that, and more hardware- and virt-specific that I think we should be doing in the virt-independent control plane (scheduler aside perhaps).","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"}],"nova/conductor/manager.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a68cbbe86aad3cb3041ce004f64ef763fb77eae2","unresolved":true,"context_lines":[{"line_number":63,"context_line":"from nova.scheduler import utils as scheduler_utils"},{"line_number":64,"context_line":"from nova import servicegroup"},{"line_number":65,"context_line":"from nova import utils"},{"line_number":66,"context_line":"from nova.virt import hardware"},{"line_number":67,"context_line":"from nova.volume import cinder"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"LOG \u003d logging.getLogger(__name__)"}],"source_content_type":"text/x-python","patch_set":12,"id":"2b8fad5f_5521c6a3","line":66,"updated":"2025-08-18 14:42:22.000000000","message":"In support of my concern about what we\u0027re doing here... this appears to be the first import of `nova.virt.*` in conductor. It feels wrong to me to be doing what we\u0027re doing here, and that detail helps reinforce it I think.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"16b1fbf855644b8ea5c4cfdbbf94176cc2ddc419","unresolved":true,"context_lines":[{"line_number":63,"context_line":"from nova.scheduler import utils as scheduler_utils"},{"line_number":64,"context_line":"from nova import servicegroup"},{"line_number":65,"context_line":"from nova import utils"},{"line_number":66,"context_line":"from nova.virt import hardware"},{"line_number":67,"context_line":"from nova.volume import cinder"},{"line_number":68,"context_line":""},{"line_number":69,"context_line":"LOG \u003d logging.getLogger(__name__)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5066af6f_b98784fa","line":66,"in_reply_to":"2b8fad5f_5521c6a3","updated":"2025-08-18 17:59:43.000000000","message":"This is a fair point but see my other reply, not yet sure what\u0027s the best way to go about this.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a68cbbe86aad3cb3041ce004f64ef763fb77eae2","unresolved":true,"context_lines":[{"line_number":312,"context_line":"                    hardware.get_tpm_secret_security_constraint("},{"line_number":313,"context_line":"                        request_spec.flavor, request_spec.image)):"},{"line_number":314,"context_line":"            tpm_secret_security \u003d instance.system_metadata.get("},{"line_number":315,"context_line":"                \u0027image_hw_tpm_secret_security\u0027)"},{"line_number":316,"context_line":"            if tpm_secret_security:"},{"line_number":317,"context_line":"                request_spec.image.properties.hw_tpm_secret_security \u003d ("},{"line_number":318,"context_line":"                    tpm_secret_security)"}],"source_content_type":"text/x-python","patch_set":12,"id":"0e87ec50_7fe691bf","line":315,"updated":"2025-08-18 14:42:22.000000000","message":"This will be `None` if the user has not done a hard reboot, which means the admin will not be able to migrate instances during an upgrade anyway right?\n\nAlso, I assume that this also means that instances that were shelved before migration, or perhaps even after, won\u0027t be able to unshelve because they can\u0027t hard reboot to approve the policy change right? Also maybe resize? TBF, I\u0027m not sure what the current state of resize with vtpm is ...","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"16b1fbf855644b8ea5c4cfdbbf94176cc2ddc419","unresolved":true,"context_lines":[{"line_number":312,"context_line":"                    hardware.get_tpm_secret_security_constraint("},{"line_number":313,"context_line":"                        request_spec.flavor, request_spec.image)):"},{"line_number":314,"context_line":"            tpm_secret_security \u003d instance.system_metadata.get("},{"line_number":315,"context_line":"                \u0027image_hw_tpm_secret_security\u0027)"},{"line_number":316,"context_line":"            if tpm_secret_security:"},{"line_number":317,"context_line":"                request_spec.image.properties.hw_tpm_secret_security \u003d ("},{"line_number":318,"context_line":"                    tpm_secret_security)"}],"source_content_type":"text/x-python","patch_set":12,"id":"11983720_30b0a623","line":315,"in_reply_to":"0e87ec50_7fe691bf","updated":"2025-08-18 17:59:43.000000000","message":"It will be `None` if the user has not done a hard reboot if the instance is old/existing. It will not be `None` if the instance is new (created on a upgraded compute). New instances get `image_hw_tpm_secret_security` set at create time as they are auto-confirmed with whatever is in flavor or image or default config. (This is related to the thread on the other patch that I pointed gibi to, about the default_tpm_secret_security on new instances).\n\nWhen you say \"migration\", do you mean upgrade? No, there will be no restriction or block of unshelve. It is not required to have `image_hw_tpm_secret_security` to unshelve. If it is not set (old instance not yet confirmed/hard-rebooted) it will unshelve to any compute host that supports TPM, same as legacy behavior. It will behave as a legacy TPM instance until it is hard rebooted. Which just means it will not be allowed to live migrate. If it is set (new instance auto-confirmed), then it will be able to live migrate to any host that supports its required secret security policy.\n\nMaybe I am missing your point or missing something else.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"90adc2abadb70f7bc8fa49c69f4f722d9d6fbdaa","unresolved":true,"context_lines":[{"line_number":312,"context_line":"                    hardware.get_tpm_secret_security_constraint("},{"line_number":313,"context_line":"                        request_spec.flavor, request_spec.image)):"},{"line_number":314,"context_line":"            tpm_secret_security \u003d instance.system_metadata.get("},{"line_number":315,"context_line":"                \u0027image_hw_tpm_secret_security\u0027)"},{"line_number":316,"context_line":"            if tpm_secret_security:"},{"line_number":317,"context_line":"                request_spec.image.properties.hw_tpm_secret_security \u003d ("},{"line_number":318,"context_line":"                    tpm_secret_security)"}],"source_content_type":"text/x-python","patch_set":12,"id":"e11c4bd8_6c9023c8","line":315,"in_reply_to":"11983720_30b0a623","updated":"2025-08-18 18:25:40.000000000","message":"\u003e It will be `None` if the user has not done a hard reboot if the instance is old/existing. It will not be `None` if the instance is new (created on a upgraded compute). New instances get `image_hw_tpm_secret_security` set at create time as they are auto-confirmed with whatever is in flavor or image or default config. (This is related to the thread on the other patch that I pointed gibi to, about the default_tpm_secret_security on new instances).\n\nRight, sorry I realize I wasn\u0027t specific. I know *new* instances won\u0027t be a problem here - I mean \"an existing instance...has not done a hard reboot\".\n\n\u003e When you say \"migration\", do you mean upgrade? No, there will be no restriction or block of unshelve. It is not required to have `image_hw_tpm_secret_security` to unshelve. If it is not set (old instance not yet confirmed/hard-rebooted) it will unshelve to any compute host that supports TPM, same as legacy behavior. It will behave as a legacy TPM instance until it is hard rebooted. Which just means it will not be allowed to live migrate. If it is set (new instance auto-confirmed), then it will be able to live migrate to any host that supports its required secret security policy.\n\u003e \n\u003e Maybe I am missing your point or missing something else.\n\nWhat I meant was.. if the instance gets unshelved without the policy (because it was shelved before the upgrade), we\u0027ll get here and find None for the policy. AFAIK, if we move on from here, the `request_filter` module will only look at the reqspec, which won\u0027t have any policy and thus won\u0027t be able to unshelve...right?","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"11978f2ae99d5df7d25296390ecc930828bcd88d","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                    hardware.get_tpm_secret_security_constraint("},{"line_number":313,"context_line":"                        request_spec.flavor, request_spec.image)):"},{"line_number":314,"context_line":"            tpm_secret_security \u003d instance.system_metadata.get("},{"line_number":315,"context_line":"                \u0027image_hw_tpm_secret_security\u0027)"},{"line_number":316,"context_line":"            if tpm_secret_security:"},{"line_number":317,"context_line":"                request_spec.image.properties.hw_tpm_secret_security \u003d ("},{"line_number":318,"context_line":"                    tpm_secret_security)"}],"source_content_type":"text/x-python","patch_set":12,"id":"bd4b296e_55660506","line":315,"in_reply_to":"d203ec93_1e3c59d4","updated":"2025-08-19 14:06:38.000000000","message":"Okay, right, I think I\u0027m confusing `get_vtpm_constraint()` which always returns something (if vtpm is enabled) and `get_vtpm_security_constraint()`.\n\nSo that instance would have its policy set and auto-confirmed to whatever the default on that compute node is during `_build_and_run_instance()`....got it.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"08af465b807493fabe3eef4352624e4e31b50e3f","unresolved":true,"context_lines":[{"line_number":312,"context_line":"                    hardware.get_tpm_secret_security_constraint("},{"line_number":313,"context_line":"                        request_spec.flavor, request_spec.image)):"},{"line_number":314,"context_line":"            tpm_secret_security \u003d instance.system_metadata.get("},{"line_number":315,"context_line":"                \u0027image_hw_tpm_secret_security\u0027)"},{"line_number":316,"context_line":"            if tpm_secret_security:"},{"line_number":317,"context_line":"                request_spec.image.properties.hw_tpm_secret_security \u003d ("},{"line_number":318,"context_line":"                    tpm_secret_security)"}],"source_content_type":"text/x-python","patch_set":12,"id":"d203ec93_1e3c59d4","line":315,"in_reply_to":"e11c4bd8_6c9023c8","updated":"2025-08-18 20:51:36.000000000","message":"No, it will be able to unshelve because the request filter does sort of the inverse. If we find a non-None policy, we add a required trait for the required policy which restricts the possible destinations. For a None policy, there is no required trait and any host will pass as all hosts support legacy vTPM.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a68cbbe86aad3cb3041ce004f64ef763fb77eae2","unresolved":true,"context_lines":[{"line_number":319,"context_line":"                LOG.info("},{"line_number":320,"context_line":"                    f\"Updating request_spec with \u0027{tpm_secret_security}\u0027 TPM \""},{"line_number":321,"context_line":"                    \u0027secret security\u0027, instance\u003dinstance)"},{"line_number":322,"context_line":"                request_spec.save()"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"    # TODO(tdurakov): remove `live` parameter here on compute task api RPC"},{"line_number":325,"context_line":"    # version bump to 2.x"}],"source_content_type":"text/x-python","patch_set":12,"id":"273113a0_74df1c0d","line":322,"updated":"2025-08-18 14:42:22.000000000","message":"This seems sort of gross to me. This is doing a data migration in the middle of a migration, yet I don\u0027t see anywhere else that this is going to be done for other instances. This feels to me like the kind of thing where in five years, we\u0027ll be wondering why something broke when someone did something to one instance, but worked with another and we realize that the one that worked was migrated at some point.\n\nI know this is a convenient place to \"just make it work\" but it seems a bit obscure and unconventional to me. Are we planning a proper data migration that updates the request spec for all instances before they move? Sounds like we need that. It surely seems to me like shortcutting that at the beginning of a migration is just a one-off fix, but I also know that it\u0027s probably unavoidable...It just really feels messy to me...\n\nIt also seems like this is bringing too much compute/virt logic into the conductor/control plane, no? I mean, we\u0027re not just copying the policy the compute has stamped into sysmeta into the reqspec, we\u0027re doing all the version/model compat check stuff that is in `get_vtpm_constraint()` which could definitely differ in the future, both because that code evolved or because some other hypervisor started supporting this with much different rules.\n\nCould we not have a more generic migration routine that just copies the compute-approved policy into the reqspec without `nova.virt.hardware` involvement, and then also do that same thing in a data migration?","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"16b1fbf855644b8ea5c4cfdbbf94176cc2ddc419","unresolved":true,"context_lines":[{"line_number":319,"context_line":"                LOG.info("},{"line_number":320,"context_line":"                    f\"Updating request_spec with \u0027{tpm_secret_security}\u0027 TPM \""},{"line_number":321,"context_line":"                    \u0027secret security\u0027, instance\u003dinstance)"},{"line_number":322,"context_line":"                request_spec.save()"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"    # TODO(tdurakov): remove `live` parameter here on compute task api RPC"},{"line_number":325,"context_line":"    # version bump to 2.x"}],"source_content_type":"text/x-python","patch_set":12,"id":"7ebb6317_c4d6e3b0","line":322,"in_reply_to":"273113a0_74df1c0d","updated":"2025-08-18 17:59:43.000000000","message":"It is unconventional yes but the constraints here are:\n\n* This has to be done somewhere not in nova-compute to avoid an upcall to access the request spec\n\n* This has to be done after instance creation on nova-compute and it will not be a one-time thing. This will be needed for any newly created instance, forever. If a secret security policy is not specified in flavor or image (which is allowed), the policy will be taken from the compute host `[libvirt]default_tpm_secret_security` configuration and set in the instance system metadata. The challenge is, what is the best way to copy the policy from the instance system metadata to the request spec after an instance has already landed on a compute host and without making an upcall to the API database?\n\nIf you have a better idea, I will be happy to try it.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"90adc2abadb70f7bc8fa49c69f4f722d9d6fbdaa","unresolved":true,"context_lines":[{"line_number":319,"context_line":"                LOG.info("},{"line_number":320,"context_line":"                    f\"Updating request_spec with \u0027{tpm_secret_security}\u0027 TPM \""},{"line_number":321,"context_line":"                    \u0027secret security\u0027, instance\u003dinstance)"},{"line_number":322,"context_line":"                request_spec.save()"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"    # TODO(tdurakov): remove `live` parameter here on compute task api RPC"},{"line_number":325,"context_line":"    # version bump to 2.x"}],"source_content_type":"text/x-python","patch_set":12,"id":"fe2ee1b6_cb6a64af","line":322,"in_reply_to":"7ebb6317_c4d6e3b0","updated":"2025-08-18 18:25:40.000000000","message":"\u003e It is unconventional yes but the constraints here are:\n\u003e \n\u003e * This has to be done somewhere not in nova-compute to avoid an upcall to access the request spec\n\nYep, understand, but a data migration can do it for existing instances.\n\n\u003e * This has to be done after instance creation on nova-compute and it will not be a one-time thing. This will be needed for any newly created instance, forever. If a secret security policy is not specified in flavor or image (which is allowed), the policy will be taken from the compute host `[libvirt]default_tpm_secret_security` configuration and set in the instance system metadata. The challenge is, what is the best way to copy the policy from the instance system metadata to the request spec after an instance has already landed on a compute host and without making an upcall to the API database?\n\nAh, this is maybe an unrealized (by me at least) detail, which seems pretty non-ideal, I guess. The default even for new instances is compute-defined, which makes it difficult for us to do the right thing even for new instances and will always require this sort of patching-up. To me, that also feels a bit like the unfortunate behaviors we have around AZs. If a user has no preference for vTPM security and ends up on a user-security compute by default, as soon as they migrate (or are migrated) they\u0027ll be stuck with that forever, even though they made no such choice.\n\nThat seems not super awesome to me. I know we\u0027re kinda painted into a hole here, but it sucks. I\u0027ll have to think on it a bit.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"11978f2ae99d5df7d25296390ecc930828bcd88d","unresolved":true,"context_lines":[{"line_number":319,"context_line":"                LOG.info("},{"line_number":320,"context_line":"                    f\"Updating request_spec with \u0027{tpm_secret_security}\u0027 TPM \""},{"line_number":321,"context_line":"                    \u0027secret security\u0027, instance\u003dinstance)"},{"line_number":322,"context_line":"                request_spec.save()"},{"line_number":323,"context_line":""},{"line_number":324,"context_line":"    # TODO(tdurakov): remove `live` parameter here on compute task api RPC"},{"line_number":325,"context_line":"    # version bump to 2.x"}],"source_content_type":"text/x-python","patch_set":12,"id":"8f0665ce_1cb7e3b8","line":322,"in_reply_to":"fe2ee1b6_cb6a64af","updated":"2025-08-19 14:06:38.000000000","message":"So, one thing I can come up with is having a cluster-wide default value for the tpm security. That could be known/set by the api very early on, thus not requiring this patch-up every time we migrate, and also not introducing the inconsistent behavior of inheriting the policy of the compute you land on first.\n\nThe other would be to allow the policy on an instance to be unset (or set to \"auto\" or \"any\"). That means no preference and could be migrated to any host with any model, but I guess that means we\u0027d also need to know what the current host is using (and thus whether it\u0027s configured with deployment credentials, if the target is, etc.","commit_id":"5c463faa5c8ff86c4878cebf08a45d26af9b1e4b"}]}