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

Add scale subresource status to the OpenTelemetryCollector CRD status #785

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
21 changes: 20 additions & 1 deletion apis/v1alpha1/opentelemetrycollector_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,31 @@ type OpenTelemetryTargetAllocator struct {
Image string `json:"image,omitempty"`
}

// ScaleSubresourceStatus defines the observed state of the OpenTelemetryCollector's
// scale subresource.
type ScaleSubresourceStatus struct {
// The total number non-terminated pods targeted by this
// OpenTelemetryCollector's deployment or statefulSet.
// +optional
Replicas int32 `json:"replicas,omitempty"`

// The selector used to match the OpenTelemetryCollector's
// deployment or statefulSet pods.
// +optional
Selector string `json:"selector,omitempty"`
}

// OpenTelemetryCollectorStatus defines the observed state of OpenTelemetryCollector.
type OpenTelemetryCollectorStatus struct {
// Replicas is currently not being set and might be removed in the next version.
// +optional
// Deprecated: use "OpenTelemetryCollector.Status.Scale.Replicas" instead.
Replicas int32 `json:"replicas,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be deprecated and eventually removed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, should I remove it in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This status field is currently unused

Copy link
Member

Choose a reason for hiding this comment

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

Right, but I think it's still best to deprecate in one version, removing in another. Even if only to serve as a pattern for future, similar tasks.

Copy link
Member

Choose a reason for hiding this comment

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

This is good, but for reference, we typically add the version it was deprecated, so that we know when it's safe to remove. Like:

 // Deprecated: [v0.47.0] use "OpenTelemetryCollector.Status.Scale.Replicas" instead.

Copy link
Member

@pavolloffay pavolloffay Mar 31, 2022

Choose a reason for hiding this comment

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

As far as I know, properties from the CRD should not be removed. The cleanup can be done only when we promote the CRD to a new version.


// Scale is the OpenTelemetryCollector's scale subresource status.
// +optional
Scale ScaleSubresourceStatus `json:"scale,omitempty"`

// Version of the managed OpenTelemetry Collector (operand)
// +optional
Version string `json:"version,omitempty"`
Expand All @@ -149,7 +168,7 @@ type OpenTelemetryCollectorStatus struct {
// +kubebuilder:object:root=true
// +kubebuilder:resource:shortName=otelcol;otelcols
// +kubebuilder:subresource:status
// +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.replicas
// +kubebuilder:subresource:scale:specpath=.spec.replicas,statuspath=.status.scale.replicas,selectorpath=.status.scale.selector
// +kubebuilder:printcolumn:name="Mode",type="string",JSONPath=".spec.mode",description="Deployment Mode"
// +kubebuilder:printcolumn:name="Version",type="string",JSONPath=".status.version",description="OpenTelemetry Version"
// +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp"
Expand Down
16 changes: 16 additions & 0 deletions apis/v1alpha1/zz_generated.deepcopy.go

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

22 changes: 19 additions & 3 deletions bundle/manifests/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2567,10 +2567,25 @@ spec:
type: array
x-kubernetes-list-type: atomic
replicas:
description: Replicas is currently not being set and might be removed
in the next version.
description: 'Replicas is currently not being set and might be removed
in the next version. Deprecated: use "OpenTelemetryCollector.Status.Scale.Replicas"
instead.'
format: int32
type: integer
scale:
description: Scale is the OpenTelemetryCollector's scale subresource
status.
properties:
replicas:
description: The total number non-terminated pods targeted by
this OpenTelemetryCollector's deployment or statefulSet.
format: int32
type: integer
selector:
description: The selector used to match the OpenTelemetryCollector's
deployment or statefulSet pods.
type: string
type: object
version:
description: Version of the managed OpenTelemetry Collector (operand)
type: string
Expand All @@ -2580,8 +2595,9 @@ spec:
storage: true
subresources:
scale:
labelSelectorPath: .status.scale.selector
specReplicasPath: .spec.replicas
statusReplicasPath: .status.replicas
statusReplicasPath: .status.scale.replicas
status: {}
status:
acceptedNames:
Expand Down
22 changes: 19 additions & 3 deletions config/crd/bases/opentelemetry.io_opentelemetrycollectors.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2565,10 +2565,25 @@ spec:
type: array
x-kubernetes-list-type: atomic
replicas:
description: Replicas is currently not being set and might be removed
in the next version.
description: 'Replicas is currently not being set and might be removed
in the next version. Deprecated: use "OpenTelemetryCollector.Status.Scale.Replicas"
instead.'
format: int32
type: integer
scale:
description: Scale is the OpenTelemetryCollector's scale subresource
status.
properties:
replicas:
description: The total number non-terminated pods targeted by
this OpenTelemetryCollector's deployment or statefulSet.
format: int32
type: integer
selector:
description: The selector used to match the OpenTelemetryCollector's
deployment or statefulSet pods.
type: string
type: object
version:
description: Version of the managed OpenTelemetry Collector (operand)
type: string
Expand All @@ -2578,8 +2593,9 @@ spec:
storage: true
subresources:
scale:
labelSelectorPath: .status.scale.selector
specReplicasPath: .spec.replicas
statusReplicasPath: .status.replicas
statusReplicasPath: .status.scale.replicas
status: {}
status:
acceptedNames:
Expand Down
45 changes: 44 additions & 1 deletion docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -6115,11 +6115,18 @@ OpenTelemetryCollectorStatus defines the observed state of OpenTelemetryCollecto
<td><b>replicas</b></td>
<td>integer</td>
<td>
Replicas is currently not being set and might be removed in the next version.<br/>
Replicas is currently not being set and might be removed in the next version. Deprecated: use "OpenTelemetryCollector.Status.Scale.Replicas" instead.<br/>
<br/>
<i>Format</i>: int32<br/>
</td>
<td>false</td>
</tr><tr>
<td><b><a href="#opentelemetrycollectorstatusscale">scale</a></b></td>
<td>object</td>
<td>
Scale is the OpenTelemetryCollector's scale subresource status.<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>version</b></td>
<td>string</td>
Expand All @@ -6128,4 +6135,40 @@ OpenTelemetryCollectorStatus defines the observed state of OpenTelemetryCollecto
</td>
<td>false</td>
</tr></tbody>
</table>


### OpenTelemetryCollector.status.scale
<sup><sup>[↩ Parent](#opentelemetrycollectorstatus)</sup></sup>



Scale is the OpenTelemetryCollector's scale subresource status.

<table>
<thead>
<tr>
<th>Name</th>
<th>Type</th>
<th>Description</th>
<th>Required</th>
</tr>
</thead>
<tbody><tr>
<td><b>replicas</b></td>
<td>integer</td>
<td>
The total number non-terminated pods targeted by this OpenTelemetryCollector's deployment or statefulSet.<br/>
<br/>
<i>Format</i>: int32<br/>
</td>
<td>false</td>
</tr><tr>
<td><b>selector</b></td>
<td>string</td>
<td>
The selector used to match the OpenTelemetryCollector's deployment or statefulSet pods.<br/>
</td>
<td>false</td>
</tr></tbody>
</table>
69 changes: 63 additions & 6 deletions pkg/collector/reconcile/opentelemetry.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,85 @@ import (
"context"
"fmt"

appsv1 "k8s.io/api/apps/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1"
"github.com/open-telemetry/opentelemetry-operator/internal/version"
"github.com/open-telemetry/opentelemetry-operator/pkg/collector"
"github.com/open-telemetry/opentelemetry-operator/pkg/naming"
)

// Self updates this instance's self data. This should be the last item in the reconciliation, as it causes changes
// making params.Instance obsolete. Default values should be set in the Defaulter webhook, this should only be used
// for the Status, which can't be set by the defaulter.
func Self(ctx context.Context, params Params) error {
if params.Instance.Status.Version != "" {
// a version is already set, let the upgrade mechanism take care of it!
return nil
changed := params.Instance

// this field is only changed for new instances: on existing instances this
// field is reconciled when the operator is first started, i.e. during
// the upgrade mechanism
if params.Instance.Status.Version == "" {
// a version is not set, otherwise let the upgrade mechanism take care of it!
changed.Status.Version = version.OpenTelemetryCollector()
}

changed := params.Instance
changed.Status.Version = version.OpenTelemetryCollector()
if err := updateScaleSubResourceStatus(ctx, params.Client, &changed); err != nil {
return fmt.Errorf("failed to update the scale subresource status for the OpenTelemetry CR: %w", err)
}

// this is only a change for new instances: existing instances are reconciled when the operator is first started
statusPatch := client.MergeFrom(&params.Instance)
if err := params.Client.Status().Patch(ctx, &changed, statusPatch); err != nil {
return fmt.Errorf("failed to apply status changes to the OpenTelemetry CR: %w", err)
}

return nil
}

func updateScaleSubResourceStatus(ctx context.Context, cli client.Client, changed *v1alpha1.OpenTelemetryCollector) error {
mode := changed.Spec.Mode
if mode != v1alpha1.ModeDeployment && mode != v1alpha1.ModeStatefulSet {
changed.Status.Scale.Replicas = 0
changed.Status.Scale.Selector = ""

return nil
}

name := naming.Collector(*changed)

// Set the scale selector
labels := collector.Labels(*changed)
labels["app.kubernetes.io/name"] = name
selector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{MatchLabels: labels})
if err != nil {
return fmt.Errorf("failed to get selector for labelSelector: %w", err)
}
changed.Status.Scale.Selector = selector.String()

// Set the scale replicas
objKey := client.ObjectKey{
Namespace: changed.GetNamespace(),
Name: naming.Collector(*changed),
}

var replicas int32
switch mode { // nolint:exhaustive
case v1alpha1.ModeDeployment:
obj := &appsv1.Deployment{}
if err := cli.Get(ctx, objKey, obj); err != nil {
return fmt.Errorf("failed to get deployment status.replicas: %w", err)
}
replicas = obj.Status.Replicas

case v1alpha1.ModeStatefulSet:
obj := &appsv1.StatefulSet{}
if err := cli.Get(ctx, objKey, obj); err != nil {
return fmt.Errorf("failed to get statefulSet status.replicas: %w", err)
}
replicas = obj.Status.Replicas
}
changed.Status.Scale.Replicas = replicas

return nil
}