-
Notifications
You must be signed in to change notification settings - Fork 32
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
fix watch resources in each controller #1263
Conversation
5a7d871
to
d3c913a
Compare
/test test-integration |
if backupController != nil { | ||
return backupController, nil | ||
} | ||
log.Infof("start backup controller") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it after IsACMResourceReady
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if grafanaController != nil { | ||
return grafanaController, nil | ||
} | ||
log.Info("start grafana controller") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if !config.WithInventory(initOption.MulticlusterGlobalHub) { | ||
return nil, nil | ||
} | ||
log.Info("start inventory controller") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it after GetStorageConnection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -156,7 +158,17 @@ func (r *GrafanaReconciler) SetupWithManager(mgr ctrl.Manager) error { | |||
Watches(&corev1.ConfigMap{}, | |||
&handler.EnqueueRequestForObject{}, builder.WithPredicates(configmapPred)). | |||
Watches(&appsv1.Deployment{}, | |||
&handler.EnqueueRequestForObject{}, builder.WithPredicates(deplomentPred)) | |||
&handler.EnqueueRequestForObject{}, builder.WithPredicates(deploymentPred)). | |||
Watches(&corev1.Service{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to add more permissions for this controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if managedHubController != nil { | ||
return managedHubController, nil | ||
} | ||
log.Info("start managedhub controller") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it after IsACMResourceReady
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
&handler.EnqueueRequestForObject{}, builder.WithPredicates(config.GeneralPredicate)). | ||
Watches(&rbacv1.Role{}, | ||
&handler.EnqueueRequestForObject{}, builder.WithPredicates(config.GeneralPredicate)). | ||
Watches(&rbacv1.RoleBinding{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add more permissions for this controller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
if r.enableMetrics { | ||
mgrBuilder = mgrBuilder. | ||
Watches(&promv1.PrometheusRule{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it work when switch enableMetrics from false to true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to watch the resources when crd exist
@@ -148,7 +148,7 @@ var kafkaPred = predicate.Funcs{ | |||
} | |||
|
|||
func StartKafkaController(ctx context.Context, mgr ctrl.Manager, transporter transport.Transporter) error { | |||
log.Info("start kafka controller") | |||
log.Debug("start kafka controller") | |||
if startedKafkaController { | |||
return nil | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move it after startedKafkaController
. Use Info
for consistence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
if webhookReconciler != nil { | ||
return webhookReconciler, nil | ||
} | ||
log.Info("start webhook controller") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it after IsACMResourceReady
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Signed-off-by: ldpliu <[email protected]> Signed-off-by: root <[email protected]>
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clyang82, ldpliu 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 |
@ldpliu: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
Quality Gate passedIssues Measures |
Summary
We did not watch globalhub applied resources previously, if user changed these resources, we should rollback them.
Related issue(s)
Fixes #
Tests
make unit-tests
.make integration-test
.make e2e-test-all
.