Open
Conversation
Signed-off-by: Shekhar Saxena <shekhar.saxena@ibm.com>
Reviewer's GuideAdds Kubernetes/OpenShift deployment and service YAMLs for the kruize-optimizer component, defining how it runs, which image and ports it uses, and the environment configuration for its runtime behavior. Sequence diagram for kruize-optimizer bulk scheduling and webhook flowsequenceDiagram
participant Scheduler
participant Kruize_optimizer
participant Kruize_service
participant Webhook_consumer
Scheduler->>Kruize_optimizer: Trigger bulk API at interval KRUIZE_BULK_SCHEDULER_INTERVAL
Kruize_optimizer->>Kruize_service: Call bulk API using KRUIZE_URL
Kruize_service-->>Kruize_optimizer: Return optimization results
Kruize_optimizer->>Webhook_consumer: POST callback to KRUIZE_WEBHOOK_URL
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
deployment_openshift.yaml, the Deployment is created in theopenshiftnamespace while the Service is created in themonitoringnamespace, which will prevent the Service from selecting the pods as written; consider aligning these namespaces. - The two deployment YAMLs share a lot of duplicated configuration; consider extracting common parts (e.g., via kustomize bases/overlays or a single template with environment-specific patches) to reduce drift and make future changes easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `deployment_openshift.yaml`, the Deployment is created in the `openshift` namespace while the Service is created in the `monitoring` namespace, which will prevent the Service from selecting the pods as written; consider aligning these namespaces.
- The two deployment YAMLs share a lot of duplicated configuration; consider extracting common parts (e.g., via kustomize bases/overlays or a single template with environment-specific patches) to reduce drift and make future changes easier.
## Individual Comments
### Comment 1
<location path="deployment/deployment_openshift.yaml" line_range="20-21" />
<code_context>
+ spec:
+ containers:
+ - name: kruize-optimizer
+ image: quay.io/rh-ee-shesaxen/optimizer:0.1-test
+ imagePullPolicy: Always
+ ports:
+ - containerPort: 8080
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Reconsider hard-coded test image tag with `imagePullPolicy: Always` for non-dev environments
Using a mutable `0.1-test` tag with `imagePullPolicy: Always` can lead to non-reproducible rollbacks and unexpected image changes in shared/production-like clusters. If this is used beyond local/dev, switch to a stable, immutable tag (or digest) and adjust the pull policy, ideally parameterized per environment.
Suggested implementation:
```
- name: kruize-optimizer
# Use a stable, immutable tag here; override per environment via your deployment tooling (Helm values, kustomize overlays, or env-specific manifests)
image: quay.io/rh-ee-shesaxen/optimizer:0.1
imagePullPolicy: IfNotPresent
```
To fully implement environment-specific behavior:
1. In your deployment tooling (e.g., Helm, kustomize, or overlay manifests), make `image` and `imagePullPolicy` configurable so that:
- Dev can use a `-test` tag and `Always`.
- Non-dev/prod uses an immutable tag (or digest) and `IfNotPresent`.
2. Ensure any CI/CD pipelines or environment overlays override this base manifest with the appropriate values for each environment.
</issue_to_address>
### Comment 2
<location path="deployment/deployment_kind.yaml" line_range="1-10" />
<code_context>
+apiVersion: apps/v1
</code_context>
<issue_to_address>
**suggestion:** The two deployment manifests duplicate a lot of config that could be templatized
`deployment_kind.yaml` and `deployment_openshift.yaml` differ only in a few env-specific values (e.g., namespace, ENABLE_SWAGGER), which makes config drift likely. Consider extracting a shared base and expressing env differences via overlays/values (e.g., kustomize bases/overlays, Helm, or env substitution) so env vars, ports, and image settings are defined in one place.
Suggested implementation:
```
metadata:
name: kruize-optimizer
# Namespace is set via kustomize/overlays or environment-specific tooling
# to avoid duplicating manifests per environment.
# namespace:
labels:
```
To fully implement your suggestion and remove config drift between `deployment_kind.yaml` and `deployment_openshift.yaml`, you will also need to:
1. **Introduce kustomize (recommended) or similar templating:**
- Create a `deployment/base/kustomization.yaml` that lists `deployment_kind.yaml` as a resource.
- For each environment (e.g., kind, OpenShift), create overlays:
- `deployment/overlays/kind/kustomization.yaml` with:
- `namespace: monitoring` (or appropriate namespace).
- Any `patches` or `configMapGenerator`/`secretGenerator` needed for env-specific values like `ENABLE_SWAGGER`.
- `deployment/overlays/openshift/kustomization.yaml` with:
- `namespace: <openshift-namespace>`.
- Patches that set `ENABLE_SWAGGER`, service account, securityContext, etc.
2. **Refactor `deployment_openshift.yaml`:**
- Either:
- Remove the duplicated full Deployment manifest and replace it with an overlay that patches the shared base (`deployment_kind.yaml`).
- Or convert it into a kustomize overlay patch (e.g., strategic merge patch) that only contains the differing fields (namespace, env vars, possibly ports or image overrides).
3. **Template env vars and other duplicated fields:**
- In the parts of `deployment_kind.yaml` you didn’t show (containers, env, ports, image), keep only the common values.
- Move env-specific fields like `ENABLE_SWAGGER`, image tag overrides, or OpenShift-specific annotations into the overlays as patches.
This will ensure that Deployment configuration (env vars, ports, image settings) is defined in one place and environment differences are expressed in small, focused overlays.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+20
to
+21
| image: quay.io/rh-ee-shesaxen/optimizer:0.1-test | ||
| imagePullPolicy: Always |
There was a problem hiding this comment.
suggestion (bug_risk): Reconsider hard-coded test image tag with imagePullPolicy: Always for non-dev environments
Using a mutable 0.1-test tag with imagePullPolicy: Always can lead to non-reproducible rollbacks and unexpected image changes in shared/production-like clusters. If this is used beyond local/dev, switch to a stable, immutable tag (or digest) and adjust the pull policy, ideally parameterized per environment.
Suggested implementation:
- name: kruize-optimizer
# Use a stable, immutable tag here; override per environment via your deployment tooling (Helm values, kustomize overlays, or env-specific manifests)
image: quay.io/rh-ee-shesaxen/optimizer:0.1
imagePullPolicy: IfNotPresent
To fully implement environment-specific behavior:
- In your deployment tooling (e.g., Helm, kustomize, or overlay manifests), make
imageandimagePullPolicyconfigurable so that:- Dev can use a
-testtag andAlways. - Non-dev/prod uses an immutable tag (or digest) and
IfNotPresent.
- Dev can use a
- Ensure any CI/CD pipelines or environment overlays override this base manifest with the appropriate values for each environment.
Comment on lines
+1
to
+10
| apiVersion: apps/v1 | ||
| kind: Deployment | ||
| metadata: | ||
| name: kruize-optimizer | ||
| namespace: monitoring | ||
| labels: | ||
| app: kruize-optimizer | ||
| spec: | ||
| replicas: 1 | ||
| selector: |
There was a problem hiding this comment.
suggestion: The two deployment manifests duplicate a lot of config that could be templatized
deployment_kind.yaml and deployment_openshift.yaml differ only in a few env-specific values (e.g., namespace, ENABLE_SWAGGER), which makes config drift likely. Consider extracting a shared base and expressing env differences via overlays/values (e.g., kustomize bases/overlays, Helm, or env substitution) so env vars, ports, and image settings are defined in one place.
Suggested implementation:
metadata:
name: kruize-optimizer
# Namespace is set via kustomize/overlays or environment-specific tooling
# to avoid duplicating manifests per environment.
# namespace:
labels:
To fully implement your suggestion and remove config drift between deployment_kind.yaml and deployment_openshift.yaml, you will also need to:
-
Introduce kustomize (recommended) or similar templating:
- Create a
deployment/base/kustomization.yamlthat listsdeployment_kind.yamlas a resource. - For each environment (e.g., kind, OpenShift), create overlays:
deployment/overlays/kind/kustomization.yamlwith:namespace: monitoring(or appropriate namespace).- Any
patchesorconfigMapGenerator/secretGeneratorneeded for env-specific values likeENABLE_SWAGGER.
deployment/overlays/openshift/kustomization.yamlwith:namespace: <openshift-namespace>.- Patches that set
ENABLE_SWAGGER, service account, securityContext, etc.
- Create a
-
Refactor
deployment_openshift.yaml:- Either:
- Remove the duplicated full Deployment manifest and replace it with an overlay that patches the shared base (
deployment_kind.yaml). - Or convert it into a kustomize overlay patch (e.g., strategic merge patch) that only contains the differing fields (namespace, env vars, possibly ports or image overrides).
- Remove the duplicated full Deployment manifest and replace it with an overlay that patches the shared base (
- Either:
-
Template env vars and other duplicated fields:
- In the parts of
deployment_kind.yamlyou didn’t show (containers, env, ports, image), keep only the common values. - Move env-specific fields like
ENABLE_SWAGGER, image tag overrides, or OpenShift-specific annotations into the overlays as patches.
- In the parts of
This will ensure that Deployment configuration (env vars, ports, image settings) is defined in one place and environment differences are expressed in small, focused overlays.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR adds deployment YAMLs with desired configuration for optimizer.
Summary by Sourcery
Add Kubernetes manifests to deploy the kruize-optimizer service on Kubernetes and OpenShift clusters.
Deployment: