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

Add logging relation #25

Merged
merged 9 commits into from
Nov 12, 2024
Merged

Add logging relation #25

merged 9 commits into from
Nov 12, 2024

Conversation

rgildein
Copy link
Contributor

@rgildein rgildein commented Oct 16, 2024 •
edited
Loading

Add logging relation and use the Loki URL in spark properties generated by integration-hub. This feature requires to be used with latest 3.4.2 charmed-spark image, otherwise it will not work.

Related with: canonical/charmed-spark-rock#114

How I tested it:

$ juju add-model spark
$ juju deploy spark-k8s-bundle --channel 3.4/edge --trust
$ charmcraft pack
$ juju refresh integration-hub --path ./spark-integration-hub-k8s_ubuntu-22.04-amd64.charm --force-units
# configure mino
$ juju consume u2-k8s:cos.loki-logging
$ juju deploy grafana-agent-k8s --channel=stable
$ juju integrate grafana-agent-k8s loki-logging
$ juju integrate grafana-agent-k8s integration-hub:logging

# check the configuration provided by integration-hub
$ spark-client.service-account-registry create --username spark --namespace spark
$ spark-client.service-account-registry get-config --username spark --namespace spark 
spark.eventLog.dir=s3a://spark-tutorial//spark

spark.eventLog.enabled=true

spark.executorEnv.LOKI_URL=http://grafana-agent-k8s-0.grafana-agent-k8s-endpoints.spark.svc.cluster.local:3500/loki/api/v1/push

spark.hadoop.fs.s3a.access.key=2SADHC75MH98XSKFA64C

spark.hadoop.fs.s3a.aws.credentials.provider=org.apache.hadoop.fs.s3a.SimpleAWSCredentialsProvider

spark.hadoop.fs.s3a.connection.ssl.enabled=false

spark.hadoop.fs.s3a.endpoint=http://10.152.183.252

spark.hadoop.fs.s3a.path.style.access=true

spark.hadoop.fs.s3a.secret.key=sufsVr4X0ekcGHuS3Pi6HjN5MSAPGlTNoo0Ph4XY

spark.history.fs.logDirectory=s3a://spark-tutorial//spark

spark.kubernetes.driverEnv.LOKI_URL=http://grafana-agent-k8s-0.grafana-agent-k8s-endpoints.spark.svc.cluster.local:3500/loki/api/v1/push

spark.kubernetes.file.upload.path=s3a://spark-tutorial/

spark.sql.warehouse.dir=s3a://spark-tutorial/warehouse
spark.kubernetes.authenticate.driver.serviceAccountName=spark
spark.kubernetes.namespace=spark

@rgildein rgildein self-assigned this Oct 16, 2024
@rgildein
add cosl lib
96e76a0 
Signed-off-by: Robert Gildein <[email protected]>
@rgildein
Add logging relation
23cd48c 
Add logging relation and use the Loki URL in spark properties
generated by integration-hub.

Signed-off-by: Robert Gildein <[email protected]>
@rgildein rgildein force-pushed the feat/DPE-5633/log-forwarding branch from 7f3ddb2 to ece7923 Compare October 23, 2024 20:19
@rgildein
Add unit and integration tests
bacc00c 
Fixed some small left over for manual testing purpose.

Signed-off-by: Robert Gildein <[email protected]>
@rgildein rgildein force-pushed the feat/DPE-5633/log-forwarding branch 3 times, most recently from 3d0f5f1 to bacc00c Compare October 29, 2024 13:26
@rgildein rgildein force-pushed the feat/DPE-5633/log-forwarding branch from 8a658de to 5c86c35 Compare October 30, 2024 13:15
@rgildein rgildein changed the title [WIP] Add logging relation Add logging relation Oct 30, 2024
@rgildein rgildein marked this pull request as ready for review October 30, 2024 15:51
welpaolo
welpaolo reviewed Nov 6, 2024
View reviewed changes
Copy link
Contributor

@welpaolo welpaolo left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left a comments in the tests

tests/integration/test_charm.py Outdated Show resolved Hide resolved
@rgildein rgildein mentioned this pull request Nov 6, 2024
Merged
@rgildein
Add grafana-agent as fixture
4894259 
Signed-off-by: Robert Gildein <[email protected]>
@rgildein rgildein force-pushed the feat/DPE-5633/log-forwarding branch from 227bb52 to 4894259 Compare November 6, 2024 16:03
deusebio
deusebio approved these changes Nov 6, 2024
View reviewed changes
Copy link
Contributor

@deusebio deusebio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes are exactly what I would have expected! LGTM! Thanks!

@rgildein rgildein requested a review from welpaolo November 7, 2024 09:20
theoctober19th
theoctober19th approved these changes Nov 11, 2024
View reviewed changes
src/constants.py Outdated Show resolved Hide resolved
src/core/context.py Outdated Show resolved Hide resolved
@rgildein
apply suggestions
4111519 
Signed-off-by: Robert Gildein <[email protected]>
theoctober19th
theoctober19th approved these changes Nov 11, 2024
View reviewed changes
Copy link
Member

@theoctober19th theoctober19th left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

welpaolo
welpaolo approved these changes Nov 12, 2024
View reviewed changes
Copy link
Contributor

@welpaolo welpaolo left a comment

Choose a reason for hiding this comment

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

LGTM! Great contribution!

@welpaolo welpaolo merged commit a79180b into main Nov 12, 2024
10 checks passed
@welpaolo welpaolo deleted the feat/DPE-5633/log-forwarding branch November 12, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theoctober19th theoctober19th theoctober19th approved these changes

@welpaolo welpaolo welpaolo approved these changes

@deusebio deusebio Awaiting requested review from deusebio

Assignees

@rgildein rgildein

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rgildein @welpaolo @deusebio @theoctober19th