Introduce new component config flag #325

ardaguclu · 2025-01-16T10:10:24Z

What type of PR is this?

/kind feature

What this PR does / why we need it

This PR adds configuration file and flag to customize the functionality via new config file resource which is already supported by Kueue and JobSet.

Which issue(s) this PR fixes

Fixes #170, #322

Does this PR introduce a user-facing change?

Introducing new config flag to customize behaviors via a flag

k8s-ci-robot · 2025-01-16T10:10:34Z

Welcome @ardaguclu!

It looks like this is your first PR to kubernetes-sigs/lws 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/lws has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2025-01-16T10:10:36Z

Hi @ardaguclu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ardaguclu · 2025-01-16T10:15:14Z

This PR is mostly inspired by kubernetes-sigs/jobset#609 and nearly all credit should still go to @rainfd :)

kannon92 · 2025-01-16T15:29:02Z

/ok-to-test

api/config/v1alpha1/configuration_types.go

kannon92 · 2025-01-16T15:32:53Z

pkg/config/validation.go

+func validateInternalCertManagement(c *configapi.Configuration) field.ErrorList {
+	var allErrs field.ErrorList
+	if c.InternalCertManagement == nil || !ptr.Deref(c.InternalCertManagement.Enable, false) {
+		return allErrs


This would return a nil error.

Maybe you should add an error message here that if certManagement is enabled we need the internalCertManager filed out.

Thank you for review @kannon92.

I'm not sure I fully understand, If internal certificate management is disabled, shouldn't we return nil?. As far as I understand from the code block, if internal cert management is nil or false, we don't need to pursue further validation for the other fields.

Yea that is true. Maybe we just return nil there to make it clear we don’t expect an error.

I think this is fine as is. I was thinking we could validate if the settings are set if it’s enabled false so we bring awareness that they don’t be used

api/config/v1alpha1/configuration_types.go

api/config/v1alpha1/defaults.go

api/config/v1alpha1/defaults_test.go

api/config/v1alpha1/groupversion_info.go

kannon92 · 2025-01-16T15:39:23Z

The e2e tests are failing due to some oddities with the docker file.

https://github.com/kubernetes-sigs/lws/blob/main/Dockerfile#L19

I think you need to add config to the dockerfile.

Edwinhr716 · 2025-01-16T21:30:29Z

@ahg-g is there a reason why we copy all the folders inside pkg/ separately instead of just copying the whole folder?

ardaguclu · 2025-01-17T05:47:32Z

/label tide/merge-method-squash

kannon92 · 2025-01-17T21:06:00Z

@ahg-g is there a reason why we copy all the folders inside pkg/ separately instead of just copying the whole folder?

I think this comes from the default template for kubebuilder.

kannon92 · 2025-01-19T14:52:42Z

Could you update manager to use component config as default for kustomize and helm?

https://github.com/kubernetes-sigs/lws/blob/main/config/manager/manager.yaml

see https://github.com/kubernetes-sigs/jobset/pull/724/files for an example

ardaguclu · 2025-01-20T05:21:42Z

Could you update manager to use component config as default for kustomize and helm?

https://github.com/kubernetes-sigs/lws/blob/main/config/manager/manager.yaml

see https://github.com/kubernetes-sigs/jobset/pull/724/files for an example

@kannon92 I believe that I've successfully updated the required configurations for this.

kannon92 · 2025-01-20T15:29:45Z

LGTM

/assign @ahg-g @kerthcet @Edwinhr716

kerthcet

Thanks @ardaguclu This is great, generally LGTM.

Can we have one more test to cover that: the flags will overwrite the config file? And left another comment about Remove the flags in the future.

kerthcet · 2025-01-21T03:07:14Z

cmd/main.go

-	kubeConfig.QPS = float32(qps)
-	kubeConfig.Burst = burst
+
+	kubeConfig.QPS = *cfg.ClientConnection.QPS


Can we remove the flags then since we have configuration files. We can mark them as deprecated in the follow up and remove them finally. And no need to handle the fallback here.

I agree with you about the deprecation of the flags and can be done in a followup PR.

ardaguclu · 2025-01-21T06:31:00Z

/retest

ardaguclu · 2025-01-21T06:39:17Z

/test pull-lws-test-integration-main
again to ensure that there is no issue

ardaguclu · 2025-01-21T06:41:42Z

Thanks @ardaguclu This is great, generally LGTM.

Can we have one more test to cover that: the flags will overwrite the config file? And left another comment about Remove the flags in the future.

@kerthcet thank you for review. I've added unit tests based on your suggestions.

kerthcet · 2025-01-23T10:44:22Z

cmd/main.go

@@ -116,6 +143,10 @@ func main() {
 		c.NextProtos = []string{"http/1.1"}
 	}

+	if !flagsSet["metrics-bind-address"] {


I think we can move this logic to the apply function as well, right, to make that function more pure. But it's ok to defer this with the deprecation PR together, this one is good enough to me. Feel free to unhold the PR.
/lgtm
/approve
/hold

Thanks @ardaguclu also cc @ahg-g especially about the deprecation decision.

Thank you for review. I'll give one day for lazy consensus and unhold the PR.

k8s-ci-robot · 2025-01-23T10:44:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, kerthcet

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:

~~OWNERS~~ [kerthcet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahg-g · 2025-01-21T06:31:14Z

config/crd/bases/lws.x-k8s.io_configurations.yaml

@@ -0,0 +1,54 @@
+---
+apiVersion: apiextensions.k8s.io/v1


@kerthcet Component config is not a CRD (not an object api), do we really need this definition?

It is not stored in API server. You are right, we can remove this.

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 16, 2025

k8s-ci-robot requested review from ahg-g and Edwinhr716 January 16, 2025 10:10

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 16, 2025

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 16, 2025

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 16, 2025