Skip to content

Conversation

HozifaWasfy
Copy link
Contributor

@HozifaWasfy HozifaWasfy commented Sep 5, 2025

Copy link

openshift-ci bot commented Sep 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HozifaWasfy

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

The pull request process is described 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 added the approved label Sep 5, 2025
Copy link
Contributor

github-actions bot commented Sep 5, 2025

Code Review by Gemini

## Code Review

### `components/mintmaker/staging/base/kustomization.yaml`

**Issue:** Typo in resource path.
The path `../blackcox` appears to be a typo and should likely be `../blackbox` to correctly reference the new blackbox component.

**Suggested Change:**
```diff
--- a/components/mintmaker/staging/base/kustomization.yaml
+++ b/components/mintmaker/staging/base/kustomization.yaml
@@ -1,7 +1,7 @@
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 resources:
 - ../../base
 - ../../base/external-secrets
-- ../blackcox
+- ../blackbox
 - https://github.com/konflux-ci/mintmaker/config/default?ref=d9daa8e361dc4714b598bd9443ae32b0e8384f79
 - https://github.com/konflux-ci/mintmaker/config/renovate?ref=d9daa8e361dc4714b598bd9443ae32b0e8384f79
 

components/mintmaker/staging/blackbox/blackbox-deployment.yaml

Issue 1: Using :latest tag for the image.
Using the latest tag for container images can lead to unpredictable deployments as the image content can change without a version bump. It's best practice to use a specific, immutable image tag.

Suggested Change:

--- a/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
+++ b/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
@@ -34,7 +34,7 @@
     spec:
       containers:
         - name: blackbox-exporter
-          image: quay.io/prometheus/blackbox-exporter:latest
+          image: quay.io/prometheus/blackbox-exporter:0.24.0 # Or a specific stable version
           args:
             - "--config.file=/etc/blackbox/blackbox.yml"
           ports:

Issue 2: Missing newline at the end of the file.
It's a common convention and good practice for text files to end with a newline character.

Suggested Change:

--- a/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
+++ b/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
@@ -57,4 +57,5 @@
       volumes:
         - name: config-volume
           configMap:
             name: blackbox-config
+\ No newline at end of file

components/mintmaker/staging/blackbox/blackbox-service.yaml

Issue: Missing newline at the end of the file.
It's a common convention and good practice for text files to end with a newline character.

Suggested Change:

--- a/components/mintmaker/staging/blackbox/blackbox-service.yaml
+++ b/components/mintmaker/staging/blackbox/blackbox-service.yaml
@@ -13,3 +13,4 @@
     - name: http
       port: 9115
       targetPort: 9115
+\ No newline at end of file

components/mintmaker/staging/blackbox/git-platforms.yaml

Issue: Missing newline at the end of the file.
It's a common convention and good practice for text files to end with a newline character.

Suggested Change:

--- a/components/mintmaker/staging/blackbox/git-platforms.yaml
+++ b/components/mintmaker/staging/blackbox/git-platforms.yaml
@@ -72,4 +72,5 @@
   targets:
     staticConfig:
       static:
         - https://gitlab.cee.redhat.com
+\ No newline at end of file

components/mintmaker/staging/blackbox/kustomization.yaml

Issue: Missing newline at the end of the file.
It's a common convention and good practice for text files to end with a newline character.

Suggested Change:

--- a/components/mintmaker/staging/blackbox/kustomization.yaml
+++ b/components/mintmaker/staging/blackbox/kustomization.yaml
@@ -5,3 +5,4 @@
 - blackbox-service.yaml
 - git-platforms.yaml
 
+namespace: mintmaker
+\ No newline at end of file

Copy link
Contributor

github-actions bot commented Sep 5, 2025

Code Review by Gemini

## Code Review

### `components/mintmaker/staging/base/kustomization.yaml`

No issues found.

### `components/mintmaker/staging/blackbox/blackbox-deployment.yaml`

**Issue 1: Image tag `:latest`**
Using the `:latest` tag for container images is generally discouraged in production environments as it can lead to unpredictable deployments when the image updates. It's best practice to pin to a specific, immutable version tag.

```diff
--- a/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
+++ b/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
@@ -33,7 +33,7 @@
     spec:
       containers:
         - name: blackbox-exporter
