-
Notifications
You must be signed in to change notification settings - Fork 344
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
Automatically set num shards and replicas from referenced OCP ES #1737
Changes from all commits
26f6b06
eef0162
6a2a796
1bd6cf7
dca8691
52633c0
994ab95
b6baf9d
33f325d
b9d5fbe
039a508
1d33758
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,15 @@ | ||
package v1 | ||
|
||
import ( | ||
"context" | ||
"fmt" | ||
|
||
esv1 "github.com/openshift/elasticsearch-operator/apis/logging/v1" | ||
"k8s.io/apimachinery/pkg/api/errors" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/apimachinery/pkg/types" | ||
ctrl "sigs.k8s.io/controller-runtime" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
logf "sigs.k8s.io/controller-runtime/pkg/log" | ||
"sigs.k8s.io/controller-runtime/pkg/webhook" | ||
) | ||
|
@@ -12,10 +19,14 @@ const ( | |
) | ||
|
||
// log is for logging in this package. | ||
var jaegerlog = logf.Log.WithName("jaeger-resource") | ||
var ( | ||
jaegerlog = logf.Log.WithName("jaeger-resource") | ||
cl client.Client | ||
) | ||
|
||
// SetupWebhookWithManager adds Jaeger webook to the manager. | ||
func (j *Jaeger) SetupWebhookWithManager(mgr ctrl.Manager) error { | ||
cl = mgr.GetClient() | ||
return ctrl.NewWebhookManagedBy(mgr). | ||
For(j). | ||
Complete() | ||
|
@@ -32,6 +43,19 @@ func (j *Jaeger) Default() { | |
if j.Spec.Storage.Elasticsearch.Name == "" { | ||
j.Spec.Storage.Elasticsearch.Name = defaultElasticsearchName | ||
} | ||
|
||
if ShouldInjectOpenShiftElasticsearchConfiguration(j.Spec.Storage) && j.Spec.Storage.Elasticsearch.DoNotProvision { | ||
// check if ES instance exists | ||
es := &esv1.Elasticsearch{} | ||
err := cl.Get(context.Background(), types.NamespacedName{ | ||
Namespace: j.Namespace, | ||
Name: j.Spec.Storage.Elasticsearch.Name, | ||
}, es) | ||
if errors.IsNotFound(err) { | ||
return | ||
} | ||
j.Spec.Storage.Elasticsearch.NodeCount = OpenShiftElasticsearchNodeCount(es.Spec) | ||
} | ||
} | ||
|
||
// TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. | ||
|
@@ -48,6 +72,18 @@ func (j *Jaeger) ValidateCreate() error { | |
// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type | ||
func (j *Jaeger) ValidateUpdate(_ runtime.Object) error { | ||
jaegerlog.Info("validate update", "name", j.Name) | ||
|
||
if ShouldInjectOpenShiftElasticsearchConfiguration(j.Spec.Storage) && j.Spec.Storage.Elasticsearch.DoNotProvision { | ||
// check if ES instance exists | ||
es := &esv1.Elasticsearch{} | ||
err := cl.Get(context.Background(), types.NamespacedName{ | ||
Namespace: j.Namespace, | ||
Name: j.Spec.Storage.Elasticsearch.Name, | ||
}, es) | ||
if errors.IsNotFound(err) { | ||
return fmt.Errorf("elasticsearch instance not found: %v", err) | ||
} | ||
} | ||
return nil | ||
} | ||
|
||
|
@@ -56,3 +92,21 @@ func (j *Jaeger) ValidateDelete() error { | |
jaegerlog.Info("validate delete", "name", j.Name) | ||
return nil | ||
} | ||
|
||
// OpenShiftElasticsearchNodeCount returns total node count of Elasticsearch nodes. | ||
func OpenShiftElasticsearchNodeCount(spec esv1.ElasticsearchSpec) int32 { | ||
nodes := int32(0) | ||
for i := 0; i < len(spec.Nodes); i++ { | ||
nodes += spec.Nodes[i].NodeCount | ||
} | ||
return nodes | ||
} | ||
|
||
// ShouldInjectOpenShiftElasticsearchConfiguration returns true if OpenShift Elasticsearch is used and its configuration should be used. | ||
func ShouldInjectOpenShiftElasticsearchConfiguration(s JaegerStorageSpec) bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between this function and https://github.com/jaegertracing/jaeger-operator/blob/master/pkg/storage/elasticsearch.go#L26 ? Can we use the one we already have? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The referenced one moved here to the |
||
if s.Type != JaegerESStorage { | ||
return false | ||
} | ||
_, ok := s.Options.Map()["es.server-urls"] | ||
return !ok | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,248 @@ | ||
package v1 | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
esv1 "github.com/openshift/elasticsearch-operator/apis/logging/v1" | ||
corev1 "k8s.io/api/core/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/client-go/kubernetes/scheme" | ||
"sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestDefault(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
objs []runtime.Object | ||
j *Jaeger | ||
expected *Jaeger | ||
}{ | ||
{ | ||
name: "set missing ES name", | ||
j: &Jaeger{ | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "", | ||
}, | ||
}, | ||
}, | ||
}, | ||
expected: &Jaeger{ | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "elasticsearch", | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "set ES node count", | ||
objs: []runtime.Object{ | ||
&corev1.Namespace{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "project1", | ||
}, | ||
}, | ||
&esv1.Elasticsearch{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "my-es", | ||
Namespace: "project1", | ||
}, | ||
Spec: esv1.ElasticsearchSpec{ | ||
Nodes: []esv1.ElasticsearchNode{ | ||
{ | ||
NodeCount: 3, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
j: &Jaeger{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Namespace: "project1", | ||
}, | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Type: "elasticsearch", | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "my-es", | ||
DoNotProvision: true, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expected: &Jaeger{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Namespace: "project1", | ||
}, | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Type: "elasticsearch", | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "my-es", | ||
NodeCount: 3, | ||
DoNotProvision: true, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "do not set ES node count", | ||
j: &Jaeger{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Namespace: "project1", | ||
}, | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Type: "elasticsearch", | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "my-es", | ||
DoNotProvision: false, | ||
NodeCount: 1, | ||
}, | ||
}, | ||
}, | ||
}, | ||
expected: &Jaeger{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Namespace: "project1", | ||
}, | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Type: "elasticsearch", | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "my-es", | ||
NodeCount: 1, | ||
DoNotProvision: false, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
t.Run(test.name, func(t *testing.T) { | ||
esv1.AddToScheme(scheme.Scheme) | ||
AddToScheme(scheme.Scheme) | ||
fakeCl := fake.NewClientBuilder().WithRuntimeObjects(test.objs...).Build() | ||
cl = fakeCl | ||
|
||
test.j.Default() | ||
assert.Equal(t, test.expected, test.j) | ||
}) | ||
} | ||
} | ||
|
||
func TestValidate(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
objsToCreate []runtime.Object | ||
current *Jaeger | ||
err string | ||
}{ | ||
{ | ||
name: "ES instance exists", | ||
objsToCreate: []runtime.Object{ | ||
&corev1.Namespace{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "project1", | ||
}, | ||
}, | ||
&esv1.Elasticsearch{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "my-es", | ||
Namespace: "project1", | ||
}, | ||
Spec: esv1.ElasticsearchSpec{ | ||
Nodes: []esv1.ElasticsearchNode{ | ||
{ | ||
NodeCount: 3, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
current: &Jaeger{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Namespace: "project1", | ||
}, | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Type: "elasticsearch", | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "my-es", | ||
DoNotProvision: true, | ||
}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
{ | ||
name: "ES instance does not exist", | ||
objsToCreate: []runtime.Object{ | ||
&corev1.Namespace{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: "project1", | ||
}, | ||
}, | ||
}, | ||
current: &Jaeger{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Namespace: "project1", | ||
}, | ||
Spec: JaegerSpec{ | ||
Storage: JaegerStorageSpec{ | ||
Type: "elasticsearch", | ||
Elasticsearch: ElasticsearchSpec{ | ||
Name: "my-es", | ||
DoNotProvision: true, | ||
}, | ||
}, | ||
}, | ||
}, | ||
err: `elasticsearch instance not found: elasticsearchs.logging.openshift.io "my-es" not found`, | ||
}, | ||
} | ||
|
||
for _, test := range tests { | ||
t.Run(test.name, func(t *testing.T) { | ||
esv1.AddToScheme(scheme.Scheme) | ||
AddToScheme(scheme.Scheme) | ||
fakeCl := fake.NewClientBuilder().WithRuntimeObjects(test.objsToCreate...).Build() | ||
cl = fakeCl | ||
|
||
err := test.current.ValidateCreate() | ||
if test.err != "" { | ||
assert.Equal(t, test.err, err.Error()) | ||
} else { | ||
assert.Nil(t, err) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestShouldDeployElasticsearch(t *testing.T) { | ||
tests := []struct { | ||
j JaegerStorageSpec | ||
expected bool | ||
}{ | ||
{j: JaegerStorageSpec{}}, | ||
{j: JaegerStorageSpec{Type: JaegerCassandraStorage}}, | ||
{j: JaegerStorageSpec{Type: JaegerESStorage, Options: NewOptions(map[string]interface{}{"es.server-urls": "foo"})}}, | ||
{j: JaegerStorageSpec{Type: JaegerESStorage}, expected: true}, | ||
} | ||
for i, test := range tests { | ||
t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { | ||
assert.Equal(t, test.expected, ShouldInjectOpenShiftElasticsearchConfiguration(test.j)) | ||
}) | ||
} | ||
} |
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 don't like introducing a global state, but another option would be to implement the webhook "manually", which I like even less for validating and defaulting webhooks.
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.
Can we register another object to handle the webhook instead of Jaeger and embed Jaeger?
Just an idea (untested)
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.
that unfortunately does not work. We could add the client to the
Jaeger
but that is even worse.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 this is the "less worse" option we have, I don't like it either but I don't see any other possibility so far.
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.
While I was working on something else I became aware of the custom validator[0].
What do you think about using it? The method calls differ only slightly. In the
SetupWebhookWithManager
the locale client could be passed to the validator.0: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/admission/validator_custom.go#L30-L35