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

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.

cmd/main.go

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

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

pkg/cert/cert.go

ahg-g

few more, sorry for the late review

pkg/cert/cert.go

api/config/v1alpha1/configuration_types.go

ahg-g · 2025-01-27T06:47:09Z

api/config/v1alpha1/defaults.go

+	if cfg.LeaderElection == nil {
+		cfg.LeaderElection = &configv1alpha1.LeaderElectionConfiguration{}
+	}
+	if len(cfg.LeaderElection.ResourceName) == 0 {
+		cfg.LeaderElection.ResourceName = DefaultLeaderElectionID
+	}
+	if len(cfg.LeaderElection.ResourceLock) == 0 {
+		cfg.LeaderElection.ResourceLock = DefaultResourceLock
+	}


Since LeaderElection is a type that we import, we can default it using the upstream RecommendedDefaultLeaderElectionConfiguration from component-base.

Yes, we are importing currently. See L:77 https://github.com/kubernetes-sigs/lws/pull/325/files#diff-2f4f8def1b75dcb7b504613245740aa55320208fe63c4d75c30179f13fc7f51eR77

ahg-g · 2025-01-27T06:51:30Z

api/config/v1alpha1/configuration_types.go

+	// Host is the hostname that the webhook server binds to.
+	// It is used to set webhook.Server.Host.
+	// +optional
+	Host string `json:"host,omitempty"`
+
+	// CertDir is the directory that contains the server key and certificate.
+	// if not set, webhook server would look up the server key and certificate in
+	// {TempDir}/k8s-webhook-server/serving-certs. The server key and certificate
+	// must be named tls.key and tls.crt, respectively.
+	// +optional
+	CertDir string `json:"certDir,omitempty"`


do we actually need to make those two (host and certDir) configurable? I don't see why would someone wants to change them. I think it is important to have a tighter API and only expose what we think users will actually want to change.

Although I agree with tighter API makes sense, there are various cluster types and webhook configurations are one of the most dynamic. User may want to modify Host or temp directory for CertDir might be different than /tmp.

ardaguclu · 2025-01-27T08:09:25Z

@ahg-g I've updated based on your suggestions. Could you PTAL one more round and thank you.

ardaguclu · 2025-01-27T08:16:40Z

/hold cancel

ahg-g · 2025-01-27T20:52:43Z

api/config/v1alpha1/defaults.go

+	DefaultLeaderElectionID                    = "b8b2488c.x-k8s.io"
+	DefaultLeaderElectionLeaseDuration         = 15 * time.Second
+	DefaultLeaderElectionRenewDeadline         = 10 * time.Second
+	DefaultLeaderElectionRetryPeriod           = 2 * time.Second


I would move those to the test file since changing them is not going to actually change the default.

Makes sense. Updated.

ahg-g · 2025-01-28T05:47:45Z

/lgtm

Thanks!

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