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

Create service for extensions #3403

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Ankit152
Copy link

@Ankit152 Ankit152 commented Oct 28, 2024

Description:

The Otel Operator doesn't creates a service for extensions. This will help the operator to create Service for extensions which will be consumed by an Ingress so that users can interact with it directly. This is related to deployment of Jaeger V2 in k8s.

Link to tracking Issue(s):

Testing:

Documentation:

@Ankit152 Ankit152 requested a review from a team as a code owner October 28, 2024 14:13
@Ankit152 Ankit152 marked this pull request as draft October 28, 2024 14:15
Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

great progress,

Please add unit tests and e2e test

internal/manifests/collector/service.go Show resolved Hide resolved
@Ankit152 Ankit152 force-pushed the service-extension branch 2 times, most recently from d742325 to eb14bf7 Compare November 7, 2024 08:13
@Ankit152 Ankit152 marked this pull request as ready for review November 7, 2024 08:13
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.

This looks good to me overall, left a few comments and questions.

apis/v1beta1/config.go Show resolved Hide resolved
tests/e2e/extension/00-install.yaml Outdated Show resolved Hide resolved
tests/e2e/extension/00-assert.yaml Show resolved Hide resolved
internal/manifests/collector/service.go Outdated Show resolved Hide resolved
.chloggen/service-extension.yaml Outdated Show resolved Hide resolved
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.

I wanted to be clearer about how I'd like the e2e test you added to look like. I'm including some suggestions to help demonstrate that. Fundamentally, the manifests we assert against should only contain attributes that are relevant to this specific test.

The rest of this PR looks good to me, nicely done. 👍

Comment on lines 92 to 108
apiVersion: v1
kind: ServiceAccount
metadata:
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest
name: jaeger-inmemory-collector
ownerReferences:
- apiVersion: opentelemetry.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: OpenTelemetryCollector
name: jaeger-inmemory
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
apiVersion: v1
kind: ServiceAccount
metadata:
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest
name: jaeger-inmemory-collector
ownerReferences:
- apiVersion: opentelemetry.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: OpenTelemetryCollector
name: jaeger-inmemory
---

Comment on lines 111 to 118
metadata:
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest
operator.opentelemetry.io/collector-service-type: base
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
metadata:
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest
operator.opentelemetry.io/collector-service-type: base

Comment on lines 120 to 125
ownerReferences:
- apiVersion: opentelemetry.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: OpenTelemetryCollector
name: jaeger-inmemory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ownerReferences:
- apiVersion: opentelemetry.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: OpenTelemetryCollector
name: jaeger-inmemory

Comment on lines 127 to 130
internalTrafficPolicy: Cluster
ipFamilies:
- IPv4
ipFamilyPolicy: SingleStack
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
internalTrafficPolicy: Cluster
ipFamilies:
- IPv4
ipFamilyPolicy: SingleStack

Comment on lines 146 to 153
selector:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/part-of: opentelemetry
sessionAffinity: None
type: ClusterIP
status:
loadBalancer: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
selector:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/part-of: opentelemetry
sessionAffinity: None
type: ClusterIP
status:
loadBalancer: {}

Comment on lines 5 to 14
metadata:
annotations:
deployment.kubernetes.io/revision: "1"
generation: 1
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
metadata:
annotations:
deployment.kubernetes.io/revision: "1"
generation: 1
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest

Comment on lines 16 to 21
ownerReferences:
- apiVersion: opentelemetry.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: OpenTelemetryCollector
name: jaeger-inmemory
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ownerReferences:
- apiVersion: opentelemetry.io/v1beta1
blockOwnerDeletion: true
controller: true
kind: OpenTelemetryCollector
name: jaeger-inmemory

Comment on lines 23 to 46
replicas: 1
selector:
matchLabels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/part-of: opentelemetry
strategy:
rollingUpdate:
maxSurge: 25%
maxUnavailable: 25%
type: RollingUpdate
template:
metadata:
annotations:
prometheus.io/path: /metrics
prometheus.io/port: "8888"
prometheus.io/scrape: "true"
creationTimestamp: null
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
replicas: 1
selector:
matchLabels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/part-of: opentelemetry
strategy:
rollingUpdate:
maxSurge: 25%
maxUnavailable: 25%
type: RollingUpdate
template:
metadata:
annotations:
prometheus.io/path: /metrics
prometheus.io/port: "8888"
prometheus.io/scrape: "true"
creationTimestamp: null
labels:
app.kubernetes.io/component: opentelemetry-collector
app.kubernetes.io/managed-by: opentelemetry-operator
app.kubernetes.io/name: jaeger-inmemory-collector
app.kubernetes.io/part-of: opentelemetry
app.kubernetes.io/version: latest

Comment on lines 49 to 59
- args:
- --config=/conf/collector.yaml
env:
- name: POD_NAME
valueFrom:
fieldRef:
apiVersion: v1
fieldPath: metadata.name
image: jaegertracing/jaeger-snapshot:latest
imagePullPolicy: Always
name: otc-container
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- args:
- --config=/conf/collector.yaml
env:
- name: POD_NAME
valueFrom:
fieldRef:
apiVersion: v1
fieldPath: metadata.name
image: jaegertracing/jaeger-snapshot:latest
imagePullPolicy: Always
name: otc-container

Comment on lines 73 to 87
resources: {}
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
volumeMounts:
- mountPath: /conf
name: otc-internal
dnsConfig: {}
dnsPolicy: ClusterFirst
restartPolicy: Always
schedulerName: default-scheduler
securityContext: {}
serviceAccount: jaeger-inmemory-collector
serviceAccountName: jaeger-inmemory-collector
shareProcessNamespace: false
terminationGracePeriodSeconds: 30
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resources: {}
terminationMessagePath: /dev/termination-log
terminationMessagePolicy: File
volumeMounts:
- mountPath: /conf
name: otc-internal
dnsConfig: {}
dnsPolicy: ClusterFirst
restartPolicy: Always
schedulerName: default-scheduler
securityContext: {}
serviceAccount: jaeger-inmemory-collector
serviceAccountName: jaeger-inmemory-collector
shareProcessNamespace: false
terminationGracePeriodSeconds: 30

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.

5 participants