Skip to content
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

Merged
merged 12 commits into from
Feb 25, 2022
3 changes: 1 addition & 2 deletions apis/v1/jaeger_types.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
package v1

import (
esv1 "github.com/openshift/elasticsearch-operator/apis/logging/v1"
appsv1 "k8s.io/api/apps/v1"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

esv1 "github.com/openshift/elasticsearch-operator/apis/logging/v1"
)

// IngressSecurityType represents the possible values for the security type
Expand Down
56 changes: 55 additions & 1 deletion apis/v1/jaeger_webhook.go
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"
)
Expand All @@ -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
Copy link
Member Author

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.

Copy link
Member

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)
type JaegerWebhook struct {
	Jaeger

	cl client.Client
}

type Jaeger struct {
	...
}

Copy link
Member Author

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.

Copy link
Collaborator

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.

Copy link
Member

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.

func (r *something) SetupWebhookWithManager(mgr ctrl.Manager) error {
	return ctrl.NewWebhookManagedBy(mgr).
		For(r).
		WithValidator(newSomethingValidator(mgr.GetClient())).
		Complete()
}

0: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/webhook/admission/validator_custom.go#L30-L35

)

// 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()
Expand All @@ -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.
Expand All @@ -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
}

Expand All @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The referenced one moved here to the v1 package.

if s.Type != JaegerESStorage {
return false
}
_, ok := s.Options.Map()["es.server-urls"]
return !ok
}
248 changes: 248 additions & 0 deletions apis/v1/jaeger_webhook_test.go
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))
})
}
}
12 changes: 12 additions & 0 deletions bundle/manifests/jaeger-operator.clusterserviceversion.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,18 @@ spec:
- patch
- update
- watch
- apiGroups:
- logging.openshift.io
resources:
- elasticsearch
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- logging.openshift.io
resources:
Expand Down
12 changes: 12 additions & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,18 @@ rules:
- patch
- update
- watch
- apiGroups:
- logging.openshift.io
resources:
- elasticsearch
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- logging.openshift.io
resources:
Expand Down
Loading