-
Notifications
You must be signed in to change notification settings - Fork 54
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: enabling deleted grafana datasources to be recreated #96
Conversation
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.
Looks good, just left a few small nits.
eb2bd7a
to
cd13e13
Compare
@sthaha do you think we want an e2e test here? |
cd13e13
to
8a02154
Compare
I agree, we should test if operator restores datasource if it is modified or deleted. I suspect that given then current code that it might crash if the datasource is deleted. |
Yes this has been tested. |
Can we do this in a separate PR? |
Sure thing :-) |
@prashbnair , Could you please also ensure that deletion a namespace that contains monitoring-stacks also cleans up their associated grafana-datasources ? |
As discussed, it would be nice to have all reconciliation issues sorted in a single or multiple PRs |
8a02154
to
3b587e0
Compare
@sthaha I will address the clean up of grafana data sources when the monitoring stack or namespace is deleted as a separate PR. |
3b587e0
to
4087a83
Compare
|
||
if err != nil { | ||
log.V(3).Info("grafana data source CRD is not defined") | ||
return err |
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.
I think we should follow same the implementation in @fpetkovski 's PR ... https://github.com/rhobs/monitoring-stack-operator/pull/103/files#diff-606d75b2cccce3c13550d55e0c1b27bb1c1fad0431b4f16d466e41dd4e0c801dR355
I.E. function should return requeue (bool)
and error
func setWatch() (requeue bool, err error){
if err = listDataSources(); err != nil
return true, nil
}
err = setWatch()
return false, err
}
So that the caller (Reconcile func) can ...
if requeue, err := r.setGrafanaDatasourceWatch(); requeue || err != nil {
if err {
return ctrl.Result{}, err
}
return ctrl.Result{RequeueAfter: 10 * time.Second}, 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.
All the patchers in this reconcile
function return an error
, I have followed the same model.
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.
This part is different from the rest of the patchers. This isn't a patcher to start with :-)
More importantly ...
We need the ability to requeue
without error if GrafanaDataSource CRD is not created and to log real errors when setting up a watch fails.
|
||
f.K8sClient.Delete(context.Background(), &grafanaDS) | ||
|
||
f.GetResourceWithRetry(t, datasourceName, "monitoring-stack-operator", &grafanaDS) |
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.
I think we should make use of AssertResourceEventuallyExists
here as well
4087a83
to
f6dde2d
Compare
2e1e30c
to
e094bcb
Compare
assert.NilError(t, err, "failed to create a monitoring stack") | ||
|
||
grafanaDS := grafanav1alpha1.GrafanaDataSource{} | ||
|
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.
nit: do we need a blank line?
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.
Good stuff @prashbnair
You may want to consider incorporating the nits
e094bcb
to
3027ffd
Compare
if err != nil { | ||
return ctrl.Result{}, err | ||
} | ||
|
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.
this isn't needed now
@@ -82,7 +82,7 @@ func (f *Framework) AssertResourceEventuallyExists(name, namespace string, resou | |||
} | |||
return false, nil | |||
}); err == wait.ErrWaitTimeout { | |||
t.Fatal(fmt.Errorf("statefulset %s/%s was never created", namespace, name)) | |||
t.Fatal(fmt.Errorf("resource %s/%s was never created", namespace, name)) |
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.
good catch!
3027ffd
to
857a8a3
Compare
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.
🎉
Prior to this fix, grafana datasources which were deleted were not being recreated.
857a8a3
to
b6506ff
Compare
No description provided.