Skip to content
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

add seperate openshift block to handle openshift related configurations #245

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mittal-ishaan
Copy link
Contributor

@mittal-ishaan mittal-ishaan commented Dec 26, 2024

Adding separate openshift block to handle openshift-related configurations and make it easier for users to install opencost in an openshift environment.
However, this comes with some breaking changes:

  1. Users that had configured in-cluster Prometheus configuration will have to update their helm values to match with new config.

The following helm values will now work for openshift opencost installation:

opencost:
  platforms:
    openshift:
      enabled: true
  ui:
    extraVolumeMounts:
      - name: opencost-ui-nginx-config-volume
        mountPath: /etc/nginx/conf.d/default.nginx.conf
        subPath: default.nginx.conf

extraVolumes:
  - name: opencost-ui-nginx-config-volume
    configMap:
      name: opencost-ui-nginx-config

With #244 changes

opencost:
  platforms:
    openshift:
      enabled: true

For installing opencost with in-cluster prometheus in openshift environment:

opencost:
 metrics:
   serviceMonitor:
     enabled: true
 prometheus:
   kubeRBACProxy: true
   external:
     enabled: true
     url: https://prometheus-k8s.openshift-monitoring.svc.cluster.local:9091
   internal:
     enabled: false
 platforms:
   openshift:
     enabled: true
     createMonitoringClusterRoleBinding: true
     createMonitoringResourceReaderRoleBinding: true
     monitoringServiceAccountName: prometheus-k8s
     monitoringServiceAccountNamespace: openshift-monitoring

Although it is recommended to remove OpenShift-specific extraVolumes and extraVolumeMounts, keeping them as they are will not cause any issues. Kubernetes applies only the first defined volume mount for a given path, ensuring that duplicates do not break anything.

I have placed extraVolumes and extraVolumeMounts in the deployment before the OpenShift-specific ones so that user-defined values always take precedence in case of duplicate. While I could not find official documentation confirming this behavior, I have tested it by installing OpenCost with these changes in an OpenShift cluster.

The only scenario where this could result in an error is if we install OpenCost for the first time with these changes and define extraVolumes and extraVolumeMounts with empty-var-www. In that case, Kubernetes would throw the following error:

Error: 1 error occurred:
	* Deployment.apps "opencost" is invalid: spec.template.spec.containers[1].volumeMounts[2].mountPath: Invalid value: "/var/www": must be unique

However, if OpenCost is already installed with this volume and volume mount, upgrading with this chart will not produce an error.

@mittal-ishaan mittal-ishaan marked this pull request as ready for review December 26, 2024 22:13
@kastl-ars
Copy link
Contributor

Any news on this one? This looks promising...

@toscott
Copy link
Collaborator

toscott commented Jan 20, 2025

Hi @mittal-ishaan. Is there a conversation or specific problem this is trying to solve? It looks like it is several backwards incompatible changes and doesn't provide any functionality that can't already be achieved using the existing:
opencost.ui.extraVolumeMounts

Appears like its an attempt to maybe simplify configuration for openshift clusters?

@mittal-ishaan
Copy link
Contributor Author

Hi @toscott,

The primary goal here is to simplify the installation process for OpenCost in OpenShift environments. Currently, setting up OpenCost in OpenShift is challenging, as users need to sift through various issues and discussions to find the necessary volume mounts, as detailed in this issue. By including these configurations directly in the Helm chart, I hope to make the installation process more straightforward for OpenShift users. Additionally, I want to centralize and gate other OpenShift-specific configurations.

What are your thoughts?

@toscott
Copy link
Collaborator

toscott commented Feb 3, 2025

Hi @toscott,

The primary goal here is to simplify the installation process for OpenCost in OpenShift environments. Currently, setting up OpenCost in OpenShift is challenging, as users need to sift through various issues and discussions to find the necessary volume mounts, as detailed in this issue. By including these configurations directly in the Helm chart, I hope to make the installation process more straightforward for OpenShift users. Additionally, I want to centralize and gate other OpenShift-specific configurations.

What are your thoughts?

Sorry for the delay.

I like the idea of simplifying configuration for new users. It does make me a little nervous to break backward compatibility though.
With your current changes, I would recommend a major version bump for the chart so anyone who has auto minor/patch version updates enabled doesn't get surprised with something breaking.

What do you think about bumping the chart to 2.0.0 and calling it good? Also glad to see you were added as a contributor! Welcome!

@mittal-ishaan
Copy link
Contributor Author

Thank you @toscott
Sure, agree with bumping the chart version to 2.0.0 . I will update it.

@mittal-ishaan mittal-ishaan marked this pull request as draft February 9, 2025 17:13
@mittal-ishaan mittal-ishaan marked this pull request as ready for review February 9, 2025 19:05
@v0nNemizez
Copy link

where are we on this?

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