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

feat: configure standardResources via controller configmap #1490

Merged
merged 7 commits into from
Feb 5, 2024

Conversation

inishchith
Copy link
Contributor

@inishchith inishchith commented Feb 2, 2024

Better way to configure the memory and CPU resource requests

closes #1487

Sample configmap

apiVersion: v1
kind: ConfigMap
metadata:
  name: numaflow-controller-config
data:
  controller-config.yaml: |+
    defaults:
      containerResources: |
        limits:
          memory: "256Mi"
          cpu: "200m"
        requests:
          memory: "128Mi"
          cpu: "100m"

@inishchith inishchith force-pushed the config-standard-resource branch 2 times, most recently from 12bcf65 to b0c95db Compare February 2, 2024 03:00
closes numaproj#1487

Signed-off-by: Nishchith Shetty <inishchith@gmail.com>
Signed-off-by: Nishchith Shetty <inishchith@gmail.com>
@vigith
Copy link
Contributor

vigith commented Feb 2, 2024

Is standardResources only for UDF containers?

pkg/reconciler/config.go Outdated Show resolved Hide resolved
pkg/reconciler/config.go Outdated Show resolved Hide resolved
pkg/apis/numaflow/v1alpha1/get_spec_req.go Outdated Show resolved Hide resolved
pkg/apis/numaflow/v1alpha1/get_spec_req.go Outdated Show resolved Hide resolved
pkg/apis/numaflow/v1alpha1/get_spec_req.go Outdated Show resolved Hide resolved
pkg/apis/numaflow/v1alpha1/get_spec_req.go Outdated Show resolved Hide resolved
pkg/apis/numaflow/v1alpha1/get_spec_req.go Outdated Show resolved Hide resolved
@inishchith
Copy link
Contributor Author

inishchith commented Feb 2, 2024

Is standardResources only for UDF containers?

@vigith - Not just UDF. All init and main containers for vertex, daemon, redis_buffer_service.

Signed-off-by: Nishchith Shetty <inishchith@gmail.com>
Signed-off-by: Nishchith Shetty <inishchith@gmail.com>
Signed-off-by: Nishchith Shetty <inishchith@gmail.com>
@inishchith inishchith marked this pull request as ready for review February 4, 2024 01:10
@whynowy
Copy link
Member

whynowy commented Feb 4, 2024

@inishchith - It looks good to me, can you please:

  1. Add the new config to https://github.com/numaproj/numaflow/blob/main/config/base/controller-manager/numaflow-controller-config.yaml#L7, use the default resource settings;
  2. Add some doc to https://github.com/numaproj/numaflow/blob/main/docs/operations/controller-configmap.md, also add the new config to https://github.com/numaproj/numaflow/blob/main/docs/operations/numaflow-controller-config.yaml.
  3. Appreciate if you could add a config_test.go to cover the unit tests (unit tests for config.go are missing, you can add the one for the new function you added, but always welcome if you could cover more).

@inishchith
Copy link
Contributor Author

@whynowy Thanks!

I have updated the docs and config.
I have also added minimal tests that covers the newly introduced method. Covering the config file might require refactoring some LoadConfig, which I think might diverge from the goal of this change list. Let me know your thoughts.

pkg/reconciler/config_test.go Outdated Show resolved Hide resolved
pkg/reconciler/config.go Outdated Show resolved Hide resolved
docs/operations/controller-configmap.md Outdated Show resolved Hide resolved
pkg/reconciler/config.go Outdated Show resolved Hide resolved
@inishchith inishchith force-pushed the config-standard-resource branch 2 times, most recently from f97b00f to 8ffff3a Compare February 5, 2024 06:26
Signed-off-by: Nishchith Shetty <inishchith@gmail.com>
Copy link
Member

@whynowy whynowy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@vigith vigith merged commit 20cf66d into numaproj:main Feb 5, 2024
20 checks passed
whynowy pushed a commit that referenced this pull request Feb 11, 2024
Signed-off-by: Nishchith Shetty <inishchith@gmail.com>
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.

Better way to configure the memory and CPU resource requests
3 participants