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

Introduce managedBy field #440

Closed
Tracked by #438
tenzen-y opened this issue Mar 4, 2024 · 21 comments
Closed
Tracked by #438

Introduce managedBy field #440

tenzen-y opened this issue Mar 4, 2024 · 21 comments
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@tenzen-y
Copy link
Member

tenzen-y commented Mar 4, 2024

What would you like to be added:
Remove the alpha.jobset.sigs.k8s.io/managed-by label and introduce .spec.managedBy field.

@mimowo @ahg-g @alculquicondor WDYT?

/kind feature

Why is this needed:
Since #407, we introduced the new managed-by label based on the batch/job KEP: kubernetes/enhancements#4370.

But, we decided to introduce a new managedBy field to batch/job here: kubernetes/enhancements#4530
So, I think that we should use the managedBy field instead of the dedicated label as well as batch/job.

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 4, 2024
@ahg-g
Copy link
Contributor

ahg-g commented Mar 4, 2024

We added in the previous release, see #379

@ahg-g ahg-g closed this as completed Mar 4, 2024
@ahg-g ahg-g reopened this Mar 4, 2024
@ahg-g
Copy link
Contributor

ahg-g commented Mar 4, 2024

oh, you are referring to a field. Yeah, that sounds good, consistency with job is good.

@kannon92
Copy link
Contributor

kannon92 commented Mar 4, 2024

Should we revert that PR then?

@alculquicondor
Copy link

+1 for replacing with a field.

I suppose the alpha annotation already made it to the release. I would suggest keeping both mechanism for at least one release, to facilitate upgrades to Kueue+Jobset.

@tenzen-y
Copy link
Member Author

tenzen-y commented Mar 4, 2024

+1 for replacing with a field.

I suppose the alpha annotation already made it to the release. I would suggest keeping both mechanism for at least one release, to facilitate upgrades to Kueue+Jobset.

That makes sense.

@kannon92
Copy link
Contributor

kannon92 commented Mar 4, 2024

I tried a quick revert option on the PR but that failed so reverting may be a bit of a manual process..

@mimowo
Copy link
Contributor

mimowo commented Mar 20, 2024

+1 for using field, for ecosystem consistency, but also so that we fail fast when someone tries to use it on an old version (same argument as for the k8s job).

@trasc
Copy link
Contributor

trasc commented Mar 20, 2024

/assign

@danielvegamyhre
Copy link
Contributor

@trasc have you already started this? I was hoping to have a team member of mine work on this as a good first issue to ramp up on JobSet, we just hadn't assigned it to him yet, so i'd like to assign it to them if that's okay

@trasc
Copy link
Contributor

trasc commented Mar 21, 2024

@danielvegamyhre Not yet.

/unassign

@danielvegamyhre
Copy link
Contributor

Thanks!

/assign @jedwins1998

@k8s-ci-robot
Copy link
Contributor

@danielvegamyhre: GitHub didn't allow me to assign the following users: jedwins1998.

Note that only kubernetes-sigs members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Thanks!

/assign @jedwins1998

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/test-infra repository.

@jedwins1998
Copy link
Contributor

/assign

@kannon92
Copy link
Contributor

What is our deprecation policy for the managed-by label?

@danielvegamyhre
Copy link
Contributor

danielvegamyhre commented Mar 22, 2024

What is our deprecation policy for the managed-by label?

I just realized this change was included in the v0.3.0 release. Given that, I think we should support both the label and the field for a period before removing the label. Maybe for a couple releases? And we can emit a warning the label is being deprecated at JobSet creation time. What do you think?

@kannon92
Copy link
Contributor

I brought this on kubernetes-sigs/kueue#1870 to figure out what we want to do.

@danielvegamyhre
Copy link
Contributor

Update: we can safely drop the label entirely per conversation here kubernetes-sigs/kueue#1870 (comment)

@danielvegamyhre
Copy link
Contributor

danielvegamyhre commented Mar 27, 2024

Some additional notes for implementation:

  • Add a JobSetSpec field analogous to the one in the JobSpec here.
  • Replace references to label with the field in jobset controller, unit tests, and integration tests
  • Regenerate manifests (JobSet CRD, RBACs, service accounts etc which are generated from the API definition and kubebuilder markers): make manifests
  • Regenerate Go and Python SDKs: make generate
  • Ensure all unit, integration, e2e tests are passing:
    • unit: make test
    • integration: make test-integration
    • e2e: make test-e2e-kind
  • Run make verify to run linter, go vet, etc and make sure api/, config/, client-go/ are all updated together.

@jedwins1998 you can generalize this into an improved contributor guide like you were talking about as well

@jedwins1998
Copy link
Contributor

Created #487 as an initial implementation.

@jedwins1998
Copy link
Contributor

PR has been merged. Will this issue auto-close or is there something I need to do?

@danielvegamyhre
Copy link
Contributor

PR has been merged. Will this issue auto-close or is there something I need to do?

It will auto-close if you add "Fixes #issueNumber" in the issue description. We can close this one manually. Thanks for your work on this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

No branches or pull requests

9 participants