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

Helm install Error #1459

Closed
B1F030 opened this issue Dec 15, 2023 · 27 comments · Fixed by #1507
Closed

Helm install Error #1459

B1F030 opened this issue Dec 15, 2023 · 27 comments · Fixed by #1507
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@B1F030
Copy link
Member

B1F030 commented Dec 15, 2023

What happened:

[root@test-1 charts]# helm install kueue kueue/ --create-namespace --namespace kueue-system
Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest:
 error validating "": error validating data: [apiVersion not set, kind not set]

What you expected to happen:
install success
How to reproduce it (as minimally and precisely as possible):

git clone https://github.com/kubernetes-sigs/kueue.git
cd kueue/charts
helm install kueue kueue/ --create-namespace --namespace kueue-system

Anything else we need to know?:
I tried helm template ./charts/kueue --values ./charts/kueue/values.yaml to debug where did this error happen,
then I find out:

---
# Source: kueue/templates/visibility/kustomization.yaml
resources:
- apiservice.yaml
- role_binding.yaml
- service.yaml
---

This is where helm got Error, so I tried to remove kueue/templates/visibility/kustomization.yaml, it works.
So I wonder, should we remove this file?

Environment:

  • Kubernetes version (use kubectl version): v1.26.0
  • Kueue version (use git describe --tags --dirty --always): main
  • Cloud provider or hardware configuration:
  • OS (e.g: cat /etc/os-release):
  • Kernel (e.g. uname -a):
  • Install tools: Helm
  • Others: helm version: v3.9.0
@B1F030 B1F030 added the kind/bug Categorizes issue or PR as related to a bug. label Dec 15, 2023
@B1F030
Copy link
Member Author

B1F030 commented Dec 15, 2023

I can help fix that, just need to make sure if that is feasible first.
/assign

@B1F030
Copy link
Member Author

B1F030 commented Dec 15, 2023

@PBundyra What do you think?

@B1F030
Copy link
Member Author

B1F030 commented Dec 15, 2023

I found that this bug may not be easily fixed by removing the kustomization.yaml.
Maybe we need to keep the file, but still have to fix this bug.
I tried add apiVersion and kind for it:

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - apiservice.yaml
  - role_binding.yaml
  - service.yaml

But it still don't work:

[root@test-1 charts]# helm install kueue kueue/ --namespace kueue-system
Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest:
 resource mapping not found for name: "" namespace: "" from "": 
no matches for kind "Kustomization" in version "kustomize.config.k8s.io/v1beta1"

I'll keep trying to fix this.

@tenzen-y
Copy link
Member

@B1F030 We can remove the kueue/templates/visibility/kustomization.yaml file since helm doesn't recognize customization files.

@tenzen-y
Copy link
Member

cc @mimowo

@mimowo
Copy link
Contributor

mimowo commented Dec 15, 2023

Here is where we copy the visibility files for helm:

kueue/hack/update-helm.sh

Lines 99 to 120 in a68c958

