Skip to content

KubeArchive loki: remove second tmp folder #7550

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

Conversation

rh-hemartin
Copy link
Contributor

@rh-hemartin rh-hemartin commented Aug 8, 2025

Remove a tmp folder because there are two things trying to mount into /tmp. Added OWNER files as well.

@openshift-ci openshift-ci bot requested review from elsony and hugares August 8, 2025 12:35
Copy link
Contributor

github-actions bot commented Aug 8, 2025

Code Review by Gemini

The changes correctly remove the tmp-storage emptyDir volume and its corresponding volumeMount from the Loki Helm values. This aligns with the commit message's intent to resolve a conflict where multiple entities were attempting to mount to /tmp.

No bugs or issues are found in the provided diff. The removal is consistent and resolves the stated problem.

--- a/components/vector-kubearchive-log-collector/staging/stone-stg-rh01/loki-helm-values.yaml
+++ b/components/vector-kubearchive-log-collector/staging/stone-stg-rh01/loki-helm-values.yaml
@@ -28,11 +28,9 @@
   # Add writable volume mounts for Loki data directories
   extraVolumeMounts:
     - name: loki-storage
       mountPath: /var/loki
-    - name: tmp-storage
-      mountPath: /tmp
   extraVolumes:
     - name: loki-storage
       emptyDir: {}
-    - name: tmp-storage
-      emptyDir: {}
 
 # Basic Loki configuration with S3 storage
 loki:

@maruiz93
Copy link
Contributor

maruiz93 commented Aug 8, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 8, 2025
@rh-hemartin rh-hemartin force-pushed the kubearchive-logging-tmp-folder branch from 905b4dc to 9c5e24a Compare August 11, 2025 12:00
@openshift-ci openshift-ci bot removed the lgtm label Aug 11, 2025
Copy link
Contributor

Code Review by Gemini

The pull request includes several changes, some of which appear unrelated to the commit message "KubeArchive loki: remove second tmp folder".

### Issues and Suggestions:

