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

TRACING-4752: Add OpenTelemetry-Collector as optional sub-package #4281

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

copejon
Copy link
Contributor

@copejon copejon commented Dec 6, 2024

Which issue(s) this PR addresses:

Closes #

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2024
Copy link
Contributor

openshift-ci bot commented Dec 6, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Dec 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: copejon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 6, 2024
Requires: opentelemetry-collector

%description observability
Demo otel-col config geared for microshift. Client certificates are generated by microshift on start, then placed in
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
Demo otel-col config geared for microshift. Client certificates are generated by microshift on start, then placed in
Demo OpenTelemetry configuration geared for MicroShift. MicroShift OpenTelemetry client certificates are copied to /etc/pki/microshift-observability to facilitate OpenTelemetry collector service connection to MicroShift.

Comment on lines +11 to +13
ca_file: /etc/pki/microshift-opentelemetry-collector-client/client-ca.crt
key_file: /etc/pki/microshift-opentelemetry-collector-client/client.key
cert_file: /etc/pki/microshift-opentelemetry-collector-client/client.crt
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
ca_file: /etc/pki/microshift-opentelemetry-collector-client/client-ca.crt
key_file: /etc/pki/microshift-opentelemetry-collector-client/client.key
cert_file: /etc/pki/microshift-opentelemetry-collector-client/client.crt
ca_file: /etc/pki/microshift-observability/client-ca.crt
key_file: /etc/pki/microshift-observability/client.key
cert_file: /etc/pki/microshift-observability/client.crt

# filelog/workload:
# Path: /….
# filelog/kube-system:
# Path: /….
Copy link
Contributor

Choose a reason for hiding this comment

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

If we'd like to keep comments in the file, can we add an explanation why?

journald:
units:
- microshift
- crio
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to monitor microshift-etcd unit too?

@@ -482,6 +492,10 @@ cat assets/optional/gateway-api/kustomization.x86_64.yaml >> %{buildroot}/%{_pre
mkdir -p -m755 %{buildroot}%{_datadir}/microshift/release
install -p -m644 assets/optional/gateway-api/release-gateway-api-{x86_64,aarch64}.json %{buildroot}%{_datadir}/microshift/release/

#observability
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
#observability
# observability

@@ -645,6 +662,9 @@ fi
%files gateway-api-release-info
%{_datadir}/microshift/release/release-gateway-api-{x86_64,aarch64}.json

%files observability
%config %{_sysconfdir}/microshift/opentelemetry-collector.yaml
%config %{_unitdir}/microshift-observability.service
Copy link
Contributor

Choose a reason for hiding this comment

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

How can we undo the certificate file copy on the package removal?


# It takes a bit for the certs to be created. This service will reach it's burst limit almost immediately, pretty much
# guaranteeing that it will reach the restart limit before it can possibly succeed.
RestartSec=200ms
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to optimize this to avoid service failures / restarts. See the unit section comment.

Comment on lines +5 to +6
AssertPathExists=/var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.key
AssertPathExists=/var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.crt
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is necessary. We declare the current unit After MicroShift, so it should only start when MicroShift declares it's running, right? This means, certificates are created for sure.
What am I missing?

@@ -0,0 +1,27 @@
[Unit]
Description=MicroShift Observability
BindsTo=microshift.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to run the collector even when MicroShift fails?

Comment on lines 9 to 15
ExecStartPre=+/bin/mkdir -p /etc/pki/microshift-observability/
ExecStartPre=+/bin/cp /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.key /etc/pki/microshift-observability/
ExecStartPre=+/bin/cp /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.key /etc/pki/microshift-observability/
ExecStartPre=+/bin/cp /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.crt /etc/pki/microshift-observability/
ExecStartPre=+/bin/chown -R observability /etc/pki/microshift-observability
ExecStartPre=+/bin/chmod -R 600 /etc/pki/microshift-observability/
ExecStartPre=+/bin/chmod 755 /etc/pki/microshift-observability
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
ExecStartPre=+/bin/mkdir -p /etc/pki/microshift-observability/
ExecStartPre=+/bin/cp /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.key /etc/pki/microshift-observability/
ExecStartPre=+/bin/cp /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.key /etc/pki/microshift-observability/
ExecStartPre=+/bin/cp /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.crt /etc/pki/microshift-observability/
ExecStartPre=+/bin/chown -R observability /etc/pki/microshift-observability
ExecStartPre=+/bin/chmod -R 600 /etc/pki/microshift-observability/
ExecStartPre=+/bin/chmod 755 /etc/pki/microshift-observability
ExecStartPre=+/bin/mkdir -p -m 755 /etc/pki/microshift-observability/
ExecStartPre=+/bin/install -o observability -m 600 /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.key /etc/pki/microshift-observability/client.key
ExecStartPre=+/bin/install -o observability -m 600 /var/lib/microshift/certs/kube-apiserver-localhost-signer/observability/client.crt /etc/pki/microshift-observability/client.crt

@ggiguash
Copy link
Contributor

ggiguash commented Dec 9, 2024

/retitle NO-ISSUE: OpenTelemetry certificates and service for MicroShift

@openshift-ci openshift-ci bot changed the title No issue generate otel cert NO-ISSUE: OpenTelemetry certificates and service for MicroShift Dec 9, 2024
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Dec 9, 2024
@openshift-ci-robot
Copy link

@copejon: This pull request explicitly references no jira issue.

In response to this:

Which issue(s) this PR addresses:

Closes #

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link
Contributor

openshift-ci bot commented Dec 10, 2024

@copejon: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

# specifies how frequently the conditions for compaction are being checked
check_interval: 5s # Default

service:
Copy link
Member

Choose a reason for hiding this comment

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

Docs for internal collector metrics https://opentelemetry.io/docs/collector/internal-telemetry/#configure-internal-metrics

it would be good to send them as well so users can monitor the collector.

@copejon copejon changed the title NO-ISSUE: OpenTelemetry certificates and service for MicroShift TRACING-4752: Add OpenTelemetry-Collector as optional sub-package Dec 12, 2024
@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 12, 2024

@copejon: This pull request references TRACING-4752 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

Which issue(s) this PR addresses:

Closes #

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@copejon
Copy link
Contributor Author

copejon commented Dec 12, 2024

/jira refresh

@openshift-ci-robot
Copy link

openshift-ci-robot commented Dec 12, 2024

@copejon: This pull request references TRACING-4752 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

implemented opentelemetry-collector in packaging pipeline. the otel binary is unable to see the cert files, but the config paths are correct and the files exist. file permissions for the observability user have been checked, but are good. WIP

Signed-off-by: Jon Cope <[email protected]>
ensure the otelcol process creates necessary dirs

added firewall port handling for otel-col exporter

Signed-off-by: Jon Cope <[email protected]>
… to allow the collection of pod logs from the host filesystem

Signed-off-by: Jon Cope <[email protected]>
@copejon copejon force-pushed the no-issue-generate-otel-cert branch from fa4f579 to fede276 Compare December 12, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants