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

New DeploymentStrategy type for JaegerSpec.Strategy #704

Merged
merged 7 commits into from
Oct 22, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion pkg/apis/jaegertracing/v1/jaeger_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,33 @@ const (
IngressSecurityOAuthProxy IngressSecurityType = "oauth-proxy"
)

// DeploymentStrategy represents the possible values for deployment strategies
// +k8s:openapi-gen=true
type DeploymentStrategy string
Copy link
Contributor

Choose a reason for hiding this comment

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

You could implement the UnmarshalJSON method to the type, to ensure the values read from JSON are converted to lowercase.


const (
// DeploymentStrategyDeprecatedAllInOne represents the (deprecated) 'all-in-one' deployment strategy
// +k8s:openapi-gen=true
DeploymentStrategyDeprecatedAllInOne DeploymentStrategy = "all-in-one"

// DeploymentStrategyAllInOne represents the 'allInOne' deployment strategy (default)
// +k8s:openapi-gen=true
DeploymentStrategyAllInOne DeploymentStrategy = "allinone"

// DeploymentStrategyStreaming represents the 'streaming' deployment strategy
// +k8s:openapi-gen=true
DeploymentStrategyStreaming DeploymentStrategy = "streaming"

// DeploymentStrategyProduction represents the 'production' deployment strategy
// +k8s:openapi-gen=true
DeploymentStrategyProduction DeploymentStrategy = "production"
)

