Add dataverse exporter sidecar for feedback/transcripts collection#102
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds a Dataverse exporter sidecar to LCore deployments when data collection is enabled. The operator now manages an exporter ConfigMap with service ID and ingress configuration, wires the sidecar into the Pod with shared user data volumes, extends RBAC rules to access cluster resources, and validates the setup through KUTTL test assertions. ChangesDataverse Exporter Sidecar Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/controller/lcore_config.go (3)
210-210: ⚖️ Poor tradeoffHardcoded ingress URL may limit environment flexibility.
The
ingress_server_urlis hardcoded to Red Hat's production console API. Consider making this configurable via the OpenStackLightspeed CR spec to support different environments (dev, staging) or on-premise deployments where users might need to point to a different ingress endpoint.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/lcore_config.go` at line 210, The hardcoded ingress_server_url value should be made configurable: add a new field (e.g., IngressServerURL) to the OpenStackLightspeed CR spec and update the code that builds lcore_config (the ingress_server_url assignment in internal/controller/lcore_config.go) to read that field with a sensible default of "https://console.redhat.com/api/ingress/v1/upload" when the CR value is empty; ensure the CR field is documented and used wherever lcore_config is constructed so dev/staging/on‑prem deployments can override the endpoint.
209-218: 💤 Low valueConsider using yaml.Marshal for consistency.
The exporter config is built using
fmt.Sprintf, whilebuildLCoreConfigYAML(line 234) usesyaml.Marshalfor structured config generation. While the current approach works for this simple static configuration, usingyaml.Marshalwould provide better consistency and type safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/lcore_config.go` around lines 209 - 218, Summary: exporterConfig is built with fmt.Sprintf instead of using yaml.Marshal like buildLCoreConfigYAML, reducing consistency and type safety. Replace the raw fmt.Sprintf block that assigns exporterConfig with a small struct (or map) whose fields match the YAML keys (service_id, ingress_server_url, allowed_subdirs, collection_interval, cleanup_after_send, ingress_connection_timeout), set service_id using ServiceIDRHOSO, call yaml.Marshal on that struct, and assign the result to exporterConfig; ensure you handle and return/log any marshal error similarly to other error handling in buildLCoreConfigYAML.
211-214: 💤 Low valueDocument the purpose of the "config_status" subdirectory.
The
allowed_subdirsincludes "config_status" in addition to "feedback" and "transcripts", but this is not mentioned in the PR description or comments. Consider adding a comment explaining what data the exporter collects in this subdirectory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/lcore_config.go` around lines 211 - 214, The allowed_subdirs list currently includes "config_status" but lacks documentation; in internal/controller/lcore_config.go add a concise comment near the allowed_subdirs variable (or the constant/struct that defines it) that explains the purpose of the "config_status" subdirectory and exactly what data the exporter writes there (e.g., exporter-generated configuration snapshots, validation results, status metadata, timestamps, etc.), so reviewers and maintainers understand what to expect from the exporter and why it's allowed alongside "feedback" and "transcripts".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/lcore_deployment.go`:
- Line 131: The exporter sidecar currently sets ImagePullPolicy to
corev1.PullAlways; change this to corev1.PullIfNotPresent in the exporter
container spec (where ImagePullPolicy is assigned for the exporter sidecar) to
improve production stability, and ensure image tags are versioned (not :latest);
optionally make the policy configurable via a constant or Deployment field
(e.g., Exporter ImagePullPolicy variable) so it can be overridden in testing if
needed.
In `@internal/controller/lcore_reconciler.go`:
- Around line 119-130: The RBAC currently grants get access to the "pull-secret"
Secret although no code references it; either remove the Resource/ResourceNames
entry for "secrets"/"pull-secret" from the ClusterRole (to least-privilege) or
explicitly use the secret where intended—e.g., add code that reads the Secret
for image registry credentials or dataverse auth and reference it in deployment
logic; search for clusterversions usage in ocp_version.go and the watch setup in
openstacklightspeed_controller.go to confirm clusterversions stays, and if you
keep the pull-secret permission instead of using it in code, add a short note in
deployment/security docs explaining why the operator needs get on the
"pull-secret" Secret and how it is used.
---
Nitpick comments:
In `@internal/controller/lcore_config.go`:
- Line 210: The hardcoded ingress_server_url value should be made configurable:
add a new field (e.g., IngressServerURL) to the OpenStackLightspeed CR spec and
update the code that builds lcore_config (the ingress_server_url assignment in
internal/controller/lcore_config.go) to read that field with a sensible default
of "https://console.redhat.com/api/ingress/v1/upload" when the CR value is
empty; ensure the CR field is documented and used wherever lcore_config is
constructed so dev/staging/on‑prem deployments can override the endpoint.
- Around line 209-218: Summary: exporterConfig is built with fmt.Sprintf instead
of using yaml.Marshal like buildLCoreConfigYAML, reducing consistency and type
safety. Replace the raw fmt.Sprintf block that assigns exporterConfig with a
small struct (or map) whose fields match the YAML keys (service_id,
ingress_server_url, allowed_subdirs, collection_interval, cleanup_after_send,
ingress_connection_timeout), set service_id using ServiceIDRHOSO, call
yaml.Marshal on that struct, and assign the result to exporterConfig; ensure you
handle and return/log any marshal error similarly to other error handling in
buildLCoreConfigYAML.
- Around line 211-214: The allowed_subdirs list currently includes
"config_status" but lacks documentation; in internal/controller/lcore_config.go
add a concise comment near the allowed_subdirs variable (or the constant/struct
that defines it) that explains the purpose of the "config_status" subdirectory
and exactly what data the exporter writes there (e.g., exporter-generated
configuration snapshots, validation results, status metadata, timestamps, etc.),
so reviewers and maintainers understand what to expect from the exporter and why
it's allowed alongside "feedback" and "transcripts".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25ea4017-87e5-45ac-98da-37329738162e
📒 Files selected for processing (24)
internal/controller/constants.gointernal/controller/errors.gointernal/controller/lcore_config.gointernal/controller/lcore_deployment.gointernal/controller/lcore_reconciler.gotest/kuttl/common/openstack-lightspeed-instance/assert-exporter-config.yamltest/kuttl/common/openstack-lightspeed-instance/assert-openstack-lightspeed-instance.yamltest/kuttl/tests/basic-openstack-lightspeed-configuration/05-assert-exporter-config.yamltest/kuttl/tests/basic-openstack-lightspeed-configuration/06-assert-openstack-lightspeed-instance.yamltest/kuttl/tests/basic-openstack-lightspeed-configuration/07-cleanup-openstack-lightspeed-instance.yamltest/kuttl/tests/basic-openstack-lightspeed-configuration/08-errors-openstack-lightspeed-instance.yamltest/kuttl/tests/basic-openstack-lightspeed-configuration/09-cleanup-mock-objects.yamltest/kuttl/tests/basic-openstack-lightspeed-configuration/10-errors-mock-objects.yamltest/kuttl/tests/update-openstacklightspeed/05-assert-exporter-config.yamltest/kuttl/tests/update-openstacklightspeed/06-assert-openstack-lightspeed-instance.yamltest/kuttl/tests/update-openstacklightspeed/07-update-openstack-lightspeed-instance.yamltest/kuttl/tests/update-openstacklightspeed/08-assert-configmaps-update.yamltest/kuttl/tests/update-openstacklightspeed/09-assert-lightspeed-stack-config-update.yamltest/kuttl/tests/update-openstacklightspeed/10-assert-llama-stack-config-update.yamltest/kuttl/tests/update-openstacklightspeed/11-assert-openstacklightspeed-update.yamltest/kuttl/tests/update-openstacklightspeed/12-cleanup-openstack-lightspeed-instance.yamltest/kuttl/tests/update-openstacklightspeed/13-errors-openstack-lightspeed-instance.yamltest/kuttl/tests/update-openstacklightspeed/14-cleanup-mock-objects.yamltest/kuttl/tests/update-openstacklightspeed/15-errors-mock-objects.yaml
omkarjoshi0304
force-pushed
the
lcore-dev
branch
3 times, most recently
from
30d2eb3 to
cdad45d
Compare
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
Nice one! 👍 Overall looks good to me!:)
omkarjoshi0304
force-pushed
the
lcore-dev
branch
4 times, most recently
from
b060ba8 to
75741be
Compare
umago
left a comment
umago
left a comment
There was a problem hiding this comment.
Code looks good. The only exception is the kubectl issue Lukas pointed out.
Deploy a dataverse exporter sidecar container that collect user feedback and transcripts from a shared EmptyDir volume and uploads them to Red Hat Dataverse. The exporter is conditionally deployed only when data collection is enabled (FeedbackDisabled/TranscriptsDisabled both default to false). Key changes: - Add exporter container, shared data volume, and config volume to pod spec - Add exporter ConfigMap reconciliation (create when enabled, delete when disabled) - Add RBAC for pull-secret and clusterversions access (exporter authentication) - Add KUTTL test assertions for exporter ConfigMap and updated ClusterRole
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
Overall LGTM:) 👍 but there is one thing we need to figure out before merging. It looks like the test transcripts and feedback were not sent to https://console.redhat.com/api/ingress/v1/upload during testing. Let's figure out what is happening there. I'm going to take a look.
|
/lgtm The code looks OK, but we are currently losing the data somewhere in the pipeline. After discussing this with the team, I agree it might be a good idea to merge it, as it seems to be working (the data gets sent to the endpoint and the configuration seems to be correct). As a follow-up, we just need to figure out what is happening with the data after it gets sent out to console.redhat.com. |
|
/approved |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lpiwowar, omkarjoshi0304, umago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit d98fee9
into
openstack-lightspeed:lcore-migration
3 participants
Deploy a dataverse exporter sidecar container that collect user feedback and transcripts from a shared EmptyDir volumeand uploads them to Red Hat Dataverse. The exporter is conditionally deployed only when data collection is enabled (FeedbackDisabled/TranscriptsDisabled both default to false).
Key changes:
Summary by CodeRabbit