-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resource UUID context #286
base: stable/rocky-m3
Are you sure you want to change the base?
Conversation
47df9c4
to
70cccf5
Compare
11e1c74
to
d76f8f0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For all the functions and services we haven't marked explicitly, the context is passed through RPC and thus creates a new RequestContext
object with the passed data, which will call update_store()
in its __init__()
, do I understand that correctly?
How confident are you, that you found all the places that need an explicit set of context.resource_id
?
What about the following instance=
in LOG.*
- https://github.com/sapcc/nova/blob/d76f8f076ef81a90bf877f168b09aea89a683e3b/nova/virt/vmwareapi/driver.py#L1026
- https://github.com/sapcc/nova/blob/d76f8f076ef81a90bf877f168b09aea89a683e3b/nova/virt/vmwareapi/vmops.py#L2944-L2945
- https://github.com/sapcc/nova/blob/d76f8f076ef81a90bf877f168b09aea89a683e3b/nova/virt/vmwareapi/vmops.py#L2953-L2954
- https://github.com/sapcc/nova/blob/d76f8f076ef81a90bf877f168b09aea89a683e3b/nova/virt/vmwareapi/vmops.py#L2956-L2961
I find it funny, that even resource_uuid
in the context is a nova-ism that only ends up in the instance
variable of the log. Proper way seems to be using resource
, but that would need a change in log-format, too, I guess, as it populates the resource
key of the log record.
Exactly, the
Hmm, not 100%. I am fairly sure to have covered all the places where we work with a single instance, and during the creation of instances, as that is fairly well encapsulated. If there is a lack of coverage, I suspect it to be where we iterate over instances. I didn't pay attention to other drivers.
The one with in driver.py is probably creeped in due to rebasing. I'll change that.
Hmm, I didn't notice that, but you are right. But that would also mean passing everywhere to the logger |
d76f8f0
to
dc9deb3
Compare
Looks like this broke some tests. |
I'll fix them, but I am not sure anymore that is the right way. |
fixing tests or your changes adding the |
Fixing the tests is certainly sensible, that is why I do it. But if the whole approach is sensible. I still think it is stupid to pass an instance (or resource) around just to have the logging right. |
fe320b9
to
92c8034
Compare
Currently, the code relies on passing instance to each log call as a keyword parameter. The issue there is, that it is not done necessarily 100% consistently, which causes difficulty to match the log messages with the instance at time. Also it requires at some places the caller to pass the instance purely for logging purposes. Finally, this allows olso.log to remove nova specific code to handle the instance/instance_uuid, but use the generic resource_uuid from the context. Change-Id: I3aade880d3cf5edb0d866b6b72fdeec8a0ae0679
The resource_id is now set automatically, so no need to set it explicitly. Change-Id: Ia2b9ed0167a6420b7441401c5f4614b2ce305962
92c8034
to
bbb5870
Compare
These changes store the instance.uuid in the project independent field
resource_uuid
reserved for that purpose.The second patch removes the now superfluous instance parameter from all logging calls.