// JaegerSpec defines the desired state of Jaeger
// +k8s:openapi-gen=true
type JaegerSpec struct {
// +optional
Strategy string `json:"strategy,omitempty"`
Strategy DeploymentStrategy `json:"strategy,omitempty"`

// +optional
AllInOne JaegerAllInOneSpec `json:"allInOne,omitempty"`
Expand Down
5 changes: 2 additions & 3 deletions pkg/deployment/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"
"sort"
"strconv"
"strings"

"github.com/spf13/viper"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -76,9 +75,9 @@ func (c *Collector) Get() *appsv1.Deployment {
}

storageType := c.jaeger.Spec.Storage.Type
// If strategy is "streaming", then change storage type
// If strategy is DeploymentStrategyStreaming, then change storage type
// to Kafka, and the storage options will be used in the Ingester instead
if strings.EqualFold(c.jaeger.Spec.Strategy, "streaming") {
if c.jaeger.Spec.Strategy == v1.DeploymentStrategyStreaming {
Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if we have a CR with a strategy set to Streaming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was assuming that the value was being normalized somewhere else. Sorry, my bad.

storageType = "kafka"
}
options := allArgs(c.jaeger.Spec.Collector.Options,
Expand Down
4 changes: 2 additions & 2 deletions pkg/deployment/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,7 +377,7 @@ func TestCollectorWithKafkaStorageType(t *testing.T) {
Name: "TestCollectorWithIngesterStorageType",
},
Spec: v1.JaegerSpec{
Strategy: "streaming",
Strategy: v1.DeploymentStrategyStreaming,
Collector: v1.JaegerCollectorSpec{
Options: v1.NewOptions(map[string]interface{}{
"kafka.producer.topic": "mytopic",
Expand Down Expand Up @@ -417,7 +417,7 @@ func TestCollectorWithIngesterNoOptionsStorageType(t *testing.T) {
Name: "TestCollectorWithIngesterNoOptionsStorageType",
},
Spec: v1.JaegerSpec{
Strategy: "streaming",
Strategy: v1.DeploymentStrategyStreaming,
Storage: v1.JaegerStorageSpec{
Type: "elasticsearch",
Options: v1.NewOptions(map[string]interface{}{
Expand Down
3 changes: 1 addition & 2 deletions pkg/deployment/ingester.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"fmt"
"sort"
"strconv"
"strings"

"github.com/spf13/viper"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -41,7 +40,7 @@ func NewIngester(jaeger *v1.Jaeger) *Ingester {

// Get returns a ingester pod
func (i *Ingester) Get() *appsv1.Deployment {
if !strings.EqualFold(i.jaeger.Spec.Strategy, "streaming") {
if i.jaeger.Spec.Strategy != v1.DeploymentStrategyStreaming {
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/deployment/ingester_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ func TestIngesterWithStorageType(t *testing.T) {
Name: "TestIngesterStorageType",
},
Spec: v1.JaegerSpec{
Strategy: "streaming",
Strategy: v1.DeploymentStrategyStreaming,
Ingester: v1.JaegerIngesterSpec{
Options: v1.NewOptions(map[string]interface{}{
"kafka.consumer.topic": "mytopic",
Expand Down Expand Up @@ -394,7 +394,7 @@ func newIngesterJaeger(name string) *v1.Jaeger {
Name: name,
},
Spec: v1.JaegerSpec{
Strategy: "streaming",
Strategy: v1.DeploymentStrategyStreaming,
Ingester: v1.JaegerIngesterSpec{
Options: v1.NewOptions(map[string]interface{}{
"any": "option",
Expand Down
7 changes: 3 additions & 4 deletions pkg/ingress/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@ package ingress

import (
"fmt"
"strings"

extv1beta1 "k8s.io/api/extensions/v1beta1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"

"github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
"github.com/jaegertracing/jaeger-operator/pkg/service"
"github.com/jaegertracing/jaeger-operator/pkg/util"
)
Expand Down Expand Up @@ -49,9 +48,9 @@ func (i *QueryIngress) Get() *extv1beta1.Ingress {
ServiceName: service.GetNameForQueryService(i.jaeger),
ServicePort: intstr.FromInt(service.GetPortForQueryService(i.jaeger)),
}
if _, ok := i.jaeger.Spec.AllInOne.Options.Map()["query.base-path"]; ok && strings.EqualFold(i.jaeger.Spec.Strategy, "allinone") {
if _, ok := i.jaeger.Spec.AllInOne.Options.Map()["query.base-path"]; ok && i.jaeger.Spec.Strategy == v1.DeploymentStrategyAllInOne {
spec.Rules = append(spec.Rules, getRule(i.jaeger.Spec.AllInOne.Options, backend))
} else if _, ok := i.jaeger.Spec.Query.Options.Map()["query.base-path"]; ok && strings.EqualFold(i.jaeger.Spec.Strategy, "production") {
} else if _, ok := i.jaeger.Spec.Query.Options.Map()["query.base-path"]; ok && i.jaeger.Spec.Strategy == v1.DeploymentStrategyProduction {
spec.Rules = append(spec.Rules, getRule(i.jaeger.Spec.Query.Options, backend))
} else {
spec.Backend = &backend
Expand Down
4 changes: 2 additions & 2 deletions pkg/ingress/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func TestQueryIngressAllInOneBasePath(t *testing.T) {
name := "TestQueryIngressAllInOneBasePath"
jaeger := v1.NewJaeger(types.NamespacedName{Name: name})
jaeger.Spec.Ingress.Enabled = &enabled
jaeger.Spec.Strategy = "allInOne"
jaeger.Spec.Strategy = v1.DeploymentStrategyAllInOne
jaeger.Spec.AllInOne.Options = v1.NewOptions(map[string]interface{}{"query.base-path": "/jaeger"})
ingress := NewQueryIngress(jaeger)

Expand All @@ -68,7 +68,7 @@ func TestQueryIngressQueryBasePath(t *testing.T) {
name := "TestQueryIngressQueryBasePath"
jaeger := v1.NewJaeger(types.NamespacedName{Name: name})
jaeger.Spec.Ingress.Enabled = &enabled
jaeger.Spec.Strategy = "production"
jaeger.Spec.Strategy = v1.DeploymentStrategyProduction
jaeger.Spec.Query.Options = v1.NewOptions(map[string]interface{}{"query.base-path": "/jaeger"})
ingress := NewQueryIngress(jaeger)

Expand Down
2 changes: 1 addition & 1 deletion pkg/strategy/all_in_one.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
)

func newAllInOneStrategy(jaeger *v1.Jaeger) S {
c := S{typ: AllInOne}
c := S{typ: v1.DeploymentStrategyAllInOne}
jaeger.Logger().Debug("Creating all-in-one deployment")

dep := deployment.NewAllInOne(jaeger)
Expand Down
18 changes: 9 additions & 9 deletions pkg/strategy/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,19 @@ var (

// For returns the appropriate Strategy for the given Jaeger instance
func For(ctx context.Context, jaeger *v1.Jaeger, secrets []corev1.Secret) S {
if strings.EqualFold(jaeger.Spec.Strategy, "all-in-one") {
if jaeger.Spec.Strategy == v1.DeploymentStrategyDeprecatedAllInOne {
jaeger.Logger().Warn("Strategy 'all-in-one' is no longer supported, please use 'allInOne'")
jaeger.Spec.Strategy = "allInOne"
jaeger.Spec.Strategy = v1.DeploymentStrategyAllInOne
}

normalize(jaeger)

jaeger.Logger().WithField("strategy", jaeger.Spec.Strategy).Debug("Strategy chosen")
if strings.EqualFold(jaeger.Spec.Strategy, "allinone") {
if jaeger.Spec.Strategy == v1.DeploymentStrategyAllInOne {
return newAllInOneStrategy(jaeger)
}

if strings.EqualFold(jaeger.Spec.Strategy, "streaming") {
if jaeger.Spec.Strategy == v1.DeploymentStrategyStreaming {
return newStreamingStrategy(jaeger)
}

Expand Down Expand Up @@ -73,15 +73,15 @@ func normalize(jaeger *v1.Jaeger) {
}

// normalize the deployment strategy
if !strings.EqualFold(jaeger.Spec.Strategy, "production") && !strings.EqualFold(jaeger.Spec.Strategy, "streaming") {
jaeger.Spec.Strategy = "allInOne"
if jaeger.Spec.Strategy != v1.DeploymentStrategyProduction && jaeger.Spec.Strategy != v1.DeploymentStrategyStreaming {
jaeger.Spec.Strategy = v1.DeploymentStrategyAllInOne
}

// check for incompatible options
// if the storage is `memory`, then the only possible strategy is `all-in-one`
if !distributedStorage(jaeger.Spec.Storage.Type) && !strings.EqualFold(jaeger.Spec.Strategy, "allinone") {
jaeger.Logger().WithField("storage", jaeger.Spec.Storage.Type).Warn("No suitable storage provided. Falling back to all-in-one")
jaeger.Spec.Strategy = "allInOne"
if !distributedStorage(jaeger.Spec.Storage.Type) && jaeger.Spec.Strategy != v1.DeploymentStrategyAllInOne {
jaeger.Logger().WithField("storage", jaeger.Spec.Storage.Type).Warn("No suitable storage provided. Falling back to allInOne")
jaeger.Spec.Strategy = v1.DeploymentStrategyAllInOne
}

// we always set the value to None, except when we are on OpenShift *and* the user has not explicitly set to 'none'
Expand Down
32 changes: 16 additions & 16 deletions pkg/strategy/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,24 @@ func TestNewControllerForAllInOneAsDefault(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNewControllerForAllInOneAsDefault"})

ctrl := For(context.TODO(), jaeger, []corev1.Secret{})
assert.Equal(t, ctrl.Type(), AllInOne)
assert.Equal(t, ctrl.Type(), v1.DeploymentStrategyAllInOne)
}

func TestNewControllerForAllInOneAsExplicitValue(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNewControllerForAllInOneAsExplicitValue"})
jaeger.Spec.Strategy = "ALL-IN-ONE" // same as 'all-in-one'
jaeger.Spec.Strategy = v1.DeploymentStrategyDeprecatedAllInOne // same as 'all-in-one'

ctrl := For(context.TODO(), jaeger, []corev1.Secret{})
assert.Equal(t, ctrl.Type(), AllInOne)
assert.Equal(t, ctrl.Type(), v1.DeploymentStrategyAllInOne)
}

func TestNewControllerForProduction(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNewControllerForProduction"})
jaeger.Spec.Strategy = "production"
jaeger.Spec.Strategy = v1.DeploymentStrategyProduction
jaeger.Spec.Storage.Type = "elasticsearch"

ctrl := For(context.TODO(), jaeger, []corev1.Secret{})
assert.Equal(t, ctrl.Type(), Production)
assert.Equal(t, ctrl.Type(), v1.DeploymentStrategyProduction)
}

func TestUnknownStorage(t *testing.T) {
Expand All @@ -45,7 +45,7 @@ func TestUnknownStorage(t *testing.T) {

func TestElasticsearchAsStorageOptions(t *testing.T) {
jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestElasticsearchAsStorageOptions"})
jaeger.Spec.Strategy = "production"
jaeger.Spec.Strategy = v1.DeploymentStrategyProduction
jaeger.Spec.Storage.Type = "elasticsearch"
jaeger.Spec.Storage.Options = v1.NewOptions(map[string]interface{}{
"es.server-urls": "http://elasticsearch-example-es-cluster:9200",
Expand Down Expand Up @@ -75,63 +75,63 @@ func TestDefaultName(t *testing.T) {
func TestIncompatibleMemoryStorageForProduction(t *testing.T) {
jaeger := &v1.Jaeger{
Spec: v1.JaegerSpec{
Strategy: "production",
Strategy: v1.DeploymentStrategyProduction,
Storage: v1.JaegerStorageSpec{
Type: "memory",
},
},
}
normalize(jaeger)
assert.Equal(t, "allInOne", jaeger.Spec.Strategy)
assert.Equal(t, v1.DeploymentStrategyAllInOne, jaeger.Spec.Strategy)
}

func TestIncompatibleBadgerStorageForProduction(t *testing.T) {
jaeger := &v1.Jaeger{
Spec: v1.JaegerSpec{
Strategy: "production",
Strategy: v1.DeploymentStrategyProduction,
Storage: v1.JaegerStorageSpec{
Type: "badger",
},
},
}
normalize(jaeger)
assert.Equal(t, "allInOne", jaeger.Spec.Strategy)
assert.Equal(t, v1.DeploymentStrategyAllInOne, jaeger.Spec.Strategy)
}

func TestIncompatibleStorageForStreaming(t *testing.T) {
jaeger := &v1.Jaeger{
Spec: v1.JaegerSpec{
Strategy: "streaming",
Strategy: v1.DeploymentStrategyStreaming,
Storage: v1.JaegerStorageSpec{
Type: "memory",
},
},
}
normalize(jaeger)
assert.Equal(t, "allInOne", jaeger.Spec.Strategy)
assert.Equal(t, v1.DeploymentStrategyAllInOne, jaeger.Spec.Strategy)
}

func TestDeprecatedAllInOneStrategy(t *testing.T) {
jaeger := &v1.Jaeger{
Spec: v1.JaegerSpec{
Strategy: "all-in-one",
Strategy: v1.DeploymentStrategyDeprecatedAllInOne,
},
}
For(context.TODO(), jaeger, []corev1.Secret{})
assert.Equal(t, "allInOne", jaeger.Spec.Strategy)
assert.Equal(t, v1.DeploymentStrategyAllInOne, jaeger.Spec.Strategy)
}

func TestStorageMemoryOnlyUsedWithAllInOneStrategy(t *testing.T) {
jaeger := &v1.Jaeger{
Spec: v1.JaegerSpec{
Strategy: "production",
Strategy: v1.DeploymentStrategyProduction,
Storage: v1.JaegerStorageSpec{
Type: "memory",
},
},
}
For(context.TODO(), jaeger, []corev1.Secret{})
assert.Equal(t, "allInOne", jaeger.Spec.Strategy)
assert.Equal(t, v1.DeploymentStrategyAllInOne, jaeger.Spec.Strategy)
}

func TestSetSecurityToNoneByDefault(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/strategy/production.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
)

func newProductionStrategy(jaeger *v1.Jaeger, es *storage.ElasticsearchDeployment) S {
c := S{typ: Production}
c := S{typ: v1.DeploymentStrategyProduction}
collector := deployment.NewCollector(jaeger)
query := deployment.NewQuery(jaeger)
agent := deployment.NewAgent(jaeger)
Expand Down
2 changes: 1 addition & 1 deletion pkg/strategy/production_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ func TestOptionsArePassed(t *testing.T) {
Namespace: "simple-prod-ns",
},
Spec: v1.JaegerSpec{
Strategy: "production",
Strategy: v1.DeploymentStrategyProduction,
Storage: v1.JaegerStorageSpec{
Type: "elasticsearch",
Options: v1.NewOptions(map[string]interface{}{
Expand Down
20 changes: 3 additions & 17 deletions pkg/strategy/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ import (
"k8s.io/api/extensions/v1beta1"
rbac "k8s.io/api/rbac/v1"

jv1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

We typically use v1 for Jaeger, and corev1 for k8s.io/api/core/v1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes a lot of sense!

I saw that v1 was already in use and invented a different alias for Jaeger. Didn't think about the possibility of renaming the existing one :). Will do it now.

esv1 "github.com/jaegertracing/jaeger-operator/pkg/storage/elasticsearch/v1"
)

// S knows what type of deployments to build based on a given spec
type S struct {
typ Type
typ jv1.DeploymentStrategy
accounts []v1.ServiceAccount
configMaps []v1.ConfigMap
cronJobs []batchv1beta1.CronJob
Expand All @@ -29,28 +30,13 @@ type S struct {
secrets []v1.Secret
}

// Type represents a specific deployment strategy, like 'all-in-one'
type Type string

const (

// AllInOne represents the 'all-in-one' deployment strategy
AllInOne Type = "allInOne"

// Production represents the 'production' deployment strategy
Production Type = "production"

// Streaming represents the 'streaming' deployment strategy
Streaming Type = "streaming"
)

// New constructs a new strategy from scratch
func New() *S {
return &S{}
}

// Type returns the strategy type for the given strategy
func (s S) Type() Type {
func (s S) Type() jv1.DeploymentStrategy {
return s.typ
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/strategy/streaming.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import (
)

func newStreamingStrategy(jaeger *v1.Jaeger) S {
c := S{typ: Streaming}
c := S{typ: v1.DeploymentStrategyStreaming}
collector := deployment.NewCollector(jaeger)
query := deployment.NewQuery(jaeger)
agent := deployment.NewAgent(jaeger)
Expand Down
Loading