Reorganize validations for the frameworks and the externalFrameworks #2130

tenzen-y · 2024-05-05T02:10:44Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

To re-organize the frameworks and externalFrameworks validation, I changed the following:

Moved frameworks and externalFrameworks validations to the pkg/config package.
Moved registration of externalFrameworks to the SetUpControllers function in the jobframework package.
Expand validations
1. Added the duplication error for the frameworks.
2. Added the invalid format error for the externalFrameworks.

Which issue(s) this PR fixes:

Part-of #1601

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

k8s-ci-robot · 2024-05-05T02:10:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tenzen-y

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~~ [tenzen-y]

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

netlify · 2024-05-05T02:11:06Z

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

Name	Link
🔨 Latest commit	`0335c34`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-kueue/deploys/663bb158e1e0050008214e0d

k8s-ci-robot · 2024-05-06T14:38:37Z

PR needs rebase.

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.

trasc · 2024-05-08T09:42:42Z

pkg/controller/jobframework/reconciler.go

-	LabelKeysToCopy    []string
+	IntegrationOptions        map[string]any
+	EnabledFrameworks         sets.Set[string]
+	EnabledExternalFrameworks []string


Suggested change

EnabledExternalFrameworks []string

EnabledExternalFrameworks sets.Set[string]

I'm not sure about the advantage of sets.Set here because the EnabledExternalFrameworks is used only in

kueue/pkg/controller/jobframework/setup.go

Lines 50 to 54 in ddbbfdd

for _, fwkName := range options.EnabledExternalFrameworks {

if err := RegisterExternalJobType(fwkName); err != nil {

return err

}

}

Conceptually there should be no duplicates (in my opinion).

Correct. Actually, we verify the duplications here:

kueue/pkg/config/validation.go

Line 134 in ddbbfdd

allErrs = append(allErrs, field.Duplicate(integrationsExternalFrameworkPath.Index(idx), framework))

But, I'll convert type to Set here for the users who set up external controller without

kueue/pkg/controller/jobframework/setup.go

Line 47 in ddbbfdd

func SetupControllers(mgr ctrl.Manager, log logr.Logger, opts ...Option) error {

.

Thanks.

trasc · 2024-05-08T09:44:33Z

pkg/controller/jobframework/reconciler.go

@@ -135,6 +136,16 @@ func WithEnabledFrameworks(i *configapi.Integrations) Option {
 	}
 }

+// WithEnabledExternalFrameworks adds framework names managed by external controller in the Config API.
+func WithEnabledExternalFrameworks(i *configapi.Integrations) Option {


Maybe just pass the string slice of external integrations.

Yeah, I was thinking about a similar thing, but I would like to keep using this arg since I want to perform null pointer check here, not in advance.

If we use the external frameworks as an arg here, we need to verify null pointer like this, right?

var external []string if config.Integratons != nil { external = config.Integratons.ExternalFrameworks } [...] .WithEnabledExternalFrameworks(external) [...]

In my opinion the nil check should be performed by the caller, also the fact that we get that pointer when the option is created and only use it when the option is applied can be seen as problematic.

In case config.Integratons is nil, we are failing before this because of:

kueue/cmd/kueue/main.go

Line 272 in 6cceaa0

jobframework.WithIntegrationOptions(corev1.SchemeGroupVersion.WithKind("Pod").String(), cfg.Integrations.PodOptions),

For this PR, I guess, we can have it like this as well but we'll need a cleanup PR for this and WithEnabledFrameworks .

Uhm, that sounds reasonable.
Let me try to refactor the WithEnabledFrameworks and WithIntegrationOptions in another PR, first.
Thanks.

/hold

Wait, I just remembered the reason why I didn't implement the null pointer check here.

kueue/cmd/kueue/main.go

Line 272 in 6cceaa0

jobframework.WithIntegrationOptions(corev1.SchemeGroupVersion.WithKind("Pod").String(), cfg.Integrations.PodOptions),

We already checked if the cfg.integrations is not null here:

kueue/pkg/config/validation.go

Lines 128 to 130 in 6cceaa0

if c.Integrations == nil {

return field.ErrorList{field.Required(integrationsPath, "cannot be empty")}

}

So, I don't this that we should verify the null pointer for the WithIntegrationOptions.
But I agree with passing the integrations.Frameworks to the WithEnabledFrameworks instead of the integrations.

I just can change the arg of the WithEnabledFrameworks in this PR.

/hold cancel

I just can change the arg of the WithEnabledFrameworks in this PR.

It seems that we need to change many places if we want to change the arg of the WithEnabledFrameworks.
So, let me do it in the follow-ups.

/hold cancel

tenzen-y · 2024-05-08T16:05:28Z

After the fair sharing PR is merged, I will rebase this PR since both PR have conflicts.

tenzen-y · 2024-05-08T17:06:18Z

/hold cancel

Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y · 2024-05-08T17:09:12Z

I'm not sure the reason why the needs-rebase label is not removed 🧐

0335c34 (HEAD -> move-integrations-validation-to-dedicated-pkg, origin/move-integrations-validation-to-dedicated-pkg) Reorganize validations for the frameworks and the externalFrameworks
54d1c01 (origin/main, origin/HEAD) Add APIs for configuring fair sharing (#2070)
6cceaa0 [KEP] Support new ProvisioningRequest's conditions (#2042)

/assign @trasc

trasc

/lgtm

k8s-ci-robot · 2024-05-08T17:30:57Z

LGTM label has been added.

Git tree hash: 6f0c2a3e541a6587bc56d23ac3c14f63a5ac9cd8

…ubernetes-sigs#2130) Signed-off-by: Yuki Iwai <[email protected]>

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels May 5, 2024

k8s-ci-robot requested review from kerthcet and trasc May 5, 2024 02:10

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 5, 2024

tenzen-y force-pushed the move-integrations-validation-to-dedicated-pkg branch from ddbb2c5 to ddbbfdd Compare May 5, 2024 02:11

tenzen-y changed the title ~~Reorganize validatins for the frameworks and externalFrameworks~~ Reorganize validations for the frameworks and the externalFrameworks May 5, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2024

trasc reviewed May 8, 2024

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 8, 2024

tenzen-y force-pushed the move-integrations-validation-to-dedicated-pkg branch from ddbbfdd to 11b4061 Compare May 8, 2024 17:04

Reorganize validations for the frameworks and the externalFrameworks

0335c34

Signed-off-by: Yuki Iwai <[email protected]>

tenzen-y force-pushed the move-integrations-validation-to-dedicated-pkg branch from 11b4061 to 0335c34 Compare May 8, 2024 17:07

k8s-ci-robot assigned trasc May 8, 2024

trasc reviewed May 8, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 8, 2024

k8s-ci-robot merged commit 156529d into kubernetes-sigs:main May 8, 2024
15 checks passed

k8s-ci-robot added this to the v0.7 milestone May 8, 2024

tenzen-y deleted the move-integrations-validation-to-dedicated-pkg branch May 8, 2024 17:36

tenzen-y mentioned this pull request May 8, 2024

Enforce the pre null pointer check for the Integrations when performing WithEnabledFrameworks() #2166

Merged

kannon92 pushed a commit to openshift-kannon92/kubernetes-sigs-kueue that referenced this pull request Nov 19, 2024

Reorganize validations for the frameworks and the externalFrameworks (k…

e09815b

…ubernetes-sigs#2130) Signed-off-by: Yuki Iwai <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize validations for the frameworks and the externalFrameworks #2130

Reorganize validations for the frameworks and the externalFrameworks #2130

tenzen-y commented May 5, 2024 •

edited

Loading

k8s-ci-robot commented May 5, 2024

netlify bot commented May 5, 2024 •

edited

Loading

k8s-ci-robot commented May 6, 2024

trasc May 8, 2024

tenzen-y May 8, 2024

trasc May 8, 2024

tenzen-y May 8, 2024

tenzen-y May 8, 2024

trasc May 8, 2024

tenzen-y May 8, 2024

trasc May 8, 2024 •

edited

Loading

tenzen-y May 8, 2024

tenzen-y May 8, 2024

tenzen-y May 8, 2024

tenzen-y May 8, 2024

tenzen-y May 8, 2024

tenzen-y May 8, 2024

tenzen-y commented May 8, 2024 •

edited

Loading

tenzen-y commented May 8, 2024

tenzen-y commented May 8, 2024

trasc left a comment

k8s-ci-robot commented May 8, 2024

	EnabledExternalFrameworks []string
	EnabledExternalFrameworks sets.Set[string]

	for _, fwkName := range options.EnabledExternalFrameworks {
	if err := RegisterExternalJobType(fwkName); err != nil {
	return err
	}
	}

	if c.Integrations == nil {
	return field.ErrorList{field.Required(integrationsPath, "cannot be empty")}
	}

Reorganize validations for the frameworks and the externalFrameworks #2130

Reorganize validations for the frameworks and the externalFrameworks #2130

Conversation

tenzen-y commented May 5, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented May 5, 2024

netlify bot commented May 5, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-kueue canceled.

k8s-ci-robot commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trasc May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tenzen-y commented May 8, 2024 • edited Loading

tenzen-y commented May 8, 2024

tenzen-y commented May 8, 2024

trasc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 8, 2024

tenzen-y commented May 5, 2024 •

edited

Loading

netlify bot commented May 5, 2024 •

edited

Loading

trasc May 8, 2024 •

edited

Loading

tenzen-y commented May 8, 2024 •

edited

Loading