[generic_config_updater] Logging #1864

ghooo · 2021-10-07T18:31:29Z

What I did

Add some logs to generic_updater

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

Empty patch

admin@vlab-01:~$ sudo config apply-patch empty.json-patch 
Patch Applier: Patch application starting.
Patch Applier: Patch: []
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config according to YANG models.
... [logs from sonic-yang-mgmt framework]
Patch Applier: Sorting patch updates.
Patch Applier: The patch was sorted into 0 changes.
Patch Applier: Applying changes in order.
Patch Applier: Verifying patch updates are reflected on ConfigDB.
Patch Applier: Patch application completed.
Patch applied successfully.
admin@vlab-01:~$

Single change patch

admin@vlab-01:~$ sudo config apply-patch dhcp_add.json-patch 
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "add", "path": "/VLAN/Vlan1000/dhcp_servers/4", "value": "192.0.0.5"}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config according to YANG models.
... [logs from sonic-yang-mgmt framework]
Patch Applier: The patch was sorted into 1 change:
Patch Applier:   * [{"op": "add", "path": "/VLAN/Vlan1000/dhcp_servers/4", "value": "192.0.0.5"}]
Patch Applier: Applying changes in order.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: ChangeApplier.apply(change) is not implemented yet
admin@vlab-01:~$

Multi change patch:

admin@vlab-01:~$ sudo config apply-patch update_lanes.json-patch 
Patch Applier: Patch application starting.
Patch Applier: Patch: [{"op": "replace", "path": "/PORT/Ethernet100/lanes", "value": "121,122,123"}]
Patch Applier: Validating patch is not making changes to tables without YANG models.
Patch Applier: Getting current config db.
Patch Applier: Simulating the target full config after applying the patch.
Patch Applier: Validating target config according to YANG models.
... [logs from sonic-yang-mgmt framework]
Patch Applier: Sorting patch updates.
... [logs from sonic-yang-mgmt framework]
Patch Applier: The patch was sorted into 2 changes:
Patch Applier:   * [{"op": "remove", "path": "/PORT/Ethernet100"}]
Patch Applier:   * [{"op": "add", "path": "/PORT/Ethernet100", "value": {"alias": "fortyGigE0/100", "description": "fortyGigE0/100", "index": "25", "lanes": "121,122,123", "mtu": "9100", "pfc_asym": "off", "speed": "40000", "tpid": "0x8100"}}]
Patch Applier: Applying changes in order.
Failed to apply patch
Usage: config apply-patch [OPTIONS] PATCH_FILE_PATH
Try "config apply-patch -h" for help.

Error: ChangeApplier.apply(change) is not implemented yet
admin@vlab-01:~$

lgtm-com · 2021-10-07T18:44:02Z

This pull request introduces 5 alerts when merging b56fbde0b7b5bac8e7056ceb1ee80260c823bd3d into d13955a - view on LGTM.com

new alerts:

4 for Wrong number of arguments in a call
1 for First parameter of a method is not named 'self'

qiluo-msft · 2021-10-12T06:29:59Z

generic_config_updater/gu_common.py

 import subprocess
 import yang as ly
 import copy
 import re
 from enum import Enum

 YANG_DIR = "/usr/local/yang-models"
+SYSLOG_IDENTIFIER = "GenericConfigUpdater"


GenericConfigUpdater

Is this ident too long? The default will be process name such as config, is it good enough since you have add title to diffentiate. #Closed

Here is how it look like:

Oct 11 18:15:50.604939 vlab-01 INFO GenericConfigUpdater: Patch Applier: Getting current config db. Oct 11 18:15:51.494117 vlab-01 INFO GenericConfigUpdater: Patch Applier: Simulating the target full config after applying the patch. Oct 11 18:15:51.502449 vlab-01 INFO GenericConfigUpdater: Patch Applier: Validating target config according to YANG models.

I think it is OK, this way I can grep all syslogs associated with GenericConfigUpdater i.e. cat /var/log/syslog | grep GenericConfigUpdater

I think we will add more logs to other parts such config replace, rollback checkpoint and the applier.

Also I don't think we should use config since this library might be used with REST APIs like was requested in some of the earlier meetings with SONiC Community

qiluo-msft · 2021-10-12T06:40:32Z

generic_config_updater/generic_updater.py

        self.config_wrapper = config_wrapper if config_wrapper is not None else ConfigWrapper()
        self.patch_wrapper = patch_wrapper if patch_wrapper is not None else PatchWrapper()
        self.patchsorter = patchsorter if patchsorter is not None else PatchSorter(self.config_wrapper, self.patch_wrapper)
        self.changeapplier = changeapplier if changeapplier is not None else ChangeApplier()

    def apply(self, patch):
+        logger.log_info(self.LOGTITLE, "Patch application starting.")


self.LOGTITLE

You add the same title everywhere. Is it better to define an instance in this class, and init with the title. #Closed

This way makes it easier to add new log msgs, I don't have to create a new instance of logger for each class.

nvm, created a logger instance for the PatchApplier class

qiluo-msft · 2021-10-12T06:43:03Z

generic_config_updater/gu_common.py

@@ -691,3 +693,44 @@ def _get_model(self, model, name):
                    return submodel

        return None
+
+class GenericUpdaterLogger:


GenericUpdaterLogger

This class seems have general value, like https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-py-common/sonic_py_common/logger.py.

Is it better to enhance existing Logger class? If something is impossible, we can inherit the class. #Closed

used sonic-py-common logger

qiluo-msft · 2021-10-12T06:43:33Z

generic_config_updater/gu_common.py

+        Console settings: log levels [error, warning, info] always printed, [debug] only printed if verbose logging
+        Syslog settings: log all levels [error, warning, info, debug]
+    """
+    def init_verbose_logging(self, verbose):


init_verbose_logging

Better use __init__() #Closed

if I use __init__ it means when creating the logger I initialize verbose, but this is not the case as we modify verbosity later.

used loggingSettings instead and the logger is created from loggingSettings.getlogger

qiluo-msft · 2021-10-12T06:44:30Z

generic_config_updater/gu_common.py

+    def _log_to_console(self, msg, logLevel):
+        # Always log [warning, error, info] i.e. not debug, but if verbose logging print debug as well
+        if logLevel < syslog.LOG_DEBUG or self.enable_verbose_logging:
+            print(msg)


print

It is an error, should we print to stderr? How about warning? #Closed

used common logger logic

qiluo-msft · 2021-10-12T06:46:49Z

generic_config_updater/gu_common.py

+        syslog.syslog(logLevel, msg)
+        syslog.closelog()
+
+logger = GenericUpdaterLogger()


logger

If global object is used by multiple places, and init verbose multiple time, user will be confused about the interference #Closed

The idea is to have a single global logger in gu_common for which verbosity can be modified.

Another idea is to create an init_env(...) method in gu_common and this function takes care of creating the logger.

global loggingSettings and then each logger instance is created per class, there is no global logger

init_env(...) will solve the user confusing issue I mentioned.

@qiluo-msft I have used loggingSettings instead, there the verbose flag can change, but then once a logger is created its verbosity can not change. Do you think this is confusing?

qiluo-msft · 2021-10-14T01:00:54Z

generic_config_updater/gu_common.py

+    def getLogger(self, title):
+        return GenericUpdaterLogger(title, self._verbose)
+
+loggingSettings = LoggingSettings()


loggingSettings

rename: logger #Closed

qiluo-msft · 2021-10-14T01:01:38Z

generic_config_updater/gu_common.py

@@ -691,3 +693,26 @@ def _get_model(self, model, name):
                    return submodel

        return None
+
+class GenericUpdaterLogger(logger.Logger):
+    def __init__(self, title, verbose):


title

Also add ident as another parameter #Closed

qiluo-msft · 2021-10-14T01:02:06Z

generic_config_updater/gu_common.py

@@ -691,3 +693,26 @@ def _get_model(self, model, name):
                    return submodel

        return None
+
+class GenericUpdaterLogger(logger.Logger):


GenericUpdaterLogger

rename: TitledLogger.
It could be design more general. #Closed

qiluo-msft · 2021-10-14T01:02:19Z

generic_config_updater/gu_common.py

+        combined_msg = f"{self._title}: {msg}"
+        super().log(priority, combined_msg, also_print_to_console)
+
+class LoggingSettings:


LoggingSettings

rename: GenericUpdaterLogger #Closed

ghooo force-pushed the dev/mghoneim/gu/syslogging branch from b56fbde to 82b53eb Compare October 11, 2021 17:58

ghooo changed the title ~~[generic_config_updater] Syslogging~~ [generic_config_updater] Logging Oct 11, 2021

[generic_config_updater] Logging

a35081b

ghooo force-pushed the dev/mghoneim/gu/syslogging branch from 82b53eb to a35081b Compare October 11, 2021 18:03

ghooo mentioned this pull request Oct 12, 2021

Generic_upater: Apply JSON change #1856

Merged

qiluo-msft reviewed Oct 12, 2021

View reviewed changes

ghooo force-pushed the dev/mghoneim/gu/syslogging branch from 10fd12d to 0789414 Compare October 13, 2021 00:39

using common logger

ad29105

ghooo force-pushed the dev/mghoneim/gu/syslogging branch from 0789414 to ad29105 Compare October 13, 2021 00:40

qiluo-msft reviewed Oct 14, 2021

View reviewed changes

creating titledlogger, and gulogging

4539f6b

ghooo force-pushed the dev/mghoneim/gu/syslogging branch from ab35ee6 to 4539f6b Compare October 14, 2021 01:14

qiluo-msft approved these changes Oct 14, 2021

View reviewed changes

ghooo merged commit 4d732c6 into sonic-net:master Oct 14, 2021

ghooo mentioned this pull request Jun 24, 2022

SONiC Generic Update and Rollback - HLD sonic-net/SONiC#736

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[generic_config_updater] Logging #1864

[generic_config_updater] Logging #1864

ghooo commented Oct 7, 2021 •

edited

Loading

lgtm-com bot commented Oct 7, 2021

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 12, 2021

ghooo Oct 12, 2021

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 12, 2021 •

edited

Loading

ghooo Oct 13, 2021

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 13, 2021 •

edited

Loading

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 12, 2021

ghooo Oct 13, 2021

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 13, 2021

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 12, 2021

ghooo Oct 13, 2021

qiluo-msft Oct 13, 2021

ghooo Oct 13, 2021

qiluo-msft Oct 14, 2021 •

edited

Loading

qiluo-msft Oct 14, 2021 •

edited

Loading

qiluo-msft Oct 14, 2021 •

edited

Loading

qiluo-msft Oct 14, 2021 •

edited

Loading

[generic_config_updater] Logging #1864

[generic_config_updater] Logging #1864

Conversation

ghooo commented Oct 7, 2021 • edited Loading

What I did

How I did it

How to verify it

Previous command output (if the output of a command-line utility has changed)

New command output (if the output of a command-line utility has changed)

lgtm-com bot commented Oct 7, 2021

qiluo-msft Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

ghooo Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

ghooo Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiluo-msft Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

qiluo-msft Oct 14, 2021 • edited Loading

Choose a reason for hiding this comment

ghooo commented Oct 7, 2021 •

edited

Loading

qiluo-msft Oct 12, 2021 •

edited

Loading

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 12, 2021 •

edited

Loading

qiluo-msft Oct 12, 2021 •

edited

Loading

ghooo Oct 13, 2021 •

edited

Loading

qiluo-msft Oct 12, 2021 •

edited

Loading

qiluo-msft Oct 12, 2021 •

edited

Loading

qiluo-msft Oct 12, 2021 •

edited

Loading

qiluo-msft Oct 14, 2021 •

edited

Loading

qiluo-msft Oct 14, 2021 •

edited

Loading

qiluo-msft Oct 14, 2021 •

edited

Loading

qiluo-msft Oct 14, 2021 •

edited

Loading