Utilize and expose functions for starting up the manager

[tenzen-y]

What would you like to be cleaned: I would like to utilize and expose the below functions for starting up the manager: We can probably cut an initialize package under the pkg directory, and then we can put the below functions there.

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L194

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L221

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L317

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L343

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L365

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L369

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L373

https://github.com/kubernetes-sigs/kueue/blob/60177bf1ace328629bafae6ac3dcbe9ff5568aac/cmd/kueue/main.go#L403

Why is this needed:

When the platform developers implement the managers for the in-house custom jobs, they can avoid copying and pasting those functions to their manager.

[tenzen-y]

/assign

[alculquicondor]

Some of them don't seem to be necessary for a custom integration, unless you are essentially forking kueue?

My general recommendation would be run the custom integration in a separate binary that can be released independently, similar to https://github.com/kubernetes-sigs/kueue/blob/main/cmd/experimental/podtaintstolerations/main.go

[tenzen-y]

My general recommendation would be run the custom integration in a separate binary that can be released independently, similar to https://github.com/kubernetes-sigs/kueue/blob/main/cmd/experimental/podtaintstolerations/main.go

Yes, I meant a separate binary. Currently, separate main function can not import functions like setupProbeEndpoints since those functions aren't exported.

It means that the platform developer implements the main function like the following: I would like to avoid copying these similar functions. @alculquicondor Does this make sense?

func main() {
...
    ctx := ctrl.SetupSignalHandler()
    setupIndexes(ctx, mgr)
    setupProbeEndpoints(mgr)
    go setupControllers(mgr, certsReady, &cfg)

    setupLog.Info("starting manager")
    if err := mgr.Start(ctx); err != nil {
        setupLog.Error(err, "could not run manager")
        os.Exit(1)
    }

func setupIndexes(ctx context.Context, mgr ctrl.Manager) {
    err := jobframework.ForEachIntegration(func(name string, cb jobframework.IntegrationCallbacks) error {
        if err := cb.SetupIndexes(ctx, mgr.GetFieldIndexer()); err != nil {
            return fmt.Errorf("integration %s: %w", name, err)
        }
        return nil
    })
    if err != nil {
        setupLog.Error(err, "unable to setup jobs indexes")
    }
}

func setupProbeEndpoints(mgr ctrl.Manager) {
    defer setupLog.Info("probe endpoints are configured on healthz and readyz")

    if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil {
        setupLog.Error(err, "unable to set up health check")
        os.Exit(1)
    }
    if err := mgr.AddReadyzCheck("readyz", healthz.Ping); err != nil {
        setupLog.Error(err, "unable to set up ready check")
        os.Exit(1)
    }
}

func setupControllers(mgr ctrl.Manager, certsReady chan struct{}, cfg *configapi.Configuration) {
...
    opts := []jobframework.Option{
        jobframework.WithManageJobsWithoutQueueName(cfg.ManageJobsWithoutQueueName),
        jobframework.WithWaitForPodsReady(waitForPodsReady(cfg)),
    }
    err := jobframework.ForEachIntegration(func(name string, cb jobframework.IntegrationCallbacks) error {
        log := setupLog.WithValues("jobFrameworkName", name)
        if err := cb.NewReconciler(
            mgr.GetClient(),
            mgr.GetEventRecorderFor(fmt.Sprintf("%s-%s-controller", name, constants.ManagerName)),
            opts...,
...
    if err != nil {
        os.Exit(1)
    }
    // +kubebuilder:scaffold:builder
}

func waitForPodsReady(cfg *configapi.Configuration) bool {
    return cfg.WaitForPodsReady != nil && cfg.WaitForPodsReady.Enable
}

[alculquicondor]

oh I see. So you have a single binary to add multiple integrations. Then it might make sense to export things like SetupIndexes and SetupControllers.

But I don't think you should be using Kueue's Configuration API. You should probably have your own.

[tenzen-y]

oh I see. So you have a single binary to add multiple integrations. Then it might make sense to export things like SetupIndexes and SetupControllers.

Yes, I will expose the only needed functions.

But I don't think you should be using Kueue's Configuration API. You should probably have your own.

In my env, I deployed another ConfigMap embedded Kueue Config to cluster and then the another Kueue Config is mounted on the inhouse kueue manager.

It means that I have 2 configMaps embedded kueue config.

[alculquicondor]

I think we shouldn't make the same assumption in the exported functions. The functions could take some specific fields.

[tenzen-y]

I think we shouldn't make the same assumption in the exported functions. The functions could take some specific fields.

Maybe, I couldn't understand correctly what you want to mean.

But I don't think you should be using Kueue's Configuration API. You should probably have your own.

Does this mean that you suggested not mounting another Kueue's Configuration on the in-house Kueue manager? Or you meant that we shouldn't expose functions with Kueue's Configuration as args?

[alculquicondor]

I think we should export something like SetupControllers(mgr ctrl.Manager, certsReady chan struct{}, opts... jobframework.Options), without making assumptions about how your configuration API looks like.

[tenzen-y]

I think we should export something like SetupControllers(mgr ctrl.Manager, certsReady chan struct{}, opts... jobframework.Options), without making assumptions about how your configuration API looks like.

Ah, it makes sense. Thank you for the clarifications!

[mimowo]

@tenzen-y I see https://github.com/kubernetes-sigs/kueue/pull/1630 merged. Is there more to do, or we can close?

[tenzen-y]

@tenzen-y I see #1630 merged. Is there more to do, or we can close?

I'm still working on this, but I'm not sure if we can finalize this by the deadline for v0.6.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tenzen-y]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[tenzen-y]

/remove-lifecycle stale