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

1.4 forgiving of 1.5 or other future versions. #185

Merged
merged 3 commits into from
Jun 15, 2023
Merged

Conversation

ibaldin
Copy link
Contributor

@ibaldin ibaldin commented Jun 14, 2023

Made all children of JSONField forgiving based on a set_fields(forgiving) value which by default is set to False. So normally these classes will throw exceptions if an attempt to set a non-existent field is made. However when deserializing from JSON they are more forgiving and just print warnings.

Note that because FIM normally stored unaltered GraphML, if you have a topology produced with a later version of FIM that has new fields not known to this version, it will load the graph and will print warnings every time you try to access the fields. If you want to get rid of all FIM warnings do something like this:

import logging
import fim.logging.fim_logger as fl

logger = logging.getLogger('NULLFIMLOGGER')
logger.addHandler(logging.NullHandler())

fl.set_logger(logger)

This is posted on PyPi as fabric_fim==1.4.14.

@kthare10 please test - I won't merge until you are good with these changes.

Ilya Baldin added 2 commits June 14, 2023 14:47
… be forgiving when reading from JSON - in case it is produced by a later version of the library that has new fields
@ibaldin ibaldin requested a review from kthare10 June 14, 2023 21:23
@kthare10
Copy link
Collaborator

Tested by installing fabric-fim==1.4.14 on Release 1.4.6 container. Slice provisioning works and no errors observed about numa. I did see the following warning logs in the fablib logs even after including the logger import as suggested above.

[23:25:26] {/opt/conda/lib/python3.9/site-packages/fim/slivers/capacities_labels.py:454} WARNING - Using logger <Logger FIM (INFO)> Unable to set field numa of labels, no such field available ['bdf', 'mac', 'ipv4', 'ipv4_range', 'ipv4_subnet', 'ipv6', 'ipv6_range', 'ipv6_subnet', 'asn', 'vlan', 'vlan_range', 'inner_vlan', 'instance', 'instance_parent', 'local_name', 'local_type', 'device_name', 'bgp_key', 'account_id', 'region', 'usb_id']

@ibaldin
Copy link
Contributor Author

ibaldin commented Jun 15, 2023

If your code (or someone's code) does logging.basicConfig() this creates a root logger which then catches these log messages regardless. In that case I think you may need to do

root_logger = logging.getLogger() # get root logger
root_logger.removeHandler(root_logger.handlers[0]) # basicConfig creates a single StreamHandler

@ibaldin
Copy link
Contributor Author

ibaldin commented Jun 15, 2023

This may mess up other logging, of course

@kthare10
Copy link
Collaborator

Since, this only goes in fablib.log file, I am inclined to not change the logger as users are recommended to look at it only in case of failures. Please share your thoughts.

@ibaldin
Copy link
Contributor Author

ibaldin commented Jun 15, 2023

Let me look at it some more - I don't quite understand the root logger behavior - I can see that calling logging.basicConfig() does what I described above, but I do not understand why the log messages go to the root logger.

@ibaldin
Copy link
Contributor Author

ibaldin commented Jun 15, 2023

Seems like this is the logging behavior - if a root logger has a handler - it intercepts all messages. logging.basicConfig() installs a root logger with a stream handler and then it intercepts messages. Sounds like what you are saying is if we mess with the root logger we will turn off logging elsewhere, which is undesirable - so we don't have any choice, I think, but to live with these log messages.

@ibaldin
Copy link
Contributor Author

ibaldin commented Jun 15, 2023

We may need a ticket for fablib or other libraries to make sure they do not do logging.basicConfig() and use root loggers in the future. Instead create module-specific loggers and configure them individually.

@kthare10
Copy link
Collaborator

For now, I am pushing a pypi version of fabrictestbed-extensions with updated fim dependencies.

Copy link
Collaborator

@kthare10 kthare10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@ibaldin ibaldin merged commit 6e302e0 into master Jun 15, 2023
@ibaldin ibaldin deleted the 1.4-forgiving-of-1.5 branch June 15, 2023 15:12
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.

2 participants