Skip to content

Update more details-Observability service (in review) #7974

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

Merged
merged 7 commits into from
Aug 6, 2025

Conversation

PoornimaSingour
Copy link
Contributor

@PoornimaSingour PoornimaSingour commented Jun 18, 2025

Update more details related to StorageClass in Enabling the Observability service Section.

BUG : https://issues.redhat.com/browse/ACM-21655

Added:

Note: By default if you do not define the storageConfig.storageClass field in the MultiClusterObservability custom resource, platforms default StorageClass will be populated in the storageConfig section of MultiClusterObservability. For example for AWS default storageClass will be gp2. You can verify default storageClass using below command:

$ oc get storageClass
NAME                PROVISIONER       RECLAIMPOLICY   VOLUMEBINDINGMODE      ALLOWVOLUMEEXPANSION   AGE
gp2-csi             ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   151m
gp3-csi (default)   ebs.csi.aws.com   Delete          WaitForFirstConsumer   true                   151m

Also $ was missing in front of all the command so added that as well.

…lity service Section

Update more details related to StorageClass in Enabling the Observability service  Section
@PoornimaSingour
Copy link
Contributor Author

PoornimaSingour commented Jun 18, 2025

/label peer-review-in-needed

Copy link

openshift-ci bot commented Jun 18, 2025

@PoornimaSingour: The label(s) /label peer-review-in-progress cannot be applied. These labels are supported: acknowledge-critical-fixes-only, platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, ux-approved, no-qe, downstream-change-needed, rebase/manual, cluster-config-api-changed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, stability-fix-approved, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label peer-review-in-progress

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.

@PoornimaSingour
Copy link
Contributor Author

/label peer-review-in-needed

Copy link

openshift-ci bot commented Jun 18, 2025

@PoornimaSingour: The label(s) /label peer-review-in-needed cannot be applied. These labels are supported: acknowledge-critical-fixes-only, platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, ux-approved, no-qe, downstream-change-needed, rebase/manual, cluster-config-api-changed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, stability-fix-approved, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/label peer-review-in-needed

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.

@swopebe
Copy link
Contributor

swopebe commented Jun 18, 2025

@PoornimaSingour thanks for the PR, however, can you point us to the JIRA issue for the doc team that reflects this change?
We cannot start peer review or tech review without tacking the change/understanding the change via a doc issue.

If no issue was opened, you can open one from this procedure:

https://docs.google.com/document/d/1YTqpZRH54Bnn4WJ2nZmjaCoiRtqmrc2w6DdQxe_yLZ8/edit?tab=t.0#heading=h.nmome2yn9lvr

@swopebe swopebe changed the title Update more details related to StorageClass in Enabling the Observability service Section Update more details-Observability service (not ready for review, issue requested-bcs) Jun 18, 2025
@PoornimaSingour
Copy link
Contributor Author

HI @swopebe Yes here is the JIRA https://issues.redhat.com/browse/ACM-21655. DO let me know if any details are needed.

@swopebe swopebe changed the title Update more details-Observability service (not ready for review, issue requested-bcs) Update more details-Observability service Jun 20, 2025
@swopebe
Copy link
Contributor

swopebe commented Jun 20, 2025

Thanks, we will pick this up as soon as we can. We generally don't add $ to the commands in ACM doc, we never really have (this content was moved from IBM and we stuck to those practices.) I know OCP adds it.

@swopebe swopebe changed the title Update more details-Observability service Update more details-Observability service (post GA or when in progress features are complete) Jun 23, 2025
@oafischer oafischer requested a review from dockerymick June 26, 2025 14:51
@@ -50,7 +50,7 @@ Complete the following steps to enable the Observability service:
+
[source,bash]
----
oc create namespace open-cluster-management-observability
$ oc create namespace open-cluster-management-observability
Copy link
Contributor

Choose a reason for hiding this comment

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

remove $

Copy link
Contributor

@swopebe swopebe Jul 25, 2025

Choose a reason for hiding this comment

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

keep consistent throughout. This topic should have "required access:" at the top, to fit with the ACM format. -- just checked and it does.

Copy link
Contributor

Choose a reason for hiding this comment

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

This file also has ROKS in it as a product name, which is against company standards so that needs to change, as well.

+
All the pods in `open-cluster-management-observability` namespace for Thanos, Grafana and Alertmanager are created. All the managed clusters connected to the {acm-short} hub cluster are enabled to send metrics back to the {acm-short} Observability service.
All the pods in `open-cluster-management-observability` namespace for Thanos, Grafana and Alertmanager are created. All the managed clusters connected to the {acm-short} hub cluster are enabled to send metrics back to the {acm-short} Observability service.
Copy link
Contributor

@swopebe swopebe Jul 25, 2025

Choose a reason for hiding this comment

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

Suggested change
All the pods in `open-cluster-management-observability` namespace for Thanos, Grafana and Alertmanager are created. All the managed clusters connected to the {acm-short} hub cluster are enabled to send metrics back to the {acm-short} Observability service.
All the pods in `open-cluster-management-observability` namespace for Thanos, Grafana, and Alertmanager are created. All the managed clusters connected to the {acm-short} hub cluster are enabled to send metrics back to the {acm-short} 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.

line 505 really should not be here--we don't know that they "are created"; for a while some writers were writing this in place of "verification steps" (this is not a verification step) without checking in for peer review--we have stopped doing that. I think this should be removed. You don't know that the pods are created.

@swopebe swopebe changed the title Update more details-Observability service (post GA or when in progress features are complete) Update more details-Observability service (in review) Jul 25, 2025
Copy link
Contributor

@dockerymick dockerymick left a comment

Choose a reason for hiding this comment

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

Made updates based on earlier doc review

@openshift-ci openshift-ci bot added the lgtm label Jul 31, 2025
@coleenquadros
Copy link
Contributor

/lgtm

Copy link

openshift-ci bot commented Aug 1, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: coleenquadros, dockerymick, PoornimaSingour

The full list of commands accepted by this bot can be found 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 removed the lgtm label Aug 1, 2025
Copy link

openshift-ci bot commented Aug 1, 2025

New changes are detected. LGTM label has been removed.

@@ -489,9 +489,20 @@ For more information, see link:https://docs.redhat.com/en/documentation/openshif
----
oc apply -f multiclusterobservability_cr.yaml
----

+
*Note:* By default, if you do not define the `storageConfig.storageClass` field in the `MultiClusterObservability` custom resource, platform default `StorageClass` fields are populated in the `storageConfig` section of the `MultiClusterObservability` resource. For example, AWS default `storageClass` is set to `gp2`. You can verify default `storageClass` by running the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a step inside this note.

@dockerymick
Copy link
Contributor

Created tech debt issue to improve the doc: https://issues.redhat.com/browse/ACM-22916

Otherwise, the request from the original issue is addressed. Merging this PR

@dockerymick dockerymick merged commit 0e43796 into stolostron:2.14_stage Aug 6, 2025
1 of 2 checks passed
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.

4 participants