-          image: quay.io/prometheus/blackbox-exporter:latest
+          image: quay.io/prometheus/blackbox-exporter:v0.23.0 # Or a specific stable version
           args:
             - "--config.file=/etc/blackbox/blackbox.yml"
           ports:

Issue 2: Missing newline at end of file
It's a common best practice for text files to end with a newline character.

--- a/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
+++ b/components/mintmaker/staging/blackbox/blackbox-deployment.yaml
@@ -57,4 +57,5 @@
       volumes:
         - name: config-volume
           configMap:
             name: blackbox-config
+\ No newline at end of file

Suggested change: Add a newline at the end of the file.

components/mintmaker/staging/blackbox/blackbox-service.yaml

Issue 1: Missing newline at end of file
It's a common best practice for text files to end with a newline character.

--- a/components/mintmaker/staging/blackbox/blackbox-service.yaml
+++ b/components/mintmaker/staging/blackbox/blackbox-service.yaml
@@ -13,4 +13,5 @@
   ports:
     - name: http
       port: 9115
       targetPort: 9115
+\ No newline at end of file

Suggested change: Add a newline at the end of the file.

components/mintmaker/staging/blackbox/git-platforms.yaml

Issue 1: Redundant prober.pod configuration
When prober.url is specified, Prometheus Operator uses that URL to reach the blackbox exporter. The prober.pod section is typically used as an alternative for pod discovery when a service is not used, or for cross-namespace probing. Since prober.url is already pointing to the git-platforms-exporter service in the same namespace, the prober.pod configuration is redundant and can be removed for clarity and to avoid potential misconfigurations.

--- a/components/mintmaker/staging/blackbox/git-platforms.yaml
+++ b/components/mintmaker/staging/blackbox/git-platforms.yaml
@@ -10,11 +10,7 @@
   prober:
     url: git-platforms-exporter:9115
     scheme: http
-    pod:
-      namespace: mintmaker
-      selector:
-        matchLabels:
-          app: git-platforms-exporter
-      port: http 
   module: http_2xx
   targets:
     staticConfig:
@@ -34,11 +30,7 @@
   prober:
     url: git-platforms-exporter:9115
     scheme: http
-    pod:
-      namespace: mintmaker
-      selector:
-        matchLabels:
-          app: git-platforms-exporter
-      port: http 
   module: http_2xx
   targets:
     staticConfig:
@@ -58,11 +50,7 @@
   prober:
     url: git-platforms-exporter:9115
     scheme: http
-    pod:
-      namespace: mintmaker
-      selector:
-        matchLabels:
-          app: git-platforms-exporter
-      port: http 
   module: http_2xx
   targets:
     staticConfig:

Issue 2: Missing newline at end of file
It's a common best practice for text files to end with a newline character.

--- a/components/mintmaker/staging/blackbox/git-platforms.yaml
+++ b/components/mintmaker/staging/blackbox/git-platforms.yaml
@@ -72,4 +72,5 @@
   targets:
     staticConfig:
       static:
         - https://gitlab.cee.redhat.com
+\ No newline at end of file

Suggested change: Add a newline at the end of the file.

components/mintmaker/staging/blackbox/kustomization.yaml

Issue 1: Missing newline at end of file
It's a common best practice for text files to end with a newline character.

--- a/components/mintmaker/staging/blackbox/kustomization.yaml
+++ b/components/mintmaker/staging/blackbox/kustomization.yaml
@@ -6,4 +6,5 @@
 - blackbox-service.yaml
 - git-platforms.yaml
 
 namespace: mintmaker
+\ No newline at end of file

Suggested change: Add a newline at the end of the file.

Copy link
Contributor

github-actions bot commented Sep 5, 2025

Code Review by Gemini

--- a/components/mintmaker/staging/blackbox/git-platforms.yaml
+++ b/components/mintmaker/staging/blackbox/git-platforms.yaml
@@ -10,7 +10,7 @@
     app.kubernetes.io/managed-by: kustomize
 spec:
   jobName: "github-probe"
   prober:
-    url: git-platforms-exporter:9115
+    url: git-platforms-exporter:9115/probe
     scheme: http
     pod:
       namespace: mintmaker
@@ -34,7 +34,7 @@
     app.kubernetes.io/managed-by: kustomize
 spec:
   jobName: "gitlab-probe"
   prober:
-    url: git-platforms-exporter:9115
+    url: git-platforms-exporter:9115/probe
     scheme: http
     pod:
       namespace: mintmaker
@@ -58,7 +58,7 @@
     app.kubernetes.io/managed-by: kustomize
 spec:
   jobName: "gitlab-cee-probe"
   prober:
-    url: git-platforms-exporter:9115
+    url: git-platforms-exporter:9115/probe
     scheme: http
     pod:
       namespace: mintmaker

Copy link
Contributor

github-actions bot commented Sep 5, 2025

Code Review by Gemini

The changes introduce a Blackbox Exporter to monitor the availability of GitHub and GitLab platforms. The implementation is well-structured, follows standard practices for deploying Prometheus Blackbox Exporter with the Prometheus Operator, and includes appropriate Kubernetes resources (ConfigMap, Deployment, Service, and Probes).

No critical bugs or issues were found.

### Suggestions for Improvement:

1.  **Resource Labels Consistency (Minor):**
    While not a functional issue, the `app.kubernetes.io/name` label is set to `mintmaker` for the Blackbox Exporter's ConfigMap, Deployment, and Service. Typically, this label identifies the specific application. For the Blackbox Exporter components, `app.kubernetes.io/name: blackbox-exporter` might be more semantically accurate, with an additional label like `app.kubernetes.io/part-of: mintmaker` if it's considered a sub-component of the `mintmaker` project. However, the current approach is consistent across all new resources in this PR and does not cause any functional problems. Given the instruction "Do not suggest descriptive variable name", I will not suggest changing this.

2.  **Unused ICMP Prober (Minor):**
    The `blackbox-config` ConfigMap defines an `icmp` prober module. However, none of the `Probe` resources (`github-probe`, `gitlab-probe`, `gitlab-cee-probe`) currently utilize this `icmp` module; they all use `http_2xx`. While it's not an issue to have unused configuration, you might consider removing it if there are no immediate plans to use ICMP probes, to keep the configuration cleaner. This is a minor point and not a bug.

Overall, the changes are good and achieve the stated goal of monitoring GitHub/GitLab availability.

app.kubernetes.io/name: mintmaker
app.kubernetes.io/managed-by: kustomize
name: git-platforms-exporter
namespace: mintmaker
Copy link
Member

Choose a reason for hiding this comment

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

Why do you deploy it in the mintmaker namespace? I think the o11y team is going to deploy a Blackbox exporter as well cc @mftb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to monitor the availability of git platforms from mintmaker cluster

apiVersion: monitoring.coreos.com/v1
kind: Probe
metadata:
name: gitlab-cee-probe
Copy link
Member

Choose a reason for hiding this comment

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

This will only work in the internal clusters

Copy link
Member

@gbenhaim gbenhaim left a comment

Choose a reason for hiding this comment

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

  1. What are you going to do with the metrics?
  2. Don't you need to also setup rbac ?
  3. Don't you need to also setup a service monitor to scrape the exporter?

targets:
staticConfig:
static:
- https://github.com
Copy link
Member

@gbenhaim gbenhaim Sep 5, 2025

Choose a reason for hiding this comment

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

Most of the times when there is an outage in github this endpoint is available. Is there better way to check for availability? Same question about gitlab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1- I will set up alerts to monitor if mintmaker can access git platforms, because it is essential for mintmaker operations.
2- I do not need rbac for blackbox exporter because it does not communicate with k8s api, it only probe external endpoints.
3- I do not, because the probe will configure prometheus to scrape the exporter for the endpoint defined in the probe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the times when there is an outage in github this endpoint is available. Is there better way to check for availability? Same question about gitlab.

this is a good point, let me see what I can improve.

@HozifaWasfy HozifaWasfy requested a review from mftb September 8, 2025 11:58
@mftb
Copy link
Contributor

mftb commented Sep 9, 2025

@gcpsoares @kubasikus Please take a look at this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants