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.

Sign up for GitHub

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

Jump to bottom

[feature request] support scale subresource for mode: daemonset #1667

Open
nazarewk opened this issue Apr 19, 2023 · 5 comments · May be fixed by #1779
Open

[feature request] support scale subresource for mode: daemonset #1667

nazarewk opened this issue Apr 19, 2023 · 5 comments · May be fixed by #1779
Labels
area:collector Issues for deploying collector help wanted Extra attention is needed

Comments

@nazarewk
Copy link

nazarewk commented Apr 19, 2023 •
edited

Looks like Vertical Pod Autoscaler works only with deployment and statufulset modes, because the operator implements only those two:

opentelemetry-operator/pkg/collector/reconcile/opentelemetry.go

Lines 58 to 70 in 2d18b7b

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)

It would be useful to have the same working for daemonset mode.

Is there any particular reason why it was omitted from #785 ?

field similar to replicas is present in DaemonSet.status https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/daemon-set-v1/#DaemonSetStatus

resources:
@yuriolisa
Copy link
Contributor

I have no objections to implementing it. @jaronoff97, @TylerHelmuth, @pavolloffay WDYT?

@jaronoff97
Copy link
Contributor

yeah this would be great to have, not sure why we didn't have it already.

@TylerHelmuth
Copy link
Member

Let's do it

@pavolloffay
Copy link
Member

No objections, how does the scaling work with daemonset? Will it schedule additional pods on the missing nodes?

@pavolloffay pavolloffay added area:collector Issues for deploying collector help wanted Extra attention is needed labels May 24, 2023
@KitSutliff
Copy link
Contributor

Seems like a good idea. Would be glad to have a go at it.

Just to clarify, we want to set a selector on the daemon set so that the vertical pod autoscaler can scale the collector, correct?

We could probably handle sidecar with similar logic. Should we set selectors for that too while we're at it?

@KitSutliff KitSutliff linked a pull request Jun 1, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area:collector Issues for deploying collector help wanted Extra attention is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

adds vpa daemonset handling KitSutliff/opentelemetry-operator
6 participants
@nazarewk @pavolloffay @jaronoff97 @TylerHelmuth @KitSutliff @yuriolisa