Skip to content

✨ Federated learning obs sidecar - #62

Merged
openshift-merge-bot[bot] merged 30 commits into
open-cluster-management-io:mainfrom
mrrr3d:obs_sidecar
Sep 1, 2025
Merged

✨ Federated learning obs sidecar#62
openshift-merge-bot[bot] merged 30 commits into
open-cluster-management-io:mainfrom
mrrr3d:obs_sidecar

Conversation

@mrrr3d

@mrrr3d mrrr3d commented Aug 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

Federated learning obs sidecar.

Related issue(s)

open-cluster-management-io/ocm#825

Fixes #

@mrrr3d
mrrr3d marked this pull request as ready for review August 7, 2025 13:25
@openshift-ci
openshift-ci Bot requested review from qiujian16 and yanmxa August 7, 2025 13:25
yanmxa
yanmxa reviewed Aug 19, 2025
View reviewed changes
Comment thread federated-learning-controller/examples/flower/app-torch/app_torch/client_app.py Outdated
yanmxa
yanmxa reviewed Aug 19, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/controller/manifests/client/manifestwork.yaml Outdated
yanmxa
yanmxa reviewed Aug 19, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/exporter/exporter.go
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/Makefile Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/docs/configure-environment-observability.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/docs/configure-environment-observability.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/docs/configure-environment-observability.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/controller/federatedlearning_client.go Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/controller/federatedlearning_server.go Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/controller/manifests/client/manifestwork.yaml Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/README.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/README.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/README.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes

@yanmxa yanmxa left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall Review

This PR adds valuable observability capabilities through a well-designed sidecar pattern. The implementation is solid but needs some polish based on previous feedback.

Key Issues to Address:

  1. Annotation naming standardization
  2. Makefile build target inconsistency
  3. Hard-coded OTel endpoint
  4. Missing resource limits

The core functionality is good - just need to clean up these items before merge.

yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/controller/manifests/client/manifestwork.yaml Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/docs/configure-environment-observability.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/exporter/exporter.go
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/docs/configure-environment-observability.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/README.md Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/exporter/exporter.go Outdated
yanmxa
yanmxa reviewed Aug 22, 2025
View reviewed changes
Comment thread federated-learning-controller/internal/sidecar/main.go Outdated
mrrr3d and others added 20 commits August 29, 2025 17:02
@mrrr3d
extract write_metrics functionality and update metrics handling
41ba575 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
get obs-sidecar name through annotation
aaef7f9 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
make Obs sidecar image optional
e49ab41 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
add application level observability guide
357ed00 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
update observability configuration guide
273244f 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
add some comment of sidecar
b1c27f6 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
update sidecar docker build process
f1e1697 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
update sidecar image annotation to federated-learning.io/sidecar-image
36f472b 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
update namespace attribute key to pod.namespace
78e7cb4 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
update image name to federated-learning-sidecar
536c3a4 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
remove internal/sidecar/README.md
e726cab 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
change metricfile to json
86172f7 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
use quay.io
143d5c9 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
introduce constant for sidecar image annotation
430841a 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
prevent panic when sidecar image annotation is nil
3fa36c7 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
add OTEL_ENDPOINT environment variable to client and server jobs
c0893a7 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
implement support for metric labels
1e1e2b0 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
improve metric handling and callback registration
804522e 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@mrrr3d
enhance metrics Reporter comments
a35ce98 
Signed-off-by: mrrr61 <mrrr61@outlook.com>
@yanmxa @mrrr3d
patch
8aa82fd 
Signed-off-by: myan <myan@redhat.com>
@yanmxa

yanmxa commented Sep 1, 2025

Copy link
Copy Markdown
Member

/lgtm

@openshift-ci openshift-ci Bot added the lgtm label Sep 1, 2025
@yanmxa

yanmxa commented Sep 1, 2025

Copy link
Copy Markdown
Member

/approve

@openshift-ci

openshift-ci Bot commented Sep 1, 2025

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrrr3d, yanmxa

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

The pull request process is described here

Details 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 label Sep 1, 2025
@openshift-merge-bot
openshift-merge-bot Bot merged commit de1cf82 into open-cluster-management-io:main Sep 1, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@qiujian16 qiujian16 Awaiting requested review from qiujian16
1 more reviewer
@yanmxa yanmxa yanmxa left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@yanmxa yanmxa

Labels

approved lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mrrr3d @yanmxa @haoqing0110 @openshift-merge-robot