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

Added consistent hashing strategy #1087

Merged
merged 12 commits into from
Sep 16, 2022
7 changes: 7 additions & 0 deletions apis/v1alpha1/opentelemetrycollector_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,14 @@ type OpenTelemetryCollectorSpec struct {

// OpenTelemetryTargetAllocator defines the configurations for the Prometheus target allocator.
type OpenTelemetryTargetAllocator struct {
// Replicas is the number of pod instances for the underlying TargetAllocator, this should only be set to a value
// other than 1 if a strategy that allows for high availability is chosen. Currently, the only allocation strategy
// that can be run in a high availability mode is consistent-hashing.
// +optional
Replicas *int32 `json:"replicas,omitempty"`
// AllocationStrategy determines which strategy the target allocator should use for allocation
// The current options are least-weighted and consistent-hashing. The default option is least-weighted
// +optional
AllocationStrategy string `json:"allocationStrategy,omitempty"`
// ServiceAccount indicates the name of an existing service account to use with this instance.
// +optional
Expand Down
9 changes: 6 additions & 3 deletions apis/v1alpha1/opentelemetrycollector_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,15 @@ func (r *OpenTelemetryCollector) Default() {
r.Labels["app.kubernetes.io/managed-by"] = "opentelemetry-operator"
}

// We can default to one because dependent objects Deployment and HorizontalPodAutoScaler
// default to 1 as well.
one := int32(1)
if r.Spec.Replicas == nil {
// We can default to one because dependent objects Deployment and HorizontalPodAutoScaler
// default to 1 as well.
one := int32(1)
r.Spec.Replicas = &one
}
if r.Spec.TargetAllocator.Enabled && r.Spec.TargetAllocator.Replicas == nil {
r.Spec.TargetAllocator.Replicas = &one
Copy link
Member

Choose a reason for hiding this comment

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

Is it safe to share the pointer here? If these values are ever modified is it always by assigning a new pointer or is it ever possible that the underlying value will be changed?

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 think it's okay to modify because this is just for the webhook. The object passed to the operator wouldn't have a spec with a shared pointer (this worked in my testing)

}
}

// +kubebuilder:webhook:verbs=create;update,path=/validate-opentelemetry-io-v1alpha1-opentelemetrycollector,mutating=false,failurePolicy=fail,groups=opentelemetry.io,resources=opentelemetrycollectors,versions=v1alpha1,name=vopentelemetrycollectorcreateupdate.kb.io,sideEffects=none,admissionReviewVersions=v1
Expand Down
7 changes: 6 additions & 1 deletion apis/v1alpha1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 11 additions & 1 deletion bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -809,7 +809,9 @@ spec:
properties:
allocationStrategy:
description: AllocationStrategy determines which strategy the
target allocator should use for allocation
target allocator should use for allocation The current options
are least-weighted and consistent-hashing. The default option
is least-weighted
type: string
enabled:
description: Enabled indicates whether to use a target allocation
Expand All @@ -831,6 +833,14 @@ spec:
custom resources as targets or not.
type: boolean
type: object
replicas:
description: Replicas is the number of pod instances for the underlying
TargetAllocator, this should only be set to a value other than
1 if a strategy that allows for high availability is chosen.
Currently, the only allocation strategy that can be run in a
high availability mode is consistent-hashing.
format: int32
type: integer
serviceAccount:
description: ServiceAccount indicates the name of an existing
service account to use with this instance.
Expand Down
8 changes: 5 additions & 3 deletions cmd/otel-allocator/allocation/consistent_hashing.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package allocation

import (
"fmt"
"net/url"
"sync"

"github.com/buraksezer/consistent"
"github.com/cespare/xxhash/v2"
"github.com/go-logr/logr"
"github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/diff"
"github.com/prometheus/client_golang/prometheus"
"net/url"
"sync"

"github.com/open-telemetry/opentelemetry-operator/cmd/otel-allocator/diff"
)

var _ Allocator = &consistentHashingAllocator{}
Expand Down
3 changes: 2 additions & 1 deletion cmd/otel-allocator/allocation/consistent_hashing_test.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package allocation

import (
"github.com/stretchr/testify/assert"
"testing"

"github.com/stretchr/testify/assert"
)

func TestCanSetSingleTarget(t *testing.T) {
Expand Down
4 changes: 2 additions & 2 deletions cmd/otel-allocator/allocation/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,15 +159,15 @@ func TestGetAllTargetsByCollectorAndJob(t *testing.T) {
"test-label": "test-value",
"foo": "bar",
},
TargetURL: "test-url",
TargetURL: "test-url",
CollectorName: "test-collector",
},
TargetItem{
JobName: "test-job",
Label: model.LabelSet{
"test-label": "test-value",
},
TargetURL: "test-url",
TargetURL: "test-url",
CollectorName: "test-collector",
},
},
Expand Down
2 changes: 1 addition & 1 deletion cmd/otel-allocator/allocation/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ package allocation
import (
"errors"
"fmt"
"github.com/buraksezer/consistent"
"net/url"

"github.com/buraksezer/consistent"
"github.com/go-logr/logr"
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
Expand Down
12 changes: 11 additions & 1 deletion config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,9 @@ spec:
properties:
allocationStrategy:
description: AllocationStrategy determines which strategy the
target allocator should use for allocation
target allocator should use for allocation The current options
jaronoff97 marked this conversation as resolved.
Show resolved Hide resolved
are least-weighted and consistent-hashing. The default option
is least-weighted
type: string
enabled:
description: Enabled indicates whether to use a target allocation
Expand All @@ -829,6 +831,14 @@ spec:
custom resources as targets or not.
type: boolean
type: object
replicas:
description: Replicas is the number of pod instances for the underlying
TargetAllocator, this should only be set to a value other than
jaronoff97 marked this conversation as resolved.
Show resolved Hide resolved
1 if a strategy that allows for high availability is chosen.
Currently, the only allocation strategy that can be run in a
high availability mode is consistent-hashing.
format: int32
type: integer
serviceAccount:
description: ServiceAccount indicates the name of an existing
service account to use with this instance.
Expand Down
11 changes: 10 additions & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -3116,7 +3116,7 @@ TargetAllocator indicates a value which determines whether to spawn a target all
<td><b>allocationStrategy</b></td>
<td>string</td>
<td>
AllocationStrategy determines which strategy the target allocator should use for allocation<br/>
AllocationStrategy determines which strategy the target allocator should use for allocation The current options are least-weighted and consistent-hashing. The default option is least-weighted<br/>
</td>
<td>false</td>
</tr><tr>
Expand All @@ -3140,6 +3140,15 @@ TargetAllocator indicates a value which determines whether to spawn a target all
PrometheusCR defines the configuration for the retrieval of PrometheusOperator CRDs ( servicemonitor.monitoring.coreos.com/v1 and podmonitor.monitoring.coreos.com/v1 ) retrieval. All CR instances which the ServiceAccount has access to will be retrieved. This includes other namespaces.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>replicas</b></td>
<td>integer</td>
<td>
Replicas is the number of pod instances for the underlying TargetAllocator, this should only be set to a value other than 1 if a strategy that allows for high availability is chosen. Currently, the only allocation strategy that can be run in a high availability mode is consistent-hashing.<br/>
<br/>
<i>Format</i>: int32<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>serviceAccount</b></td>
<td>string</td>
Expand Down
59 changes: 58 additions & 1 deletion pkg/collector/reconcile/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ func TestExpectedDeployments(t *testing.T) {

t.Run("should not update target allocator deployment replicas when collector max replicas is set", func(t *testing.T) {
replicas, maxReplicas := int32(2), int32(10)
oneReplica := int32(1)
param := Params{
Client: k8sClient,
Instance: v1alpha1.OpenTelemetryCollector{
Expand All @@ -144,7 +145,8 @@ func TestExpectedDeployments(t *testing.T) {
Replicas: &replicas,
Mode: v1alpha1.ModeStatefulSet,
TargetAllocator: v1alpha1.OpenTelemetryTargetAllocator{
Enabled: true,
Enabled: true,
Replicas: &oneReplica,
},
Config: `
receivers:
Expand Down Expand Up @@ -177,6 +179,61 @@ func TestExpectedDeployments(t *testing.T) {
assert.Equal(t, *allocator.Spec.Replicas, int32(1))
})

t.Run("should update target allocator deployment replicas when changed", func(t *testing.T) {
initialReplicas, nextReplicas := int32(1), int32(2)
param := Params{
Client: k8sClient,
Instance: v1alpha1.OpenTelemetryCollector{
TypeMeta: metav1.TypeMeta{
Kind: "opentelemetry.io",
APIVersion: "v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
UID: instanceUID,
},
Spec: v1alpha1.OpenTelemetryCollectorSpec{
Replicas: &initialReplicas,
Mode: v1alpha1.ModeStatefulSet,
TargetAllocator: v1alpha1.OpenTelemetryTargetAllocator{
Enabled: true,
Replicas: &initialReplicas,
},
Config: `
receivers:
jaeger:
protocols:
grpc:
processors:

exporters:
logging:

service:
pipelines:
traces:
receivers: [jaeger]
processors: []
exporters: [logging]

`,
},
},
Scheme: testScheme,
Log: logger,
}
expected := []v1.Deployment{}
allocator := targetallocator.Deployment(param.Config, param.Log, param.Instance)
expected = append(expected, allocator)

assert.Len(t, expected, 1)
assert.Equal(t, *allocator.Spec.Replicas, int32(1))
param.Instance.Spec.TargetAllocator.Replicas = &nextReplicas
finalAllocator := targetallocator.Deployment(param.Config, param.Log, param.Instance)
assert.Equal(t, *finalAllocator.Spec.Replicas, int32(2))
})

t.Run("should update deployment", func(t *testing.T) {
createObjectIfNotExists(t, "test-collector", &expectedDeploy)
err := expectedDeployments(context.Background(), param, []v1.Deployment{expectedDeploy})
Expand Down
4 changes: 1 addition & 3 deletions pkg/targetallocator/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,14 @@ func Deployment(cfg config.Config, logger logr.Logger, otelcol v1alpha1.OpenTele
labels := Labels(otelcol)
labels["app.kubernetes.io/name"] = naming.TargetAllocator(otelcol)

var replicas int32 = 1

return appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
Name: naming.TargetAllocator(otelcol),
Namespace: otelcol.Namespace,
Labels: labels,
},
Spec: appsv1.DeploymentSpec{
Replicas: &replicas,
Replicas: otelcol.Spec.TargetAllocator.Replicas,
Selector: &metav1.LabelSelector{
MatchLabels: labels,
},
Expand Down