1.  **Discrepancy between Commit Message and Changes:**
    The commit message states "remove second tmp folder", which directly relates to the changes in `components/vector-kubearchive-log-collector/staging/stone-stg-rh01/loki-helm-values.yaml`. However, the PR also includes:
    *   Removal of `GOMEMLIMIT` environment variable in `components/mintmaker/production/base/manager_patch.yaml`.
    *   Update of `multi-platform-controller` references in `components/multi-platform-controller/base/kustomization.yaml`.
    *   Deletion of `rbac.yaml` and its reference in `components/vector-kubearchive-log-collector/base/kustomization.yaml`.

    These additional changes are significant and are not explained by the current commit message. It is best practice for a commit to be atomic and for its message to accurately reflect all changes.

    **Suggestion:**
    *   Consider splitting this into multiple commits. For example:
        *   One commit for the Loki `/tmp` folder fix.
        *   One commit for the `rbac.yaml` deletion (with a clear explanation of why it's being removed).
        *   One commit for the `GOMEMLIMIT` change.
        *   One commit for the `multi-platform-controller` dependency update.
    *   Alternatively, if all changes are indeed related to a single logical fix, update the commit message and body to clearly explain the rationale behind *all* modifications.

2.  **Deletion of RBAC Definitions:**
    The deletion of `components/vector-kubearchive-log-collector/base/rbac.yaml` and its removal from `components/vector-kubearchive-log-collector/base/kustomization.yaml` means that the `RoleBinding`s for `kubearchive-logging-component-maintainers` and `kubearchive-logging-admin` will no longer be applied. This is a functional change regarding permissions.

    **Suggestion:**
    *   Confirm if these RBAC definitions are no longer needed or if they are being moved/redefined elsewhere.
    *   If they are intentionally removed, provide a clear explanation in the commit message/body (ideally in a separate commit) for why these permissions are no longer required for the `product-kubearchive-logging` namespace.

### Exact Lines for Changes:

No specific line changes are suggested for the existing diff, as the primary feedback is about commit structure and clarification of intent for the non-Loki-related changes. The changes made in the diff are syntactically correct for their intended purpose (deletion/update).

However, if the intent is to revert the unrelated changes and keep only the Loki `/tmp` fix, here are the lines that would need to be reverted:

**File: `components/mintmaker/production/base/manager_patch.yaml`**
To revert the removal of `GOMEMLIMIT`:
```diff
--- a/components/mintmaker/production/base/manager_patch.yaml
+++ b/components/mintmaker/production/base/manager_patch.yaml
@@ -13,3 +13,6 @@ spec:
           requests:
             cpu: 100m
             memory: 8Gi
+-        env:
+-        - name: GOMEMLIMIT
+-          value: "7GiB"

File: components/multi-platform-controller/base/kustomization.yaml
To revert the multi-platform-controller version update:

--- a/components/multi-platform-controller/base/kustomization.yaml
+++ b/components/multi-platform-controller/base/kustomization.yaml
@@ -8,13 +8,13 @@
 resources:
 - common
 - rbac
-- https://github.com/konflux-ci/multi-platform-controller/deploy/operator?ref=12cce4151fcc80327ffd03b9454aebaa2e068296
-- https://github.com/konflux-ci/multi-platform-controller/deploy/otp?ref=12cce4151fcc80327ffd03b9454aebaa2e068296
+- https://github.com/konflux-ci/multi-platform-controller/deploy/operator?ref=944fdc5bfb70a9e0ea84ea1232fc3ed2b8d924ba
+- https://github.com/konflux-ci/multi-platform-controller/deploy/otp?ref=944fdc5bfb70a9e0ea84ea1232fc3ed2b8d924ba
 
 
 images:
 - name: multi-platform-controller
   newName: quay.io/konflux-ci/multi-platform-controller
--  newTag: 12cce4151fcc80327ffd03b9454aebaa2e068296
+-  newTag: 944fdc5bfb70a9e0ea84ea1232fc3ed2b8d924ba
 - name: multi-platform-otp-server
   newName: quay.io/konflux-ci/multi-platform-controller-otp-service
--  newTag: 12cce4151fcc80327ffd03b9454aebaa2e068296
+-  newTag: 944fdc5bfb70a9e0ea84ea1232fc3ed2b8d924ba

File: components/vector-kubearchive-log-collector/base/kustomization.yaml
To revert the removal of rbac.yaml:

--- a/components/vector-kubearchive-log-collector/base/kustomization.yaml
+++ b/components/vector-kubearchive-log-collector/base/kustomization.yaml
@@ -6,4 +6,4 @@
   argocd.argoproj.io/sync-wave: "-1"
 
 resources:
-- vector-pre.yaml
+- vector-pre.yaml
+- rbac.yaml

And restore the components/vector-kubearchive-log-collector/base/rbac.yaml file.

@rh-hemartin rh-hemartin force-pushed the kubearchive-logging-tmp-folder branch from 9c5e24a to 473248f Compare August 11, 2025 12:02
Copy link
Contributor

Code Review by Gemini

The provided change removes an explicit emptyDir volume mount for /tmp from the Loki Helm values. This change directly addresses the problem described in the commit message, where "two things [were] trying to mount into /tmp", causing a conflict.

Assuming Loki does not require a dedicated or persistent /tmp volume beyond what the container runtime provides by default (which is common for most applications), this change is correct and resolves the stated conflict without introducing new issues.

No bugs or issues are found in the provided diff. The change is a direct and appropriate solution to the described problem.

@rh-hemartin rh-hemartin force-pushed the kubearchive-logging-tmp-folder branch from 473248f to c40e0ee Compare August 11, 2025 12:11
@rh-hemartin rh-hemartin force-pushed the kubearchive-logging-tmp-folder branch from c40e0ee to d619c0d Compare August 11, 2025 12:11
Copy link
Contributor

Code Review by Gemini

The provided change removes an explicit emptyDir volume mount for /tmp from the Loki Helm values. This change directly addresses the problem described in the commit message, where "two things [were] trying to mount into /tmp", causing a conflict.

Assuming Loki does not require a dedicated or persistent /tmp volume beyond what the container runtime provides by default (which is common for most applications), this change is correct and resolves the stated conflict without introducing new issues.

No bugs or issues are found in the provided diff. The change is a direct and appropriate solution to the described problem.

Copy link
Contributor

Code Review by Gemini

The changes align with the commit message, addressing a potential mount conflict by removing a redundant /tmp volume. This appears to be a corrective action.

No bugs or issues were found in the provided diff. The changes are a direct removal of configuration that was causing a conflict. No further improvements are suggested for the changed lines.

@maruiz93
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Aug 11, 2025
@rh-hemartin
Copy link
Contributor Author

/verify-owners

@maruiz93
Copy link
Contributor

/approve

Copy link
Contributor

@hugares hugares left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

openshift-ci bot commented Aug 11, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hugares, maruiz93, rh-hemartin

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-merge-bot openshift-merge-bot bot merged commit c88a0c2 into redhat-appstudio:main Aug 11, 2025
6 checks passed
@rh-hemartin rh-hemartin deleted the kubearchive-logging-tmp-folder branch August 11, 2025 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants