Extract the configurable startup options and update the log tool #17
Conversation
|
@qclc: GitHub didn't allow me to assign the following users: your_reviewer. Note that only openyurtio members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@qclc: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qclc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
qclc
force-pushed
the
add-startup-options
branch
from
257ddbd to
9150cb7
Compare
|
@qclc: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Please take a look. @wawlian |
qclc
force-pushed
the
add-startup-options
branch
2 times, most recently
from
a9e4c5e to
4ee720c
Compare
| @@ -22,35 +22,29 @@ import ( | |||
| "fmt" | |||
There was a problem hiding this comment.
How about moving this file to clients/edgex-foundry?
| @@ -236,7 +227,7 @@ func (efc *EdgexDeviceClient) UpdatePropertyState(ctx context.Context, propertyN | |||
| dps.PutURL = putURL | |||
| } | |||
| // set the device property to desired state | |||
| efc.V(5).Info("setting the property to desired value", "property", dps.Name) | |||
| klog.V(5).Infof("setting the property %s to desired value", dps.Name) | |||
There was a problem hiding this comment.
Adding device name into the log info may be better.
There was a problem hiding this comment.
The device name has been added.
| mgr, err := ctrl.NewManager(cfg, ctrl.Options{ | ||
| Scheme: scheme, | ||
| MetricsBindAddress: opts.MetricsAddr, | ||
| Port: 9443, |
There was a problem hiding this comment.
yurt-device-controller has't involve any webhook, seems no need to set this Port field.
There was a problem hiding this comment.
The port field has been removed.
| } | ||
| } | ||
|
|
||
| func preflightCheck(mgr ctrl.Manager, opts *options.YurtDeviceControllerOptions) error { |
There was a problem hiding this comment.
Perhaps we need to check edgex apis here either.
controllers/device_controller.go
Outdated
| // Checking if device already exist on the edge platform | ||
| edgeDevice, err := r.deviceCli.Get(nil, edgeDeviceName, iotcli.GetOptions{}) | ||
| if err == nil { | ||
| // a. If object exists, the status of the device on OpenYurt is updated | ||
| log.V(4).Info("Device already exists on edge platform") | ||
| klog.V(4).Info("Device already exists on edge platform") |
There was a problem hiding this comment.
Adding device name into the log message may be better.
There was a problem hiding this comment.
The device name has been added.
controllers/device_controller.go
Outdated
| newDeviceStatus.EdgeId = createdEdgeObj.Status.EdgeId | ||
| newDeviceStatus.Synced = true | ||
| } | ||
| } else { | ||
| log.V(4).Info("failed to visit the edge platform") | ||
| klog.V(4).Info("failed to visit the edge platform") |
There was a problem hiding this comment.
Perhaps ErrorS may be better
controllers/device_controller.go
Outdated
| // the device has been added to the edge platform, check if each device property are in the desired state | ||
| newDeviceStatus := d.Status.DeepCopy() | ||
| // This list is used to hold the names of properties that failed to reconcile | ||
| var failedPropertyNames []string | ||
|
|
||
| // 1. reconciling the AdminState and OperatingState field of device | ||
| log.V(3).Info("reconciling the AdminState and OperatingState field of device") | ||
| klog.V(3).Info("reconciling the AdminState and OperatingState field of device") |
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_controller.go
Outdated
| @@ -232,16 +215,16 @@ func (r *DeviceReconciler) reconcileUpdateDevice(ctx context.Context, d *devicev | |||
| } | |||
|
|
|||
| // 2. reconciling the device properties' value | |||
| log.V(3).Info("reconciling the device properties") | |||
| klog.V(3).Info("reconciling the device properties") | |||
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_controller.go
Outdated
| } | ||
|
|
||
| d.Status = *newDeviceStatus | ||
|
|
||
| // 3. update the device status on OpenYurt | ||
| log.V(3).Info("update the device status") | ||
| klog.V(3).Info("update the device status") |
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_controller.go
Outdated
| newDeviceStatus := deviceStatus.DeepCopy() | ||
| // This list is used to hold the names of properties that failed to reconcile | ||
| var failedPropertyNames []string | ||
| // 2. reconciling the device properties' value | ||
| log.V(3).Info("reconciling the device properties' value") | ||
| klog.V(3).Info("reconciling the value of device properties") |
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_controller.go
Outdated
| for _, desiredProperty := range d.Spec.DeviceProperties { | ||
| if desiredProperty.DesiredValue == "" { | ||
| continue | ||
| } | ||
| propertyName := desiredProperty.Name | ||
| // 1.1. gets the actual property value of the current device from edge platform | ||
| log.V(4).Info("getting the actual property state", "property", propertyName) | ||
| klog.V(4).Infof("getting the actual value of property: %s", propertyName) |
There was a problem hiding this comment.
Adding device name into log message may be better.
controllers/device_controller.go
Outdated
| "property getURL", actualProperty.GetURL, | ||
| "property actual value", actualProperty.ActualValue) | ||
| klog.V(4).Infof("got the actual property state, {Name: %s, GetURL: %s, ActualValue: %s}", | ||
| propertyName, actualProperty.GetURL, actualProperty.ActualValue) |
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_controller.go
Outdated
| "desired value", desiredProperty.DesiredValue, | ||
| "actual value", actualProperty.ActualValue) | ||
| klog.V(4).Infof("the desired value and the actual value are different, desired: %s, actual: %s", | ||
| desiredProperty.DesiredValue, actualProperty.ActualValue) |
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_controller.go
Outdated
| if err := r.deviceCli.UpdatePropertyState(nil, propertyName, d, iotcli.UpdateOptions{}); err != nil { | ||
| log.V(4).Error(err, "failed to update property", "propertyName", propertyName) | ||
| klog.V(4).ErrorS(err, "failed to update property", "propertyName", propertyName) |
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_controller.go
Outdated
| failedPropertyNames = append(failedPropertyNames, propertyName) | ||
| continue | ||
| } | ||
|
|
||
| log.V(4).Info("successfully set the property to desired value", "property", propertyName) | ||
| klog.V(4).Infof("successfully set the property %s to desired value", propertyName) |
There was a problem hiding this comment.
Adding device name into log message may be better.
There was a problem hiding this comment.
The deviceName has been added.
controllers/device_syncer.go
Outdated
| return DeviceSyncer{}, err | ||
| } | ||
|
|
||
| func NewDeviceSyncer(client client.Client, opts *options.YurtDeviceControllerOptions, nodepool string) (DeviceSyncer, error) { |
There was a problem hiding this comment.
The function signature can be
func NewDeviceSyncer(client client.Client, opts *options.YurtDeviceControllerOptions) (DeviceSyncer, error)
since nodePool can be retrieved from opts.
There was a problem hiding this comment.
NodePool has been retrieved from opts.
| continue | ||
| } | ||
|
|
||
| ds.log.V(1).Info("One round of Device synchronization is complete") |
There was a problem hiding this comment.
This log may be necessary to indicate whether on round of synchronization completes or not.
controllers/deviceprofile_syncer.go
Outdated
| if err != nil { | ||
| return DeviceProfileSyncer{}, err | ||
| } | ||
| func NewDeviceProfileSyncer(client client.Client, opts *options.YurtDeviceControllerOptions, nodepool string) (DeviceProfileSyncer, error) { |
There was a problem hiding this comment.
nodepool can be retrieved from opts
There was a problem hiding this comment.
NodePool has been retrieved from opts.
| if err := deleteDeviceProfile(ds.log, ds.Client, deleteKDevs); err != nil { | ||
| ds.log.Error(err, "fail to delete device profiles") | ||
| if err := deleteDeviceProfile(ds.Client, deleteKDevs); err != nil { | ||
| klog.V(3).ErrorS(err, "fail to delete device profiles") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Adding a log line to indicate completion of one synchronization round may be better.
| @@ -175,34 +169,34 @@ func getKubeNameWithPrefix(edgeName, NodePoolName string) string { | |||
| } | |||
|
|
|||
| // createDeviceProfile creates the list of device profiles | |||
| func createDeviceProfile(log logr.Logger, cli client.Client, edgeXDevs []devicev1alpha1.DeviceProfile, NodePoolName string) error { | |||
| func createDeviceProfile(cli client.Client, edgeXDevs []devicev1alpha1.DeviceProfile, NodePoolName string) error { | |||
There was a problem hiding this comment.
NodePoolName can be retrieved from opts
There was a problem hiding this comment.
NodePoolName has been retrieved from opts
| func (r *DeviceServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| coreMetaCliInfo := edgexCli.ClientURL{Host: "edgex-core-metadata", Port: 48081} | ||
| r.deviceServiceCli = edgexCli.NewEdgexDeviceServiceClient(coreMetaCliInfo, r.Log) | ||
| func (r *DeviceServiceReconciler) SetupWithManager(mgr ctrl.Manager, opts *options.YurtDeviceControllerOptions, nodepool string) error { |
There was a problem hiding this comment.
nodepool can be retrieved from opts.
There was a problem hiding this comment.
NodePool has been retrieved from opts.
| return nil | ||
| } | ||
| } else { | ||
| // a. If object exists, the status of the device on OpenYurt is updated | ||
| log.V(4).Info("DeviceService already exists on edge platform") | ||
| klog.V(4).Info("DeviceService already exists on edge platform") |
There was a problem hiding this comment.
Adding deviceservice name into the log message may be better.
There was a problem hiding this comment.
The deviceservice name has been added.
| @@ -195,19 +181,18 @@ func (r *DeviceServiceReconciler) reconcileCreateDeviceService(ctx context.Conte | |||
| addressable := ds.Spec.Addressable | |||
| as, err := r.deviceServiceCli.GetAddressable(nil, addressable.Name, edgeInterface.GetOptions{}) | |||
| if err == nil { | |||
| log.V(4).Info("Addressable already exists on edge platform") | |||
| klog.V(4).Info("Addressable already exists on edge platform") | |||
There was a problem hiding this comment.
Adding deviceservice name into the log message may be better.
There was a problem hiding this comment.
The deviceservice name has been added.
controllers/deviceservice_syncer.go
Outdated
| if err != nil { | ||
| return DeviceServiceSyncer{}, err | ||
| } | ||
| func NewDeviceServiceSyncer(client client.Client, opts *options.YurtDeviceControllerOptions, nodepool string) (DeviceServiceSyncer, error) { |
There was a problem hiding this comment.
nodepool can be retrieved from opts
There was a problem hiding this comment.
nodepool has been retrieved from opts
| continue | ||
| } | ||
|
|
||
| ds.log.V(1).Info("One round of DeviceService synchronization is complete") |
There was a problem hiding this comment.
This log line is necessary.
qclc
force-pushed
the
add-startup-options
branch
from
4ee720c to
e97cc02
Compare
|
/lgtm |
|
@wawlian: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What type of PR is this?
/kind enhancement
What this PR does / why we need it:
klog.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
/assign @wawlian
Does this PR introduce a user-facing change?
--zap-log-levelto-v (or --v)--help (or -h)parameter:$ ./yurt-device-controller --help Launch yurt-device-controller Usage: yurt-device-controller [flags] Flags: --add_dir_header If true, adds the file directory to the header of the log messages --alsologtostderr log to standard error as well as files --core-command-address string The address of edge core-command service. (default "edgex-core-command:48082") --core-data-address string The address of edge core-data service. (default "edgex-core-data:48080") --core-metadata-address string The address of edge core-metadata service. (default "edgex-core-metadata:48081") --edge-sync-period uint The period of the device management platform synchronizing the device status to the cloud.(in seconds,not less than 5 seconds) (default 5) --health-probe-bind-address string The address the probe endpoint binds to. (default ":8081") -h, --help help for yurt-device-controller --kubeconfig string Paths to a kubeconfig. Only required if out-of-cluster. --leader-elect Enable leader election for controller manager. Enabling this will ensure there is only one active controller manager. --log_backtrace_at traceLocation when logging hits line file:N, emit a stack trace (default :0) --log_dir string If non-empty, write log files in this directory --log_file string If non-empty, use this log file --log_file_max_size uint Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800) --logtostderr log to standard error instead of files (default true) --metrics-bind-address string The address the metric endpoint binds to. (default ":8080") --namespace string The cluster namespace for edge resources synchronization. (default "default") --nodepool string The nodePool deviceController is deployed in.(just for debugging) --one_output If true, only write logs to their native severity level (vs also writing to each lower severity level) --skip_headers If true, avoid header prefixes in the log messages --skip_log_headers If true, avoid headers when opening log files --stderrthreshold severity logs at or above this threshold go to stderr (default 2) -v, --v Level number for the log level verbosity --vmodule moduleSpec comma-separated list of pattern=N settings for file-filtered loggingother Note