Revise TidbMonitor controller #1500
Conversation
Yisaer
added
area/monitor
|
please add a release note |
|
/run-all-test |
|
/run-all-tests |
| return err | ||
| } | ||
| mm.recorder.Event(monitor, corev1.EventTypeNormal, SuccessSync, fmt.Sprintf("Sync TidbMonitor[%s/%s] Service Success", monitor.Namespace, monitor.Name)) |
There was a problem hiding this comment.
I suggest not recording successful sync to reduce verbosity, also, note that there is a global event rate-limit of tidb-controller-manager
There was a problem hiding this comment.
updated, all successful sync wouldn't be recorded.
There was a problem hiding this comment.
the good practice is to record a successful event once the operation is done successfully and skip if nothing needs to do because the actual state matches the desired state
in this case, the events of objects will be like:
- optional warning events about failures
- one successful event
There was a problem hiding this comment.
this pattern can be applied to the most scenarios that the desired state is often only changed by the reconciliation controller, e.g. object specs managed by our controller
we can improve it in the future
| @@ -182,3 +209,13 @@ func (mm *MonitorManager) syncTidbMonitorRbac(monitor *v1alpha1.TidbMonitor) (*c | |||
|
|
|||
| return sa, nil | |||
| } | |||
|
|
|||
| // If the namespace in ClusterRef is empty, we would set the TidbMonitor's namespace in the default | |||
| func setDefaultNamespaceForClusterRef(tm *v1alpha1.TidbMonitor) { | |||
There was a problem hiding this comment.
Please do not mutate spec in controller, spec is user intent and by definition, is owned by user
Co-Authored-By: Aylei <[email protected]>
pkg/monitor/monitor/util.go
Outdated
| @@ -729,3 +734,10 @@ func getMonitorPVC(monitor *v1alpha1.TidbMonitor) *core.PersistentVolumeClaim { | |||
| }, | |||
| } | |||
| } | |||
|
|
|||
| // If the namespace in ClusterRef is empty, we would set the TidbMonitor's namespace in the default | |||
| func setDefaultNamespaceForClusterRef(model *MonitorConfigModel, tm *v1alpha1.TidbMonitor) { | |||
There was a problem hiding this comment.
I think we don't need to create a new function for this
Sure, and issued in #1554. |
|
BTW, it is recommended to separate PR to logic complete unit, so that each feature/fix could be independently rollbacked or cherry-picked, also, the release note will be easier to write. |
|
/merge |
|
/run-all-tests |
|
/run-all-tests |
|
@Yisaer merge failed. |
|
/run-e2e-in-kind |
|
cherry pick to release-1.1 in PR #1557 |
* fix log bug * add default namespace for clusterRef * fix error log which don't requeue * add event recorder * Update pkg/monitor/monitor/monitor_manager.go Co-Authored-By: Aylei <[email protected]> * fix by conment * Update monitor_manager.go * Update util.go Co-authored-by: Song Gao <[email protected]> Co-authored-by: Aylei <[email protected]>
What problem does this PR solve?
fix the bug for the log level of the Prometheus Spec in TidbMonitor which would cause TidbMonitor Failed to start. TidbMonitor: should not specify log level when prometheus.logLevel is empty #1482
Add default namespace for the
ClusterRefif it's empty in TidbMonitor Spec. no default for.spec.clusterRef.namespaceof TiDB monitor #1480Add events for TidbMonitor Controller for a more user-friendly experience. clearer message when TidbMonitor failed syncing because of the absence of referenced cluster #1481
Does this PR introduce a user-facing change?: