Skip to content
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

Open
wants to merge 2 commits into
base: stable/rocky-m3
Choose a base branch
from
Open

Conversation

fwiesel
Copy link
Member

@fwiesel fwiesel commented Dec 27, 2021

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.

@fwiesel fwiesel force-pushed the resource_uuid_context branch 2 times, most recently from 11e1c74 to d76f8f0 Compare January 14, 2022 08:51
Copy link

@joker-at-work joker-at-work left a 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.*

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.

nova/conductor/manager.py Show resolved Hide resolved
nova/conductor/manager.py Show resolved Hide resolved
@fwiesel
Copy link
Member Author

fwiesel commented Jan 18, 2022

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?

Exactly, the resource_id is passed through exactly like the request_id (or global_request_id).

How confident are you, that you found all the places that need an explicit set of context.resource_id?

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.

What about the following instance= in LOG.*

The one with in driver.py is probably creeped in due to rebasing. I'll change that.
The ones in vmops where using vi.instance, and I wasn't 100% if it was the same. On second look, it is the same, so it can be removed.

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.

Hmm, I didn't notice that, but you are right. But that would also mean passing everywhere to the logger resource=, just to have the uuid in the logs. But that probably reduces my chances of getting the change upstream.

@joker-at-work
Copy link

Looks like this broke some tests.

@fwiesel
Copy link
Member Author

fwiesel commented Jan 18, 2022

I'll fix them, but I am not sure anymore that is the right way.

@joker-at-work
Copy link

I'll fix them, but I am not sure anymore that is the right way.

fixing tests or your changes adding the resource_uuid?

@fwiesel
Copy link
Member Author

fwiesel commented Jan 18, 2022

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.

@fwiesel fwiesel force-pushed the resource_uuid_context branch 2 times, most recently from fe320b9 to 92c8034 Compare January 20, 2022 09:55
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants