config: Split KubeMacPool monitoring manifests into separate YAML#598
Conversation
Summary of ChangesHello @RamLavi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the flexibility of KubeMacPool deployments by separating its core Kubernetes manifests from those specifically designed for Prometheus monitoring. This architectural change allows users to install KubeMacPool without a hard dependency on Prometheus-operator, catering to environments where Prometheus might not be installed or is managed differently. The modification primarily involves reorganizing existing manifest files and updating build scripts to reflect this new modular structure, ensuring that the core functionality of KubeMacPool remains unaffected while improving deployment options. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully separates the monitoring-related Kubernetes manifests from the core kubemacpool.yaml file. This is a good change that allows users to optionally install the Prometheus-operator resources. The changes in the Makefile to generate and deploy the new kubemacpool-monitoring.yaml are correct. I've also noticed that the deploy target now correctly uses the release manifest instead of the test manifest, which is a nice fix. I have one suggestion for the Makefile to avoid generating the same monitoring manifest multiple times.
| $(KUSTOMIZE) build config/monitoring > config/release/kubemacpool-monitoring.yaml | ||
|
|
||
| generate-test: $(GO) manifests | ||
| $(KUSTOMIZE) build config/test > config/test/kubemacpool.yaml | ||
| $(KUSTOMIZE) build config/monitoring > config/test/kubemacpool-monitoring.yaml | ||
|
|
||
| generate-external: $(GO) manifests | ||
| cp -r config/test config/external | ||
| cd config/external && \ | ||
| $(KUSTOMIZE) edit set image quay.io/kubevirt/kubemacpool=$(REGISTRY)/$(IMG) | ||
| $(KUSTOMIZE) build config/external > config/external/kubemacpool.yaml | ||
| $(KUSTOMIZE) build config/monitoring > config/external/kubemacpool-monitoring.yaml |
There was a problem hiding this comment.
The command $(KUSTOMIZE) build config/monitoring is used in generate-deploy, generate-test, and generate-external to create monitoring manifests. Since config/monitoring is a standalone kustomize base, the generated manifest will be identical in all cases. This leads to unnecessary file duplication and repeated work during the build process.
Consider generating the monitoring manifest only once to a single location (e.g., config/kubemacpool-monitoring.yaml), and then have the deploy and deploy-test targets reference that single file. This would make the build process more efficient and the Makefile easier to maintain.
RamLavi
force-pushed
the
split_monitoring_yamls
branch
from
89fab2e to
b0d6ec4
Compare
|
/retest |
RamLavi
force-pushed
the
split_monitoring_yamls
branch
3 times, most recently
from
72f92b1 to
a0573db
Compare
|
Change: deploy kmp monitoring objects only when CRDs installed |
README.md
Outdated
|
|
||
| **Note:** For VirtualMachines, Kubemacpool supports primary interface MAC address allocation for [masquerade](https://kubevirt.io/user-guide/virtual_machines/interfaces_and_networks/#masquerade) binding mechanism. | ||
|
|
||
| **Monitoring manifests:** The release manifests are split into `kubemacpool.yaml` (core) and `kubemacpool-monitoring.yaml` (Prometheus Operator resources like `ServiceMonitor`/`PrometheusRule`). Apply the monitoring manifest only when those CRDs are available. |
There was a problem hiding this comment.
nit:
I would reword this section and describe it as the project support Prometheus support.
And to integrate with Prometheus say install the mentioned manifests.
Makefile
Outdated
| $(KUBECTL) apply -f config/release/kubemacpool.yaml | ||
| $(KUBECTL) apply -f config/release/kubemacpool-monitoring.yaml |
There was a problem hiding this comment.
Previously deploy used the manifest in config/test, why do we use the one from release now?
Dont we need Prometheus presense check here as well?
There was a problem hiding this comment.
ahh good catch!
Actually due to the changes we did on KMP, we might not need the separate tests config, but we should do that on separate context..
Right now kubemacpool deploys objects the require prometheus to be deployed on the cluster. This should not be a tight dependency. This commit splits monitoring related manifests so Prometheus-operator resources can be opted in independently of core installation This does not change current deployment logic, it just separates the manifests so that users could deploy kubemcapool on clusters where prometheus is not installed. Assisted-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ram Lavi <ralavi@redhat.com>
Signed-off-by: Ram Lavi <ralavi@redhat.com>
RamLavi
force-pushed
the
split_monitoring_yamls
branch
from
a0573db to
1ad32f9
Compare
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RamLavi The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Upstream kubemacpool added monitoring infrastructure [0][1][2]. - Adding the added objects, with configurable params that will be rendered on runtime by CNAO. - wrapping these objects by another param MonitoringAvailable that will be also rendered on realtime, so these objects will be deployed only then prometheus is installed on the cluster. [0] k8snetworkplumbingwg/kubemacpool#596 [1] k8snetworkplumbingwg/kubemacpool#587 [2] k8snetworkplumbingwg/kubemacpool#598 Assited-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ram Lavi <ralavi@redhat.com>
* bump kubemacpool to v0.50.0-18-gcf11f30 Signed-off-by: CNAO Bump Bot <noreply@github.com> * e2e/kubemacpool: Set monitoring lane env var Doing so tells CNAO to configure the monitoring components using the correct prometheus ns Signed-off-by: Ram Lavi <ralavi@redhat.com> * components/kubemacpool: Add monitoring objects Upstream kubemacpool added monitoring infrastructure [0][1][2]. - Adding the added objects, with configurable params that will be rendered on runtime by CNAO. - wrapping these objects by another param MonitoringAvailable that will be also rendered on realtime, so these objects will be deployed only then prometheus is installed on the cluster. [0] k8snetworkplumbingwg/kubemacpool#596 [1] k8snetworkplumbingwg/kubemacpool#587 [2] k8snetworkplumbingwg/kubemacpool#598 Assited-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ram Lavi <ralavi@redhat.com> * components/kubemacpool: Templatize monitoring params for SA Upstream kubemacpool added monitoring infrastructure [0] with hardcoded prometheus-k8s service account and monitoring namespace in the RoleBinding subjects. For CNAO, these need to be configurable via template variables, consistent how CNAO already handles it. [0] k8snetworkplumbingwg/kubemacpool#596 Assited-by: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Ram Lavi <ralavi@redhat.com> --------- Signed-off-by: CNAO Bump Bot <noreply@github.com> Signed-off-by: Ram Lavi <ralavi@redhat.com> Co-authored-by: CNAO Bump Bot <noreply@github.com> Co-authored-by: Ram Lavi <ralavi@redhat.com>
3 participants
What this PR does / why we need it:
Right now kubemacpool deploys objects the require prometheus to be deployed on the cluster.
This should not be a tight dependency.
This PR splits monitoring related manifests so Prometheus-operator resources can be opted in independently of core installation.
This does not change current deployment logic, it just separates the manifests so that users could deploy kubemcapool on clusters where prometheus is not installed.
Special notes for your reviewer:
Release note: