Build kueue-populator artifacts for release. #7940
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
|
@mbobrovskyi: GitHub didn't allow me to request PR reviews from the following users: j-skiba. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this: 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. |
|
|
||
| if [ "$HELM_CHART_PUSH" = "true" ]; then | ||
| ${HELM} push "${DEST_CHART_DIR}/kueue-${chart_version}.tgz" "oci://${HELM_CHART_REPO}" | ||
| ${HELM} push "${DEST_CHART_DIR}/kueue-populator-${chart_version}.tgz" "oci://${HELM_CHART_REPO}" |
There was a problem hiding this comment.
there's a separate script for that - https://github.com/kubernetes-sigs/kueue/blob/main/cmd/experimental/kueue-populator/hack/helm-chart-package.sh
There was a problem hiding this comment.
Do we need separate script if it is a part of Kueue?
There was a problem hiding this comment.
Not necessarily, whatever works.
There was a problem hiding this comment.
The initial idea was to make kueue-populator as independent as possible. Also as it's experimental project we are not sure if it will remain in this repository in future and we want to keep it as a separate thing. wdyt @mimowo ?
There was a problem hiding this comment.
I think we need to decide now: should it be part of Kueue (tagged v0.15.0) or released separately (v0.1.0)?
If we release it under v0.15.0, we won’t be able to downgrade to v0.1.0 later.
There was a problem hiding this comment.
This tool is tightly related to Kueue. I don’t think we need to move it to a separate repository.
We moved kjob out because it isn’t required by Kueue, but this tool directly depends on Kueue.
So I think it should stay in the Kueue repo, in a separate directory under cmd — just like kueueviz and importer.
There was a problem hiding this comment.
This tool is tightly related to Kueue. I don’t think we need to move it to a separate repository.
Sure, we don't want to move to separate repo for sure, it is going to be part of Kueue repo, just an add on project like kueueviz
There was a problem hiding this comment.
So I think it should stay in the Kueue repo, in a separate directory under cmd — just like kueueviz and importer.
Yes, we can move under cmd in a follow up. It was moved to experiment just because it is very early days of the project, but it belongs to cmd/ conceptually
|
https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_kueue/7940/pull-kueue-populator-test-e2e-main/1993976157153792000 |
|
/test pull-kueue-populator-test-e2e-main |
|
kueue dependency was removed from Chart.yaml, I think it's the cause |
Good catch, so I think we should:
|
Not sure that’s a good idea. We release kueue-populator together with Kueue, so the versions should stay the same. |
Eventually yes, but for 0.15.0 I'm ok with dependency on 0.14.4. Populator should work with Kueue 0.14+ which graduated TAS to Beta. The main thing to check is if someone has pre-installed Kueue 0.15.0, then the populator should not try to downgrade |
@j-skiba have you had a chance to check the syntax for dependency to ensure it will accept preinstalled 0.15.0? |
|
no, I haven't. I will take a look |
|
seems to be working |
nice, so you pre-installed 0.15, and the prepopulator dependency was ok with that |
|
@j-skiba IIUC you tested with the dependency? |
Correct. As long as
Yes, I did manually and the tests run like this |
|
I see so you had to add |
|
@mimowo I'm not 100% sure but |
ok, sgtm |
c482b5a to
4d8f407
Compare
8865bbb to
54346b1
Compare
54346b1 to
a2b78c6
Compare
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 6c5a926753d432f6258af51af22e4cf7a1eee072 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mbobrovskyi, mimowo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Thank you @mbobrovskyi and @j-skiba for collaborating on the release process for the sub-project 👍 |
| - name: kueue | ||
| version: "~0.14.4" | ||
| repository: "oci://registry.k8s.io/kueue/charts" | ||
| repository: "file://../../../../../charts/kueue" |
There was a problem hiding this comment.
If we release that chart don't we have to replace that back to "oci://registry.k8s.io/kueue/charts"?
There was a problem hiding this comment.
I'm not sure if this will work if someone runs it this way:
helm install kueue-populator oci://us-central1-docker.pkg.dev/k8s-staging-images/kueue/charts/kueue-populator --version=<SOME_VERSION> --create-namespace --namespace=kueue-system --set kueue.enabled=true --waitwith kueue.enabled=true then this reference:
repository: "file://../../../../../charts/kueue"
doesn't exist I think
There was a problem hiding this comment.
or is it bundled in the package "kueue-populator" already?
There was a problem hiding this comment.
the question, will this work for someone installing from LWS, or another repo?
There was a problem hiding this comment.
Yes, I tested it after build with command
➜ kueue git:(cleanup/kueue-populator-artifacts) ✗ make artifacts IMAGE_REGISTRY=us-central1-docker.pkg.dev/k8s-staging-images/kueue GIT_TAG=v0.15.0-rc.2
➜ kueue git:(cleanup/kueue-populator-artifacts) ✗ helm install kueue-populator ./artifacts/kueue-populator-0.15.0-rc.2.tgz \
--namespace kueue-system \
--create-namespace \
--set kueue.enabled=true \
--wait --timeout 300s
➜ kueue git:(cleanup/kueue-populator-artifacts) ✗ k get pods -A
NAMESPACE NAME READY STATUS RESTARTS AGE
kube-system coredns-66bc5c9577-4s4tx 1/1 Running 0 70s
kube-system coredns-66bc5c9577-jwxfn 1/1 Running 0 70s
kube-system etcd-kind-control-plane 1/1 Running 0 78s
kube-system kindnet-dk57m 1/1 Running 0 71s
kube-system kube-apiserver-kind-control-plane 1/1 Running 0 77s
kube-system kube-controller-manager-kind-control-plane 1/1 Running 0 77s
kube-system kube-proxy-v75c2 1/1 Running 0 71s
kube-system kube-scheduler-kind-control-plane 1/1 Running 0 77s
kueue-system kueue-populator-controller-manager-5f4d455cc8-dz5md 1/1 Running 0 70s
kueue-system kueue-populator-kueue-populator-fb764bfd4-lxwhk 0/1 ImagePullBackOff 0 70s
local-path-storage local-path-provisioner-7b8c8ddbd6-kjd8n 1/1 Running 0 69s
kueue-populator-kueue-populator-fb764bfd4-vct6m with error because we don't have image yet.
Butk kueue-populator-controller-manager-5f4d455cc8-rxp5x running successfully .
There was a problem hiding this comment.
It works, I tested.
If that works then it's ok for me.
There was a problem hiding this comment.
I still thinking that we should install it separately.
There was a problem hiding this comment.
wait we have enabled: false so we install seperately by default, only if requested by user it is combined.
It makes sense I think. In package managers like apt you usually get depedencies installed, unless you already have it installed. In any case users have a choice now.
|
/unhold |
|
Let's merge and see how RC3 is going |
|
Trying to test |
|
Ah, my bad. Looks like working fine. |
|
/remove-kind cleanup |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Build kueue-populator artifacts for release.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?