# Add visibility files, replace names, namespaces in helm format
for output_file in ${DEST_VISIBILITY_DIR}/*.yaml; do
# The name of the v1alpha1.visibility.kueue.x-k8s.io APIService needs to remain unchanged.
if [ "$(cat $output_file | $YQ '.metadata | has("name")')" = "true" ] &&
[ "$(cat $output_file | $YQ '.metadata.name | (. == "v1alpha1*")')" = "false" ]; then
$YQ -N -i '.metadata.name |= "{{ include \"kueue.fullname\" . }}-" + .' $output_file
fi
# The namespace of the visibility-server-auth-reader rolebinding needs to remain unchanged.
if [ "$(cat $output_file | $YQ '.metadata | has("namespace")')" = "true" ] &&
[ "$(cat $output_file | $YQ '.metadata.namespace | (. == "kube-system")')" = "false" ]; then
$YQ -N -i '.metadata.namespace = "{{ .Release.Namespace }}"' $output_file
fi
if [ "$(cat $output_file | $YQ '.spec.service | has("name")')" = "true" ]; then
$YQ -N -i '.spec.service.name |= "{{ include \"kueue.fullname\" . }}-" + .' $output_file
fi
if [ "$(cat $output_file | $YQ '.spec.service | has("namespace")')" = "true" ]; then
$YQ -N -i '.spec.service.namespace = "{{ .Release.Namespace }}"' $output_file
fi
if [ "$(cat $output_file | $YQ '.subjects.[] | has("namespace")')" = "true" ]; then
$YQ -N -i '.subjects.[].namespace = "{{ .Release.Namespace }}"' $output_file
fi
done
. I think we should omit this file.

@tenzen-y
Copy link
Member

Here is where we copy the visibility files for helm:

kueue/hack/update-helm.sh

Lines 99 to 120 in a68c958

# Add visibility files, replace names, namespaces in helm format
for output_file in ${DEST_VISIBILITY_DIR}/*.yaml; do
# The name of the v1alpha1.visibility.kueue.x-k8s.io APIService needs to remain unchanged.
if [ "$(cat $output_file | $YQ '.metadata | has("name")')" = "true" ] &&
[ "$(cat $output_file | $YQ '.metadata.name | (. == "v1alpha1*")')" = "false" ]; then
$YQ -N -i '.metadata.name |= "{{ include \"kueue.fullname\" . }}-" + .' $output_file
fi
# The namespace of the visibility-server-auth-reader rolebinding needs to remain unchanged.
if [ "$(cat $output_file | $YQ '.metadata | has("namespace")')" = "true" ] &&
[ "$(cat $output_file | $YQ '.metadata.namespace | (. == "kube-system")')" = "false" ]; then
$YQ -N -i '.metadata.namespace = "{{ .Release.Namespace }}"' $output_file
fi
if [ "$(cat $output_file | $YQ '.spec.service | has("name")')" = "true" ]; then
$YQ -N -i '.spec.service.name |= "{{ include \"kueue.fullname\" . }}-" + .' $output_file
fi
if [ "$(cat $output_file | $YQ '.spec.service | has("namespace")')" = "true" ]; then
$YQ -N -i '.spec.service.namespace = "{{ .Release.Namespace }}"' $output_file
fi
if [ "$(cat $output_file | $YQ '.subjects.[] | has("namespace")')" = "true" ]; then
$YQ -N -i '.subjects.[].namespace = "{{ .Release.Namespace }}"' $output_file
fi
done

. I think we should omit this file.

Yes, that's right.

@B1F030
Copy link
Member Author

B1F030 commented Dec 15, 2023

If we simply remove the kustomization.yaml, there will be new problems:

[root@test-1 charts]# kubectl get resourceflavor
E1215 18:17:09.715081   24954 memcache.go:255] couldn't get resource list for visibility.kueue.x-k8s.io/v1alpha1:
 the server is currently unable to handle the request
E1215 18:17:09.723755   24954 memcache.go:106] couldn't get resource list for visibility.kueue.x-k8s.io/v1alpha1:
 the server is currently unable to handle the request
No resources found

Is this another bug?

@mimowo
Copy link
Contributor

mimowo commented Dec 15, 2023

Are you asking about config/components/visibility/kustomization.yaml, or charts/kueue/templates/visibility/kustomization.yaml?

I think we should remove it from charts/kueue/templates/visibility/kustomization.yaml, and make sure it does not get copied during ./hack/update-helm.sh. Is there an issue in that case?

@B1F030
Copy link
Member Author

B1F030 commented Dec 15, 2023

Are you asking about config/components/visibility/kustomization.yaml, or charts/kueue/templates/visibility/kustomization.yaml?

charts/kueue/templates/visibility/kustomization.yaml

@mimowo
Copy link
Contributor

mimowo commented Dec 15, 2023

Oh, in that case I'm not sure, it requires investigation.

@tenzen-y
Copy link
Member

@B1F030 Which kubernetes version do you use? You're using v1.26.0 the same as kubectl version?

@B1F030
Copy link
Member Author

B1F030 commented Dec 16, 2023

@B1F030 Which kubernetes version do you use? You're using v1.26.0 the same as kubectl version?

yes

@tenzen-y
Copy link
Member

@B1F030 Recently, we fixed some permission errors related to the visibility server. So, could you verify if this error still happens?

@B1F030
Copy link
Member Author

B1F030 commented Dec 21, 2023

@B1F030 Recently, we fixed some permission errors related to the visibility server. So, could you verify if this error still happens?

I repulled the git repositry, and try the steps again:

git clone https://github.com/kubernetes-sigs/kueue.git
cd kueue/charts
helm install kueue kueue/ --namespace kueue-system

still the same problem...

[root@test-1 charts]# helm install kueue kueue/ --namespace kueue-system
Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest: 
error validating "": error validating data: [apiVersion not set, kind not set]

The file kueue/templates/visibility/kustomization.yaml still exists.

@tenzen-y
Copy link
Member

The file kueue/templates/visibility/kustomization.yaml still exists.

@B1F030 Yes, we haven't removed that file yet. I would like to ask if #1459 (comment) still happens.

@B1F030
Copy link
Member Author

B1F030 commented Dec 22, 2023

@B1F030 Yes, we haven't removed that file yet. I would like to ask if #1459 (comment) still happens.

Still the same. If I remove that kustomization.yaml and helm install, Errors still happen.

@tenzen-y
Copy link
Member

@B1F030 Yes, we haven't removed that file yet. I would like to ask if #1459 (comment) still happens.

Still the same. If I remove that kustomization.yaml and helm install, Errors still happen.

That error happens only for helm charts? Have you seen the same error when using all-in-one installation manifests (kubectl apply --server-side -k "github.com/kubernetes-sigs/kueue/config/default?ref=main") as well?

@B1F030
Copy link
Member Author

B1F030 commented Dec 22, 2023

That error happens only for helm charts? Have you seen the same error when using all-in-one installation manifests (kubectl apply --server-side -k "github.com/kubernetes-sigs/kueue/config/default?ref=main") as well?

both.

@tenzen-y
Copy link
Member

I could reproduce the above error in v1.26.3 cluster. Also, that error doesn't happen in v1.27.3 cluster.
If we disable VisibilityServer feature-gate in the v1.26 cluster, #1459 (comment) seems to happen.

@tenzen-y
Copy link
Member

@B1F030 Should we work only on #1459 (comment) error in this issue? And then, should we work on #1459 (comment) in a separate issue?

@B1F030
Copy link
Member Author

B1F030 commented Dec 22, 2023

@B1F030 Should we work only on #1459 (comment) error in this issue? And then, should we work on #1459 (comment) in a separate issue?

SGTM

@tenzen-y
Copy link
Member

tenzen-y commented Dec 22, 2023

@B1F030 So, can you create a PR to fix #1459 (comment) error?

@tenzen-y
Copy link
Member

@B1F030 So, can you create a PR to fix #1459 (comment) error?

I updated the above comment since I put the incorrect link :(

@B1F030
Copy link
Member Author

B1F030 commented Dec 22, 2023

@B1F030 So, can you create a PR to fix #1459 (comment) error?

This part could be fixed by simply remove the file kueue/templates/visibility/kustomization.yaml, right?

@tenzen-y
Copy link
Member

@B1F030 So, can you create a PR to fix #1459 (comment) error?

This part could be fixed by simply remove the file kueue/templates/visibility/kustomization.yaml, right?

Also, we should improve scripts to generate helm charts:

#1459 (comment)

@B1F030
Copy link
Member Author

B1F030 commented Dec 22, 2023

Also, we should improve scripts to generate helm charts:

#1459 (comment)

Got it. I'm glad to help.

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

Successfully merging a pull request may close this issue.

3 participants