feat: support the system notifications and improve the README.md #7357
+300
−224
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
testcara
force-pushed
the
konflux-system-alerts
branch
from
1823ae5 to
27f4ba4
Compare
testcara
force-pushed
the
konflux-system-alerts
branch
from
27f4ba4 to
d04b641
Compare
testcara
force-pushed
the
konflux-system-alerts
branch
from
d04b641 to
ed261ce
Compare
Code Review by Gemini### components/konflux-info/README.md
**Improvements:**
* **Line 103**: The numbering for "Weekly Recurring Banner" is incorrect.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -100,7 +100,7 @@
- summary: "Maintenance every Sunday"
type: "info"
dayOfWeek: 0
startTime: "02:00"
endTime: "04:00"
# No timezone is needed when you expect it's UTC.
```
-#### ✅ **2. Weekly Recurring Banner**
+#### 2.3.3. <a name='3.WeeklyRecurringBanner'></a>✅ **3. Weekly Recurring Banner**
For an event that repeats every week:
```
* **Line 111**: The numbering for "Monthly Recurring Banner" is incorrect.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -108,7 +108,7 @@
endTime: "04:00"
```
-#### ✅ **3. Monthly Recurring Banner**
+#### 2.3.4. <a name='4.MonthlyRecurringBanner'></a>✅ **4. Monthly Recurring Banner**
For an event that happens on the same day each month:
```
* **Line 119**: The numbering for "Always-On Banner" is incorrect.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -116,7 +116,7 @@
timeZone: "Asia/Shanghai"
```
-#### ✅ **4. Always-On Banner**
+#### 2.3.5. <a name='5.Always-OnBanner'></a>✅ **5. Always-On Banner**
For an event that requires immediate notification:
```
* **Line 124**: The numbering for "Empty Banner" is incorrect.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -121,7 +121,7 @@
- summary: "New feature: Pipeline Insights is live!"
type: "info"
```
-#### ✅ **5. Empty Banner**
+#### 2.3.6. <a name='6.EmptyBanner'></a>✅ **6. Empty Banner**
When there are no events to announce:
```
* **Line 132**: The heading for "How to submit a PR for Banner" is missing its section number and anchor.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -129,7 +129,7 @@
```
---
-## 📝 How to submit a PR for Banner
+### 2.4. <a name='HowtosubmitaPRforBanner'></a>📝 How to submit a PR for Banner
1. Locate the target cluster directory:
```
* **Line 154**: The heading for "Frequently Asked Questions" is missing its section number and anchor.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -151,7 +151,7 @@
Type: New banner
Purpose: Release announcement for Konflux 1.2
```
-## ❓ Frequently Asked Questions
+### 2.6. <a name='FrequentlyAskedQuestions'></a>❓ Frequently Asked Questions
- Why is only one banner shown even when multiple are configured?
```
* **Line 190**: The description for the JSON payload fields could be clearer.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -187,7 +187,7 @@
Each ConfigMap contains notification data in the `notification-content.json` field as JSON.
<strong>Key points:</strong>
-- The JSON payload supports these fields for each notification object:
+- The JSON payload for each notification object supports these fields:
- <strong>title (optional)</strong>: A short heading. <strong>Defaults to `component.metadata.name` if omitted.</strong>
- <strong>summary (required)</strong>: A brief, user-facing message displayed as the notification content.
```
* **Note**: The `title` field description "Defaults to `component.metadata.name` if omitted" implies `component.metadata.name` is part of the JSON payload, but it's likely a UI-side default based on the ConfigMap's metadata. Consider clarifying this if it's a UI behavior.
### components/konflux-info/base/rbac.yaml
**Bugs:**
* **Lines 10-12**: The removal of `resourceNames` makes the role overly permissive, allowing it to `get` any ConfigMap in the `konflux-info` namespace. This is a security regression. It should be restricted to specific ConfigMaps (e.g., `konflux-public-info`, `konflux-banner-configmap`, and ConfigMaps with the `konflux.system.notification: "true"` label).
```diff
--- a/components/konflux-info/base/rbac.yaml
+++ b/components/konflux-info/base/rbac.yaml
@@ -7,9 +7,6 @@
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: konflux-public-info-view-role
rules:
- verbs:
- get
apiGroups:
- ''
- resourceNames:
- - konflux-public-info
- - konflux-banner-configmap
resources:
- configmaps
---
```
**Suggested Change:** Re-add `resourceNames` or use `resourceNames` with `selector` if supported, or define a `resourceName` for the new system notifications. For example, to allow access to specific ConfigMaps:
```yaml
---
kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: konflux-public-info-view-role
rules:
- verbs:
- get
apiGroups:
- ''
resourceNames:
- konflux-public-info
- konflux-banner-configmap
# Add resource names for system notifications if they are fixed,
# or consider a label selector if the API supports it for roles.
# If not, a separate role for system notifications might be needed
# or rely on the UI to filter by label after getting all.
resources:
- configmaps
---
```
Alternatively, if the intent is to allow getting *all* configmaps and filter by label in the UI, this should be explicitly documented and accepted as a design choice, but it's generally less secure.
### components/konflux-info/staging/stone-stage-p01/kustomization.yaml
**Bugs:**
* **Line 6**: The directory name `system-alerts` is inconsistent with the `README.md` and `staging/stone-stg-rh01` which use `system-notifications`.
```diff
--- a/components/konflux-info/staging/stone-stage-p01/kustomization.yaml
+++ b/components/konflux-info/staging/stone-stage-p01/kustomization.yaml
@@ -3,7 +3,7 @@
resources:
- ../../base
- - auto-alerts
+ - system-alerts
generatorOptions:
- disableNameSuffixHash: true
+ disableNameSuffixHash: true
```
**Suggested Change:**
```diff
--- a/components/konflux-info/staging/stone-stage-p01/kustomization.yaml
+++ b/components/konflux-info/staging/stone-stage-p01/kustomization.yaml
@@ -3,7 +3,7 @@
resources:
- ../../base
- - system-alerts
+ - system-notifications
generatorOptions:
disableNameSuffixHash: true
```
### components/konflux-info/staging/stone-stage-p01/system-alerts/kustomization.yaml
**Bugs:**
* **Line 4**: The resource name `system-alert-1.yaml` is inconsistent with the `README.md` and `staging/stone-stg-rh01` which use `system-notification-1.yaml`.
```diff
--- a/components/konflux-info/production/kflux-prd-rh02/auto-alerts/kustomization.yaml
+++ b/components/konflux-info/staging/stone-stage-p01/system-alerts/kustomization.yaml
@@ -1,4 +1,4 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- - auto-alert-1.yaml
+ - system-alert-1.yaml
```
**Suggested Change:**
```diff
--- a/components/konflux-info/staging/stone-stage-p01/system-alerts/kustomization.yaml
+++ b/components/konflux-info/staging/stone-stage-p01/system-alerts/kustomization.yaml
@@ -1,4 +1,4 @@
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
- - system-alert-1.yaml
+ - system-notification-1.yaml
```
### components/konflux-info/staging/stone-stage-p01/system-alerts/system-alert-1.yaml
**Bugs:**
* **Line 4**: The data key `alert-content.json` is inconsistent with the `README.md` and `staging/stone-stg-rh01` which use `notification-content.json`.
* **Line 7**: The `type` value `Info` uses a capital 'I', but the `README.md` specifies `info` (lowercase). This might cause issues if the UI expects lowercase.
* **Line 10**: The label `konflux-auto-alert: "true"` is the old label. The `README.md` specifies `konflux.system.notification: "true"`.
* **Line 11**: The name `konflux-autoalert-configmap-1` is inconsistent with the new naming convention.
```diff
--- a/components/konflux-info/production/kflux-prd-rh03/auto-alerts/auto-alert-1.yaml
+++ b/components/konflux-info/staging/stone-stage-p01/system-alerts/system-alert-1.yaml
@@ -1,12 +1,13 @@
apiVersion: v1
data:
- auto-alert-content.yaml: |-
- enable: false
- type: warning
- summary: pipeline run cannot run
+ alert-content.json: |
+ {
+ "summary": "There is an example system alert!",
+ "type": "Info"
+ }
kind: ConfigMap
metadata:
labels:
konflux-auto-alert: "true"
name: konflux-autoalert-configmap-1
namespace: konflux-info
```
**Suggested Change:**
```diff
--- a/components/konflux-info/staging/stone-stage-p01/system-alerts/system-alert-1.yaml
+++ b/components/konflux-info/staging/stone-stage-p01/system-alerts/system-alert-1.yaml
@@ -1,13 +1,14 @@
apiVersion: v1
data:
- alert-content.json: |
+ notification-content.json: |
+ {
+ "summary": "There is an example system alert!",
- "type": "Info"
+ "type": "info"
+ }
kind: ConfigMap
metadata:
labels:
- konflux-auto-alert: "true"
+ konflux.system.notification: "true"
- name: konflux-autoalert-configmap-1
+ name: konflux-system-notification-configmap-1
namespace: konflux-info
```
### components/konflux-info/staging/stone-stg-rh01/system-notifications/system-notification-1.yaml
**Bugs:**
* **Line 7**: The `type` value `Info` uses a capital 'I', but the `README.md` specifies `info` (lowercase). This might cause issues if the UI expects lowercase.
* **Line 11**: The name `konflux-autoalert-configmap-1` is inconsistent with the new naming convention.
```diff
--- /dev/null
+++ b/components/konflux-info/staging/stone-stg-rh01/system-notifications/system-notification-1.yaml
@@ -0,0 +1,13 @@
+apiVersion: v1
+data:
+ notification-content.json: |
+ {
+ "summary": "There is an example system notification!",
+ "type": "Info"
+ }
+kind: ConfigMap
+metadata:
+ labels:
+ konflux-system-notification: "true"
+ name: konflux-autoalert-configmap-1
+ namespace: konflux-info
```
**Suggested Change:**
```diff
--- a/components/konflux-info/staging/stone-stg-rh01/system-notifications/system-notification-1.yaml
+++ b/components/konflux-info/staging/stone-stg-rh01/system-notifications/system-notification-1.yaml
@@ -4,9 +4,9 @@
data:
notification-content.json: |
{
"summary": "There is an example system notification!",
- "type": "Info"
+ "type": "info"
}
kind: ConfigMap
metadata:
labels:
konflux-system-notification: "true"
- name: konflux-autoalert-configmap-1
+ name: konflux-system-notification-configmap-1
namespace: konflux-info
```
### components/mintmaker/production/kflux-ocp-p01/kustomization.yaml
### components/mintmaker/production/kflux-osp-p01/kustomization.yaml
### components/mintmaker/production/kflux-prd-rh02/kustomization.yaml
### components/mintmaker/production/kflux-prd-rh03/kustomization.yaml
### components/mintmaker/production/kflux-rhel-p01/kustomization.yaml
### components/mintmaker/production/stone-prod-p01/kustomization.yaml
### components/multi-platform-controller/base/kustomization.yaml
**Note:** These files contain image tag updates. Assuming these are routine updates and not directly related to the `konflux-info` changes, no specific review comments are provided for them. |
testcara
force-pushed
the
konflux-system-alerts
branch
from
ed261ce to
b44df3a
Compare
Code Review by Gemini## Code Review
### `components/konflux-info/README.md`
* **Suggestion**: Consider removing the `<!-- vscode-markdown-toc -->` comments. While useful for VS Code users, they are tool-specific annotations that clutter the raw Markdown file and are not standard Markdown.
* **Line 1-10, 40-43**
* **Issue**: The README states that the `title` field for system notifications is optional and "Defaults to `component.metadata.name` if omitted." However, `component.metadata.name` refers to the Kubernetes ConfigMap's metadata, not a field within the `notification-content.json` payload itself. The example `notification-content.json` does not include a `title` field. This could be confusing.
* **Suggestion**: Clarify how `component.metadata.name` is used as a default title. If the UI is expected to extract the ConfigMap's `metadata.name` as a fallback, explicitly state that. If `title` is meant to be an optional field *within* the JSON, the example should reflect that.
* **Line 260**: `title (optional): A short heading. Defaults to component.metadata.name if omitted.`
* **Issue**: The `README.md` explicitly notes "no CI validation for the `notification-content.json` payload" (Line 290). This is a regression compared to the previous `auto-alert-schema.json` which provided validation for the `summary` field (including `minLength`, `maxLength`, and `pattern`). Without validation, malformed JSON or potentially unsafe content (e.g., XSS if the UI doesn't sanitize) could be introduced.
* **Suggestion**: Reintroduce schema validation for the `notification-content.json` payload. This could be a new JSON schema file and a corresponding CI check, similar to how `banner-content.yaml` is validated. This is crucial for data integrity and security.
* **Line 290**: `⚠️ Note: There is currently no CI validation for the notification-content.json payload, so please ensure proper formatting.`
### `components/konflux-info/auto-alert-schema.json`
* **Issue**: This file was deleted, which removes the schema validation for the old `auto-alerts`. As noted above, no new schema validation was introduced for the `system-notifications` JSON payload.
* **Suggestion**: Create a new JSON schema for `notification-content.json` and implement CI validation for it. This schema should at least include validation for `summary` (e.g., `minLength`, `maxLength`, and a `pattern` to restrict characters) and `type` (enum values).
### `components/konflux-info/base/rbac.yaml`
* **Issue**: The `konflux-public-info-view-role` has been broadened to allow `get` access to *all* `configmaps` in the `konflux-info` namespace, by removing specific `resourceNames`. This is a significant permission escalation. If other sensitive ConfigMaps are ever placed in this namespace, this role would grant access to them.
* **Suggestion**: Revert the change to `resourceNames` and explicitly list all ConfigMaps that this role should have access to. For example, if `konflux-public-info`, `konflux-banner-configmap`, and the new `konflux-system-notification-configmap-*` are the only intended ConfigMaps, list them explicitly. Alternatively, if Kubernetes version supports it, use a `labelSelector` to only allow access to ConfigMaps with specific labels (e.g., `konflux.system.notification: "true"` or `konflux-banner: "true"`).
* **Line 9-11**:
```diff
--- a/components/konflux-info/base/rbac.yaml
+++ b/components/konflux-info/base/rbac.yaml
@@ -7,9 +7,6 @@
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: konflux-public-info-view-role
rules:
- verbs:
- get
apiGroups:
- ''
- resourceNames:
- - konflux-public-info
- - konflux-banner-configmap
resources:
- configmaps
```
### `components/mintmaker/production/base/manager_patch.yaml`
* **Issue**: The `GOMEMLIMIT` environment variable has been removed. This variable helps Go applications manage their memory usage within a container's memory limits. Removing it might lead to the Go process attempting to use more memory than allocated, potentially resulting in Out-Of-Memory (OOM) kills, especially if the application's memory consumption is close to the container's `memory` limit (which is 8Gi).
* **Suggestion**: Re-evaluate the necessity of `GOMEMLIMIT`. If it was previously set to prevent OOMs, its removal could introduce instability. If the application's memory profile has changed significantly or other memory management strategies are in place, this change might be acceptable, but it should be explicitly justified.
* **Line 13-15**:
```diff
--- a/components/mintmaker/production/base/manager_patch.yaml
+++ b/components/mintmaker/production/base/manager_patch.yaml
@@ -10,6 +10,3 @@
requests:
cpu: 100m
memory: 8Gi
- env:
- - name: GOMEMLIMIT
- value: "7GiB"
``` |
testcara
force-pushed
the
konflux-system-alerts
branch
from
b44df3a to
209fea9
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: testcara 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 |
Code Review by Gemini## Code Review
Here's a review of the provided code changes, focusing on potential bugs and areas for improvement.
### 1. `.github/workflows/validate-notification.yaml`
* **Bug/Improvement**: The paths to `system-notifications` are relative to the repository root, but the `BANNER_DIR` environment variable is defined. It's better to use the environment variable for consistency and clarity.
```diff
--- a/.github/workflows/validate-notification.yaml
+++ b/.github/workflows/validate-notification.yaml
@@ -17,7 +17,7 @@
- name: Check konflux-system-notification label present
run: |
- for file in system-notifications/*.yaml; do
+ for file in "${BANNER_DIR}/system-notifications/*.yaml"; do
if [[ "$(basename "$file")" == "kustomization.yaml" ]]; then
continue
fi
@@ -33,7 +33,7 @@
node-version: "18"
- name: Install ajv-cli for JSON Schema validation
- run: npm install -g ajv-cli
+ run: |
+ npm install -g ajv-cli
- name: Validate notification-content.json files
run: |
- for file in system-notifications/*.yaml; do
+ for file in "${BANNER_DIR}/system-notifications/*.yaml"; do
file_name=$(basename "$file") # no spaces around =
if [[ "$file_name" == "kustomization.yaml" ]]; then
continue
```
* **Bug**: The `TMP_DIR` (`.tmp/notifications`) is used to store temporary JSON files, but the directory is not created before `yq` attempts to write to it. This will cause the workflow to fail if the directory doesn't exist.
```diff
--- a/.github/workflows/validate-notification.yaml
+++ b/.github/workflows/validate-notification.yaml
@@ -38,6 +38,7 @@
- name: Validate notification-content.json files
run: |
+ mkdir -p "${TMP_DIR}"
for file in system-notifications/*.yaml; do
file_name=$(basename "$file") # no spaces around =
if [[ "$file_name" == "kustomization.yaml" ]]; then
```
### 2. `components/konflux-info/README.md`
* **Bug**: The `auto-alert-schema.json` file is being deleted, so its reference in the directory structure should also be removed.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -17,7 +17,6 @@
The `KONFLUX-INFO` directory contains:
```bash
.
-├── auto-alert-schema.json # JSON shema definition for auto-alert-content.yaml
├── base/ # Common resources (e.g., RBAC)
├── production/ # Production cluster configurations
├── staging/ # Staging cluster configurations
```
* **Bug**: The `auto-alerts` directory is being replaced by `system-notifications`. The `README.md` should reflect this change in the directory structure and kustomization.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -26,9 +25,9 @@
```bash
.
-├── auto-alerts # The directory manages auto-generated alerts content shown in the UI
+├── system-notifications # The directory manages auto-generated notifications content shown in the UI
├── banner-content.yaml # The banner content shown in the UI
├── info.json # Metadata about the cluster
-└── kustomization.yaml # Kustomize configuration for this cluster, including base, auto-alerts, and other configs
+└── kustomization.yaml # Kustomize configuration for this cluster, including base, system-notifications, and other configs
```
```
* **Bug**: The example ConfigMap for system notifications still uses the old `auto-alert` format and label. It should be updated to match the new `notification-content.json` format and the `konflux-system-notification` label used in the new `system-notification-1.yaml` and `validate-notification.yaml` workflow.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -195,14 +195,14 @@
kind: ConfigMap
metadata:
name: konflux-auto-alert-xyz
namespace: konflux-info
labels:
- konflux-auto-alert: "true" # Required. UI filter alerts out by this label.
+ konflux-system-notification: "true"
data:
- auto-alert-content.yaml: |
- enable: true
- summary: "Builds are delayed due to maintenance"
- type: "warning"
+ notification-content.json: |-
+ {
+ "summary": "Builds are delayed due to maintenance",
+ "type": "warning",
+ "title": "From Builds Service"
+ }
```
* **Bug**: The folder structure example still refers to `auto-alerts`.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -238,11 +238,11 @@
### 3.4. <a name='FolderStructure'></a>✅ **Folder Structure**
Notifications are organized under the `system-notifications/` directory:
```bash
-auto-alerts/ # Alert ConfigMaps (one file = one alert)
+system-notifications/ # Notification ConfigMaps (one file = one notification)
.
-├── alert-1.yaml # Fully valid ConfigMap YAML
-├── alert-2.yaml
-└── kustomization.yaml # Auto-generated, includes all alert YAMLs
+├── notification-1.yaml # A ConfigMap representing one notification
+├── notification-2.yaml
+└── kustomization.yaml # Auto-generated, includes all notifications YAMLs
```
```
### 3. `components/konflux-info/base/rbac.yaml`
* **Bug/Security Concern**: The change removes specific `resourceNames` for `configmaps`, effectively granting `get` access to *all* ConfigMaps in the `konflux-info` namespace to the `konflux-public-info-view-role`. This is a potential security regression. It's best practice to grant the least privilege necessary. If the new system notification ConfigMaps are intended to be publicly viewable, they should be explicitly added to the `resourceNames` list.
```diff
--- a/components/konflux-info/base/rbac.yaml
+++ b/components/konflux-info/base/rbac.yaml
@@ -6,9 +6,9 @@
rules:
- verbs:
- get
apiGroups:
- ''
- resourceNames:
- - konflux-public-info
- - konflux-banner-configmap
resources:
- configmaps
```
**Suggested Change**: Revert the removal of `resourceNames` and add the new system notification ConfigMap names if they are meant to be public. For example:
```yaml
---
kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: konflux-public-info-view-role
rules:
- verbs:
- get
apiGroups:
- ''
resourceNames:
- konflux-public-info
- konflux-banner-configmap
- konflux-system-notification-configmap-1 # Add specific names for system notifications
# Add other system notification configmap names as needed
resources:
- configmaps
```
Alternatively, if there are many system notification ConfigMaps and they all follow a naming convention, consider using a label selector in the UI to filter them, and keep the RBAC broad if the namespace is exclusively for public info. However, explicit `resourceNames` is generally safer. Given the `konflux-info` namespace is specifically for this purpose, granting `get` on all `configmaps` might be acceptable if all configmaps in this namespace are indeed public. Please confirm this design decision.
### 4. `components/konflux-info/staging/stone-stage-p01/system-notifications/system-notification-1.yaml`
* **Improvement**: The `konflux-system-notification` label is used here, which is consistent with the workflow. However, the `README.md` example uses `konflux.system.notification`. Ensure consistency across all files. The current label in this file is good, so the `README.md` should be updated to match it.
---
Overall, the changes are well-structured and aim to improve the system. The main points to address are consistency in naming/labels across documentation and code, and the potential security implication in the RBAC definition. |
testcara
force-pushed
the
konflux-system-alerts
branch
from
209fea9 to
72f70ea
Compare
testcara
force-pushed
the
konflux-system-alerts
branch
from
72f70ea to
9630e54
Compare
Code Review by Gemini## Code Review
This pull request introduces a new GitHub Actions workflow for validating system notifications, updates the `README.md` to reflect the new "System Notifications" concept (replacing "Auto Alerts"), and refactors related Kubernetes configurations.
### Issues and Suggestions
1. **Bug: `rbac.yaml` - Permission Escalation**
* **File:** `components/konflux-info/base/rbac.yaml`
* **Lines:** 10-13
* **Issue:** The `konflux-public-info-view-role` previously had `get` access to specific `ConfigMap` resources (`konflux-public-info`, `konflux-banner-configmap`). With this change, the `resourceNames` field is removed, granting the role `get` access to *all* `ConfigMaps` within the `konflux-info` namespace. This is a significant permission escalation and could expose sensitive `ConfigMaps` if they exist in that namespace.
* **Suggestion:** Revert the change to `rbac.yaml` and ensure the role only has access to the specific `ConfigMaps` it needs, including the new system notification ConfigMaps if they are intended to be publicly viewable. If the new system notification ConfigMaps are also meant to be public, their names should be explicitly added to the `resourceNames` list.
```diff
--- a/components/konflux-info/base/rbac.yaml
+++ b/components/konflux-info/base/rbac.yaml
@@ -7,10 +7,8 @@
rules:
- verbs:
- get
apiGroups:
- ''
- resourceNames:
- - konflux-public-info
- - konflux-banner-configmap
resources:
- configmaps
---
```
* **Proposed Change (Example, assuming new notification ConfigMaps should also be public):**
```yaml
---
kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: konflux-public-info-view-role
rules:
- verbs:
- get
apiGroups:
- ''
resourceNames:
- konflux-public-info
- konflux-banner-configmap
- konflux-system-notification-configmap-1 # Add specific names for system notifications if needed
resources:
- configmaps
---
```
*Note: If system notification ConfigMaps have dynamic names, a different approach might be needed, but granting access to all ConfigMaps is generally not recommended.*
2. **Bug: `README.md` - JSON Example Syntax Error**
* **File:** `components/konflux-info/README.md`
* **Lines:** 109-110
* **Issue:** The example JSON for `notification-content.json` is missing a comma between the `type` and `title` fields. This will result in invalid JSON.
* **Suggestion:** Add a comma after the `type` field.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -106,7 +106,7 @@
{
"summary": "Builds are delayed due to maintenance",
"type": "warning"
- "title": "From Builds Service"
+ ,"title": "From Builds Service"
}
```
3. **Potential Issue: Deployment Inconsistency for System Notifications**
* **Files:**
* `components/konflux-info/production/*/kustomization.yaml`
* `components/konflux-info/staging/*/kustomization.yaml`
* **Issue:** The `auto-alerts` directory references have been removed from *all* production cluster `kustomization.yaml` files, but the new `system-notifications` directory references have only been added to *staging* cluster `kustomization.yaml` files. This means that after this change, production clusters will no longer deploy any system notifications.
* **Suggestion:** Confirm if this is the intended behavior.
* If production clusters are *not* meant to have system notifications, then this is fine, but it's a significant change in functionality that might warrant a note in the PR description or a separate discussion.
* If production clusters *should* have system notifications, then the `system-notifications` directory and its reference in `kustomization.yaml` should also be added to the production cluster configurations, similar to how it's done for staging.
### Minor Improvements
1. **`validate-notification.yaml` - Redundant Comment**
* **File:** `.github/workflows/validate-notification.yaml`
* **Line:** 49
* **Suggestion:** The comment `# no spaces around =` is redundant as `file_name=$(basename "$file")` is standard shell syntax and doesn't require this specific note. It can be removed for cleaner code.
```diff
--- a/.github/workflows/validate-notification.yaml
+++ b/.github/workflows/validate-notification.yaml
@@ -46,7 +46,7 @@
mkdir -p ${{ env.TMP_DIR }}
for file in ${{ env.NOTIFICATION_DIR }}/**/**/system-notifications/*.yaml; do
file_name=$(basename "$file") # no spaces around =
if [[ "$file_name" == "kustomization.yaml" ]]; then
continue
fi
```
* **Proposed Change:**
```yaml
- name: Validate notification-content.json files
run: |
mkdir -p ${{ env.TMP_DIR }}
for file in ${{ env.NOTIFICATION_DIR }}/**/**/system-notifications/*.yaml; do
file_name=$(basename "$file")
if [[ "$file_name" == "kustomization.yaml" ]]; then
continue
fi
# Extract JSON content from ConfigMap
yq e '.data."notification-content.json"' "$file" > "${TMP_DIR}/${file_name}.json"
ajv validate -s "${SCHEMA_FILE}" -d "${TMP_DIR}/${file_name}.json" --errors=text
if [ $? -ne 0 ]; then
echo "ERROR: notification-content.json in ${file} does not validate against schema"
exit 1
fi
done
```
|
testcara
changed the title
testcara
force-pushed
the
konflux-system-alerts
branch
from
9630e54 to
b3be250
Compare
testcara
force-pushed
the
konflux-system-alerts
branch
from
b3be250 to
88d81d1
Compare
Code Review by GeminiHere's a review of the provided code changes:
|
testcara
force-pushed
the
konflux-system-alerts
branch
from
88d81d1 to
4c02063
Compare
Code Review by Gemini## Code Review
### 1. `.github/workflows/validate-notification.yaml`
* **Improvement**: The glob pattern `**/**/system-notifications/*.yaml` can be simplified.
```diff
--- a/.github/workflows/validate-notification.yaml
+++ b/.github/workflows/validate-notification.yaml
@@ -7,7 +7,7 @@
jobs:
validate:
runs-on: ubuntu-latest
-
steps:
- uses: actions/checkout@v3
-
- name: Install yq (YAML processor)
run: |
sudo wget https://github.com/mikefarah/yq/releases/download/v4.34.1/yq_linux_amd64 -O /usr/bin/yq
sudo chmod +x /usr/bin/yq
-
- name: Check konflux-system-notification label present
run: |
mkdir -p ${{ env.TMP_DIR }}
- for file in ${{ env.NOTIFICATION_DIR }}/**/**/system-notifications/*.yaml; do
+ for file in ${{ env.NOTIFICATION_DIR }}/**/system-notifications/*.yaml; do
if [[ "$(basename "$file")" == "kustomization.yaml" ]]; then
continue
fi
label=$(yq e '.metadata.labels."konflux.system.notification"' "$file")
if [[ "$label" != "\"true\"" && "$label" != "true" ]]; then
@@ -24,7 +24,7 @@
- name: Validate notification-content.json files
run: |
mkdir -p ${{ env.TMP_DIR }}
- for file in ${{ env.NOTIFICATION_DIR }}/**/**/system-notifications/*.yaml; do
+ for file in ${{ env.NOTIFICATION_DIR }}/**/system-notifications/*.yaml; do
file_name=$(basename "$file")
if [[ "$file_name" == "kustomization.yaml" ]]; then
continue
```
### 2. `components/konflux-info/README.md`
* **Bug**: The numbering for "Usage Scenarios & Examples" is incorrect, with duplicate "2." and subsequent sections misnumbered.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -143,28 +143,28 @@
- # No timezone is needed when you expect it's UTC.
```
-#### ✅ **2. One-Time Banner**
+#### 2.3.2. <a name='2.One-TimeBanner'></a>✅ **2. One-Time Banner**
For a single event on a specific date:
```yaml
- summary: "Scheduled downtime on July 25"
type: "warning"
@@ -172,28 +172,28 @@
```
For a single event in today
```yaml
- summary: "Scheduled downtime on July 25"
type: "warning"
@@ -179,28 +179,28 @@
endTime: "14:00"
```
-#### ✅ **2. Weekly Recurring Banner**
+#### 2.3.3. <a name='3.WeeklyRecurringBanner'></a>✅ **3. Weekly Recurring Banner**
For an event that repeats every week:
```yaml
- summary: "Maintenance every Sunday"
type: "info"
@@ -189,28 +189,28 @@
endTime: "04:00"
```
-#### ✅ **3. Monthly Recurring Banner**
+#### 2.3.4. <a name='4.MonthlyRecurringBanner'></a>✅ **4. Monthly Recurring Banner**
For an event that happens on the same day each month:
```yaml
- summary: "Patch release on 1st of every month"
type: "info"
@@ -209,28 +209,28 @@
endTime: "03:00"
timeZone: "Asia/Shanghai"
```
-#### ✅ **4. Always-On Banner**
+#### 2.3.5. <a name='5.Always-OnBanner'></a>✅ **5. Always-On Banner**
For an event that requires immediate notification:
```yaml
- summary: "New feature: Pipeline Insights is live!"
type: "info"
@@ -238,28 +238,28 @@
```
-#### ✅ **5. Empty Banner**
+#### 2.3.6. <a name='6.EmptyBanner'></a>✅ **6. Empty Banner**
When there are no events to announce:
```
[]
```
```
### 3. `components/konflux-info/base/rbac.yaml`
* **Note**: The change removes specific `resourceNames` for `configmaps`, allowing the `konflux-public-info-view-role` to `get` any `configmap` within the `konflux-info` namespace. While this might be intended for public information, ensure no sensitive `configmaps` are present or could be added to this namespace that should not be publicly viewable.
```diff
--- a/components/konflux-info/base/rbac.yaml
+++ b/components/konflux-info/base/rbac.yaml
@@ -4,9 +4,6 @@
rules:
- verbs:
- get
apiGroups:
- ''
- resourceNames:
- - konflux-public-info
- - konflux-banner-configmap
resources:
- configmaps
```
### 4. `components/monitoring/prometheus/development/monitoringstack/cluster-type-patch.yaml`
* **Note**: The operation for the patch on `relabelings/0` changed from `replace` to `add`. This means that instead of replacing the first element in the `relabelings` list, it will now insert a new element at the beginning, shifting existing elements. This is a change in behavior. Confirm this is the intended effect (i.e., to insert this specific relabeling at the start of the list).
```diff
--- a/components/monitoring/prometheus/development/monitoringstack/cluster-type-patch.yaml
+++ b/components/monitoring/prometheus/development/monitoringstack/cluster-type-patch.yaml
@@ -1,6 +1,6 @@
---
-- op: replace
+- op: add
path: /spec/endpoints/0/relabelings/0
value:
targetLabel: source_environment
``` |
testcara
force-pushed
the
konflux-system-alerts
branch
from
4c02063 to
ade86c7
Compare
Code Review by Gemini## Code Review
### `components/konflux-info/README.md`
* **Improvement:** In section `2.3.3. ✅ 3. Weekly Recurring Banner`, the example title `✅ 2. Weekly Recurring Banner` is incorrect. It should be `✅ 3. Weekly Recurring Banner`.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -144,7 +144,7 @@ For a single event in today
- endTime: "14:00"
```
-#### ✅ **2. Weekly Recurring Banner**
+#### 2.3.3. <a name='3.WeeklyRecurringBanner'></a>✅ **3. Weekly Recurring Banner**
For an event that repeats every week:
```
* **Improvement:** In section `2.3.4. ✅ 4. Monthly Recurring Banner`, the example title `✅ 3. Monthly Recurring Banner` is incorrect. It should be `✅ 4. Monthly Recurring Banner`.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -153,7 +153,7 @@ For an event that repeats every week:
- endTime: "04:00"
```
-#### ✅ **3. Monthly Recurring Banner**
+#### 2.3.4. <a name='4.MonthlyRecurringBanner'></a>✅ **4. Monthly Recurring Banner**
For an event that happens on the same day each month:
```
* **Improvement:** In section `2.3.5. ✅ 5. Always-On Banner`, the example title `✅ 4. Always-On Banner` is incorrect. It should be `✅ 5. Always-On Banner`.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -162,7 +162,7 @@ For an event that happens on the same day each month:
- timeZone: "Asia/Shanghai"
```
-#### ✅ **4. Always-On Banner**
+#### 2.3.5. <a name='5.Always-OnBanner'></a>✅ **5. Always-On Banner**
For an event that requires immediate notification:
```
* **Improvement:** In section `2.3.6. ✅ 6. Empty Banner`, the example title `✅ 5. Empty Banner` is incorrect. It should be `✅ 6. Empty Banner`.
```diff
--- a/components/konflux-info/README.md
+++ b/components/konflux-info/README.md
@@ -168,7 +168,7 @@ For an event that requires immediate notification:
- summary: "New feature: Pipeline Insights is live!"
type: "info"
```
-#### ✅ **5. Empty Banner**
+#### 2.3.6. <a name='6.EmptyBanner'></a>✅ **6. Empty Banner**
When there are no events to announce:
```
### `.github/workflows/validate-notification.yaml`
* **Improvement:** The glob pattern `**/**/system-notifications/*.yaml` can be simplified to `**/system-notifications/*.yaml`.
```diff
--- a/.github/workflows/validate-notification.yaml
+++ b/.github/workflows/validate-notification.yaml
@@ -19,7 +19,7 @@ jobs:
- name: Check konflux-system-notification label present
run: |
mkdir -p ${{ env.TMP_DIR }}
- for file in ${{ env.NOTIFICATION_DIR }}/**/**/system-notifications/*.yaml; do
+ for file in ${{ env.NOTIFICATION_DIR }}/**/system-notifications/*.yaml; do
if [[ "$(basename "$file")" == "kustomization.yaml" ]]; then
continue
fi
@@ -37,7 +37,7 @@ jobs:
- name: Validate notification-content.json files
run: |
mkdir -p ${{ env.TMP_DIR }}
- for file in ${{ env.NOTIFICATION_DIR }}/**/**/system-notifications/*.yaml; do
+ for file in ${{ env.NOTIFICATION_DIR }}/**/system-notifications/*.yaml; do
file_name=$(basename "$file")
if [[ "$file_name" == "kustomization.yaml" ]]; then
continue
```
### `components/konflux-info/base/rbac.yaml`
* **Note:** The change removes specific `resourceNames` (`konflux-public-info`, `konflux-banner-configmap`) and broadens the `konflux-public-info-view-role` to allow `get` access on *all* `configmaps` within the `konflux-info` namespace. This is likely necessary for the new system notification feature to dynamically discover ConfigMaps by label, but it's a permission increase that should be acknowledged.
```diff
--- a/components/konflux-info/base/rbac.yaml
+++ b/components/konflux-info/base/rbac.yaml
@@ -5,9 +5,6 @@
rules:
- verbs:
- get
apiGroups:
- ''
- resourceNames:
- - konflux-public-info
- - konflux-banner-configmap
resources:
- configmaps
---
```
### `components/konflux-info/notification-schema.json`
* **Bug:** The `type` field in the `notification` definition is missing the `enum` constraint, which is specified in the `README.md` as `info`, `warning`, or `danger`.
```diff
--- /dev/null
+++ b/components/konflux-info/notification-schema.json
@@ -16,7 +16,8 @@
},
"type": {
"type": "string",
"description": "Required. The type of notification: 'info', 'warning', or 'danger'."
+ "enum": ["info", "warning", "danger"]
},
"activeTimestamp": {
```
### `components/monitoring/prometheus/development/monitoringstack/cluster-type-patch.yaml`
* **Bug Fix:** Changing `op: replace` to `op: add` for the `relabelings` array is a good fix. If the `relabelings` array was empty or didn't exist, `replace` would fail. `add` ensures the element is added correctly.
```diff
--- a/components/monitoring/prometheus/development/monitoringstack/cluster-type-patch.yaml
+++ b/components/monitoring/prometheus/development/monitoringstack/cluster-type-patch.yaml
@@ -1,6 +1,6 @@
---
-- op: replace
+- op: add
path: /spec/endpoints/0/relabelings/0
value:
targetLabel: source_environment
replacement: development-cluster
```
### `components/monitoring/prometheus/base/monitoringstack/monitoringstack.yaml`
* **Note:** The `writeRelabelConfigs` section has been moved from an overlay to the base `MonitoringStack` definition, and a very extensive list of `match[]` parameters has been added to the `ServiceMonitor`. This is a significant change to the Prometheus metric collection and remote write configuration. While not a bug in the code itself, it requires careful review by a monitoring expert to ensure all necessary metrics are still collected and no unintended metrics are dropped.
```diff
--- a/components/monitoring/prometheus/base/monitoringstack/monitoringstack.yaml
+++ b/components/monitoring/prometheus/base/monitoringstack/monitoringstack.yaml
@@ -35,21 +35,37 @@ spec:
secret:
key: client-id
name: rhobs
clientSecret:
key: client-secret
name: rhobs
endpointParams:
audience: # added by overlays
tokenUrl: https://sso.redhat.com/auth/realms/redhat-external/protocol/openid-connect/token
url: # added by overlays
- # writeRelabelConfigs: # added by overlays
+ writeRelabelConfigs:
+ - action: LabelKeep
+ regex: "__name__|source_environment|source_cluster|namespace|app|pod|container|\
+ label_pipelines_appstudio_openshift_io_type|health_status|dest_namespace|\
+ controller|service|reason|phase|type|resource|resourcequota|le|app|image|\
+ commit_hash|job|operation|tokenName|rateLimited|state|persistentvolumeclaim|\
+ storageclass|volumename|release_reason|instance|result|deployment_reason|\
+ validation_reason|strategy|succeeded|target|name|method|code|sp|le|\
+ unexpected_status|failure|hostname|label_app_kubernetes_io_managed_by|status|\
+ pipeline|pipelinename|pipelinerun|schedule|check|grpc_service|grpc_code|\
+ grpc_method|lease|lease_holder|deployment|platform|mode|cpu|role|node|kind|\
+ verb|request_kind|tested_cluster|resource_type|exported_job|http_method|\
+ http_route|http_status_code|gin_errors|rule_result|rule_execution_cause|\
+ policy_name|policy_background_mode|rule_type|policy_type|policy_validation_mode|\
+ resource_request_operation|resource_kind|policy_change_type|event_type"
+
---
# Grant permission to Federate In-Cluster Prometheus
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: appstudio-federate-ms-view
labels:
kubernetes.io/part-of: appstudio-federate-ms
monitoring.rhobs/stack: appstudio-federate-ms
@@ -71,7 +87,191 @@ metadata:
spec:
selector: # use the prometheus service to create a "dummy" target.
matchLabels:
app.kubernetes.io/managed-by: observability-operator
app.kubernetes.io/name: appstudio-federate-ms-prometheus
endpoints:
- - params: {} # added by overlays
+ - params:
+ 'match[]': # scrape only required metrics from in-cluster prometheus
+ - '{__name__="pipeline_service_schedule_overhead_percentage_sum"}'
+ - '{__name__="pipeline_service_schedule_overhead_percentage_count"}'
+ - '{__name__="pipeline_service_execution_overhead_percentage_sum"}'
+ - '{__name__="pipeline_service_execution_overhead_percentage_count"}'
+ - '{__name__="pipelinerun_duration_scheduled_seconds_sum"}'
+ - '{__name__="pipelinerun_duration_scheduled_seconds_count"}'
+ - '{__name__="pipelinerun_gap_between_taskruns_milliseconds_sum"}'
+ - '{__name__="pipelinerun_gap_between_taskruns_milliseconds_count"}'
+ - '{__name__="pipelinerun_kickoff_not_attempted_count"}'
+ - '{__name__="pending_resolutionrequest_count"}'
+ - '{__name__="taskrun_pod_create_not_attempted_or_pending_count"}'
+ - '{__name__="tekton_pipelines_controller_pipelinerun_count"}'
+ - '{__name__="tekton_pipelines_controller_running_pipelineruns_count"}'
+ - '{__name__="tekton_pipelines_controller_running_taskruns_throttled_by_quota_count"}'
+ - '{__name__="tekton_pipelines_controller_running_taskruns_throttled_by_node_count"}'
+ - '{__name__="tekton_pipelines_controller_running_taskruns_throttled_by_quota"}'
+ - '{__name__="tekton_pipelines_controller_running_taskruns_throttled_by_node"}'
+ - '{__name__="tekton_pipelines_controller_pipelinerun_duration_seconds_sum"}'
+ - '{__name__="tekton_pipelines_controller_pipelinerun_duration_seconds_count"}'
+ - '{__name__="watcher_workqueue_depth"}'
+ - '{__name__="watcher_client_latency_bucket"}'
+ - '{__name__="pac_watcher_work_queue_depth"}'
+ - '{__name__="pac_watcher_client_latency_bucket"}'
+ - '{__name__="grpc_server_handled_total", namespace=~"tekton-results|openshift-pipelines"}'
+ - '{__name__="grpc_server_handled_total", namespace=~"openshift-etcd"}'
+ - '{__name__="grpc_server_handling_seconds_bucket", namespace=~"tekton-results|openshift-pipelines"}'
+ - '{__name__="grpc_server_handling_seconds_bucket", namespace="openshift-etcd"}'
+ - '{__name__="grpc_server_msg_received_total", namespace="openshift-etcd"}'
+ - '{__name__="controller_runtime_reconcile_errors_total", namespace!~".*-tenant|openshift-.*|kube-.*"}'
+ - '{__name__="controller_runtime_reconcile_total", namespace!~".*-tenant|openshift-.*|kube-.*"}'
+ - '{__name__="kube_lease_owner", namespace="openshift-pipelines", lease=~"controller.tektonresolverframework.bundleresolver..*"}'
+ - '{__name__="kube_lease_owner", namespace="openshift-pipelines", lease=~"tekton-pipelines-controller.github.com.tektoncd.pipeline.pkg.reconciler..*"}'
+ - '{__name__="kube_pod_status_unschedulable", namespace!~".*-tenant|openshift-.*|kube-.*"}'
+ - '{__name__="kube_pod_container_status_restarts_total", namespace=~"openshift-pipelines|release-service"}'
+ - '{__name__="kube_pod_container_status_waiting_reason", namespace!~".*-tenant|openshift-.*|kube-.*"}'
+ - '{__name__="kube_pod_status_phase", namespace!~".*-tenant|openshift-.*|kube-.*"}'
+ - '{__name__="kube_pod_container_resource_limits", namespace="release-service"}'
+ - '{__name__="kube_pod_container_status_terminated_reason", namespace="release-service"}'
+ - '{__name__="kube_pod_container_status_last_terminated_reason", namespace="release-service"}'
+ - '{__name__="kube_pod_container_status_ready", namespace="release-service"}'
+ - '{__name__="kube_persistentvolume_status_phase", namespace!~".*-tenant|openshift-.*|kube-.*"}'
+ - '{__name__="kube_resourcequota", namespace!~".*-tenant|openshift-.*|kube-.*"}'
+ - '{__name__="kube_statefulset_status_replicas_ready", namespace="gitops-service-argocd"}'
+ - '{__name__="kube_statefulset_replicas", namespace="gitops-service-argocd"}'
+ - '{__name__="openshift_route_status", namespace="gitops-service-argocd"}'
+
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="gitops-service-argocd"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace=~"gitops-service-argocd"}'
+
+ # Namespace (expression): "build-service"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="build-service"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="build-service"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="build-service"}'
+
+ # Namespace (expression): "integration-service"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="integration-service"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="integration-service"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="integration-service"}'
+
+ # Namespace (expression): "konflux-ui"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="konflux-ui"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="konflux-ui"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="konflux-ui"}'
+ - '{__name__="kube_running_pods_ready", namespace="konflux-ui"}'
+ - '{__name__="kube_endpoint_address", namespace="konflux-ui"}'
+ - '{__name__="kube_pod_container_status_restarts_total", namespace="konflux-ui"}'
+
+ # Namespace (expression): "mintmaker"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="mintmaker"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="mintmaker"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="mintmaker"}'
+ - '{__name__="cluster_ram_requested_perc"}'
+ - '{__name__="node_memory_pressured_perc"}'
+ - '{__name__="redis_node_memory_usage_perc"}'
+
+ # Namespace (expression): ~".*monitoring.*"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace=~".*monitoring.*"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace=~".*monitoring.*"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace=~".*monitoring.*"}'
+
+ # Namespace (expression): "multi-platform-controller"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="multi-platform-controller"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="multi-platform-controller"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="multi-platform-controller"}'
+
+ # Namespace (expression): "namespace-lister"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="namespace-lister"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="namespace-lister"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="namespace-lister"}'
+
+ # Namespace (expression): "openshift-pipelines"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="openshift-pipelines"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="openshift-pipelines"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="openshift-pipelines"}'
+
+ # Namespace (expression): "product-kubearchive"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="product-kubearchive"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="product-kubearchive"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="product-kubearchive"}'
+
+ # Namespace (expression): "project-controller"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="project-controller"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="project-controller"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="project-controller"}'
+
+ # Namespace (expression): "release-service"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="release-service"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="release-service"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="release-service"}'
+
+ # Namespace (expression): ~"smee.*"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace=~"smee.*"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace=~"smee.*"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace=~"smee.*"}'
+
+ # Namespace (expression): "openshift-apiserver"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="openshift-apiserver"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="openshift-apiserver"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="openshift-apiserver"}'
+
+ # Namespace (expression): "openshift-oauth-apiserver"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="openshift-oauth-apiserver"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="openshift-oauth-apiserver"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="openshift-oauth-apiserver"}'
+
+ # Namespace (expression): "konflux-kyverno"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="konflux-kyverno"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="konflux-kyverno"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="konflux-kyverno"}'
+
+ # Namespace (expression): "openshift-kube-apiserver"
+ - '{__name__="kube_deployment_status_replicas_ready", namespace="openshift-kube-apiserver"}'
+ - '{__name__="kube_deployment_status_replicas_available", namespace="openshift-kube-apiserver"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="openshift-kube-apiserver"}'
+
+ # Namespace (expression): "konflux-user-support"
+ - '{__name__="kube_deployment_status_replicas_available", namespace="konflux-user-support"}'
+ - '{__name__="kube_deployment_spec_replicas", namespace="konflux-user-support"}'
+
+ - '{__name__="argocd_app_reconcile_bucket", namespace="gitops-service-argocd"}'
+ - '{__name__="argocd_app_info", namespace="gitops-service-argocd"}'
+ - '{__name__="container_cpu_usage_seconds_total", namespace="release-service"}'
+ - '{__name__="container_cpu_usage_seconds_total", namespace="openshift-etcd"}'
+ - '{__name__="container_memory_usage_bytes", namespace="release-service"}'
+ - '{__name__="container_memory_usage_bytes", namespace="openshift-etcd"}'
+ - '{__name__="etcd_disk_wal_fsync_duration_seconds_bucket"}'
+ - '{__name__="etcd_disk_backend_commit_duration_seconds_bucket"}'
+ - '{__name__="etcd_server_proposals_failed_total"}'
+ - '{__name__="etcd_server_leader_changes_seen_total", namespace="openshift-etcd"}'
+ - '{__name__="etcd_server_has_leader", namespace="openshift-etcd"}'
+ - '{__name__="etcd_server_is_leader", namespace="openshift-etcd"}'
+ - '{__name__="etcd_server_id", namespace="openshift-etcd"}'
+ - '{__name__="etcd_server_quota_backend_bytes", namespace="openshift-etcd"}'
+ - '{__name__="etcd_mvcc_db_total_size_in_bytes", namespace="openshift-etcd"}'
+ - '{__name__="etcd_server_received_total", namespace="openshift-etcd"}'
+ - '{__name__="etcd_network_active_peers", namespace="openshift-etcd"}'
+ - '{__name__="etcd_network_peer_round_trip_time_seconds_bucket"}'
+ - '{__name__="etcd_disk_defrag_inflight"}'
+ - '{__name__="kube_job_spec_completions"}'
+ - '{__name__="kube_job_status_succeeded"}'
+ - '{__name__="kube_job_status_failed"}'
+ - '{__name__="node_cpu_seconds_total", mode="idle"}'
+ - '{__name__="node_memory_MemTotal_bytes"}'
+ - '{__name__="node_memory_MemAvailable_bytes"}'
+ - '{__name__="platform:hypershift_hostedclusters:max"}'
+ - '{__name__="kube_node_role"}'
+ - '{__name__="etcd_shield_trigger"}'
+ - '{__name__="etcd_shield_alert_triggered"}'
+ - '{__name__="apiserver_admission_webhook_rejection_count", name="vpipelineruns.konflux-ci.dev"}'
+ - '{__name__="apiserver_watch_events_total"}'
+ - '{__name__="apiserver_storage_objects"}'
+ - '{__name__="apiserver_current_inflight_requests"}'
+ - '{__name__="resource_verb:apiserver_request_total:rate5m"}'
+ - '{__name__="code:apiserver_request_total:rate5m"}'
+ - '{__name__="instance:apiserver_request_total:rate5m"}'
+ - '{__name__="prometheus_ready"}'
+ - '{__name__="process_cpu_seconds_total", job="apiserver"}'
+ - '{__name__="namespace:container_memory_usage_bytes:sum", namespace=~"openshift-etcd|openshift-kube-apiserver|build-service|image-controller|integration-service|konflux-ui|product-kubearchive|openshift-kueue-operator|tekton-kueue|kueue-external-admission|mintmaker|multi-platform-controller|namespace-lister|openshift-pipelines|tekton-results|project-controller|smee|smee-client"}'
+ - '{__name__="namespace:container_cpu_usage:sum", namespace=~"openshift-etcd|openshift-kube-apiserver|build-service|image-controller|integration-service|konflux-ui|product-kubearchive|openshift-kueue-operator|tekton-kueue|kueue-external-admission|mintmaker|multi-platform-controller|namespace-lister|openshift-pipelines|tekton-results|project-controller|smee|smee-client"}'
+ - '{__name__="node_namespace_pod:kube_pod_info:", namespace=~"openshift-etcd|openshift-kube-apiserver|build-service|image-controller|integration-service|konflux-ui|product-kubearchive|openshift-kueue-operator|tekton-kueue|kueue-external-admission|mintmaker|multi-platform-controller|namespace-lister|openshift-pipelines|tekton-results|project-controller|smee|smee-client"}'
+ - '{__name__="kube_node_status_allocatable", resource=~"cpu|memory"}'
+ - '{__name__="kube_node_status_condition", condition="MemoryPressure", status="true"}'
+ - '{__name__="namespace_memory:kube_pod_container_resource_requests:sum", namespace=~"openshift-etcd|openshift-kube-apiserver|build-service|image-controller|integration-service|konflux-ui|product-kubearchive|openshift-kueue-operator|tekton-kueue|kueue-external-admission|mintmaker|multi-platform-controller|namespace-lister|openshift-pipelines|tekton-results|project-controller|smee|smee-client"}'
+ - '{__name__="namespace_cpu:kube_pod_container_resource_requests:sum", namespace=~"openshift-etcd|openshift-kube-apiserver|build-service|image-controller|integration-service|konflux-ui|product-kubearchive|openshift-kueue-operator|tekton-kueue|kueue-external-admission|mintmaker|multi-platform-controller|namespace-lister|openshift-pipelines|tekton-results|project-controller|smee|smee-client"}'
+ - '{__name__="namespace_memory:kube_pod_container_resource_limits:sum", namespace=~"openshift-etcd|openshift-kube-apiserver|build-service|image-controller|integration-service|konflux-ui|product-kubearchive|openshift-kueue-operator|tekton-kueue|kueue-external-admission|mintmaker|multi-platform-controller|namespace-lister|openshift-pipelines|tekton-results|project-controller|smee|smee-client"}'
+ - '{__name__="namespace_cpu:kube_pod_container_resource_limits:sum", namespace=~"openshift-etcd|openshift-kube-apiserver|build-service|image-controller|integration-service|konflux-ui|product-kubearchive|openshift-kueue-operator|tekton-kueue|kueue-external-admission|mintmaker|multi-platform-controller|namespace-lister|openshift-pipelines|tekton-results|project-controller|smee|smee-client"}'
relabelings:
# override the target's address by the prometheus-k8s service name.
- action: replace
targetLabel: __address__
replacement: prometheus-k8s.openshift-monitoring.svc:9091
```
### `components/tracing/otel-collector/staging/otel-collector-helm-values.yaml`
* **Note:** The `prometheus` exporter and the `metrics` pipeline within the `telemetry` section have been removed. This indicates a change in how metrics are exported or collected from the OpenTelemetry Collector. This is a functional change that should be verified by the tracing/monitoring team.
```diff
--- a/components/tracing/otel-collector/staging/otel-collector-helm-values.yaml
+++ b/components/tracing/otel-collector/staging/otel-collector-helm-values.yaml
@@ -3,9 +3,6 @@
sapm:
endpoint: "https://ingest.us1.signalfx.com/v2/trace"
access_token: ${env:SIGNALFX_API_TOKEN}
signalfx:
endpoint: "https://api.signalfx.com/v2/traces"
realm: "us1"
api_url: "https://api.us1.signalfx.com"
access_token: ${env:SIGNALFX_API_TOKEN}
-prometheus:
- endpoint: 0.0.0.0:8889
extensions:
# The health_check extension is mandatory for this chart.
# Without the health_check extension the collector will fail the readiness and liveliness probes.
# The health_check extension can be modified, but should never be removed.
@@ -56,18 +53,7 @@ config:
- debug
- sapm
- signalfx
processors:
- memory_limiter
- attributes/collector-info
- batch
- attributes/stage
receivers:
- otlp
- metrics:
- receivers:
- - otlp
- processors:
- - memory_limiter
- - batch
- exporters:
- - prometheus
- telemetry:
- metrics:
- level: basic
- readers:
- - pull:
- exporter: prometheus
- host: 0.0.0.0
- port: 8888
-
+ metrics: null
# Configuration for ports
ports:
otlp:
``` |
testcara
force-pushed
the
konflux-system-alerts
branch
from
ade86c7 to
87b260e
Compare
Code Review by GeminiHere's a review of the provided code changes, focusing on bugs and suggested improvements:
|
testcara
force-pushed
the
konflux-system-alerts
branch
from
87b260e to
4465a6e
Compare
Code Review by GeminiHere's a review of the provided code changes: Overall, the core changes related to supporting system notifications and updating the README are well-implemented. However, this pull request includes a significant number of unrelated changes that should ideally be separated into their own pull requests to maintain clarity, simplify reviews, and ensure proper change management. Unrelated Changes (High Priority)Several changes in this PR are unrelated to "system notifications" or "README improvement" and should be reverted and submitted in separate PRs. These changes affect different components and could have unintended side effects or require separate review by relevant teams.
Issues and Improvements (Related to System Notifications)
Good Changes
Recommendation: Please revert all the "Unrelated Changes" identified above. Once those are removed, the PR can be re-reviewed focusing solely on the system notification feature and the suggested improvements for the |
testcara
force-pushed
the
konflux-system-alerts
branch
from
4465a6e to
83f583c
Compare
Code Review by GeminiHere's a review of the provided code changes, focusing on identifying bugs and suggesting improvements.
|
testcara
force-pushed
the
konflux-system-alerts
branch
from
83f583c to
ffbfbd5
Compare
Code Review by GeminiHere's a review of the provided code changes: General Observations
Detailed Review1.
|
|
/retest |
1 similar comment
|
/retest |
|
@testcara: The following tests failed, say
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. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For https://issues.redhat.com/browse/KFLUXUI-592
The 'System Notification' chapter in README.md introduces:
With the configmaps deployed to related clusters, UI would show the notifications as:
More, it is just expanded when we click the badge in the header and UI sorts it for the newest to the oldest.
And we also provide the optional 'activeTimestamp' filed to ensure you can determine when the notification should be shown.
See the vids of UI for more details:
system-notification.mp4