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

feat: Add tests and add feature to update daemonset to have selector #2665

Merged

Conversation

rivToadd
Copy link
Contributor

@rivToadd rivToadd commented Feb 26, 2024

Description:

add feature to update daemonset to have selector field

Link to tracking Issue(s):

Testing:
wrote unit tests for said feature

ran test on local kind cluster:

yaml spec used to deploy:

apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: logs
  labels:
    app: logs
spec:
  mode: daemonset
  hostNetwork: true
  config: |
    receivers:
      jaeger:
        protocols:
          grpc:
    processors:

    exporters:
      debug:
        verbosity: detailed

    service:
      pipelines:
        traces:
          receivers: [jaeger]
          processors: []
          exporters: [debug]
  priorityClassName: system-cluster-critical
  resources:
    limits:
      memory: 128Mi
    requests:
      cpu: 10m
      memory: 128Mi
  tolerations:
  - operator: Exists
  volumeMounts:
  - mountPath: /var/log
    name: varlog
    readOnly: true
  volumes:
  - hostPath:
      path: /var/log
    name: varlog

---

apiVersion: autoscaling.k8s.io/v1
kind: VerticalPodAutoscaler
metadata:
  name: logs-collector
spec:
  resourcePolicy:
    containerPolicies:
    - containerName: '*'
      controlledResources:
      - cpu
      - memory
  targetRef:
    apiVersion: opentelemetry.io/v1alpha1
    kind: OpenTelemetryCollector
    name: logs
  updatePolicy:
    updateMode: Auto

Logs that I saw on VPA

I0226 22:37:30.086614       1 matcher.go:68] Let's choose from 1 configs for pod default/logs-collector-%
I0226 22:37:30.086717       1 recommendation_provider.go:91] updating requirements for pod logs-collector-%.
I0226 22:37:30.086839       1 server.go:112] Sending patches: [{add /spec/containers/0/resources/requests/memory 262144k} {add /spec/containers/0/resources/requests/cpu 25m} {add /spec/containers/0/resources/limits/memory 262144k} {add /metadata/annotations/vpaUpdates Pod resources updated by logs-collector: container 0: memory request, cpu request, memory limit} {add /metadata/annotations/vpaObservedContainers otc-container}]

resulting in new resource limits on collector pod

   Limits:
      memory:  262144k
    Requests:
      cpu:     25m
      memory:  262144k

Documentation:

@rivToadd rivToadd requested a review from a team as a code owner February 26, 2024 07:52
@rivToadd rivToadd force-pushed the enable-selectors-daemonset-status branch from 8df903d to 795e4f6 Compare February 26, 2024 07:56
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM overall. Have you tested if the VPA works correctly after this change?

internal/status/collector/collector.go Outdated Show resolved Hide resolved
@rivToadd rivToadd force-pushed the enable-selectors-daemonset-status branch from 795e4f6 to 7887eb3 Compare February 26, 2024 17:35
@rivToadd
Copy link
Contributor Author

@swiatekm-sumo I've updated the PR description to document the local testing that I did.

@jaronoff97 jaronoff97 merged commit 1e80370 into open-telemetry:main Feb 28, 2024
29 checks passed
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VPA does not work when using DaemonSet mode
3 participants