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

support accelerated networking #84

Merged
merged 2 commits into from
Aug 21, 2024
Merged

Conversation

davidkarlsen
Copy link
Contributor

@davidkarlsen davidkarlsen commented Aug 9, 2024

@paulczar PTAL

@davidkarlsen
Copy link
Contributor Author

or perhaps @shane-snyder or @diana-sari ?

@diana-sari
Copy link
Contributor

hello @davidkarlsen thanks for the PR!
i ran this locally and encountered this error, any thoughts on the taints formatting there?

➜  helm-charts git:(feature/accel-nw) helm upgrade --install -n openshift-machine-api \
  infra mobb/aro-machinesets
Release "infra" does not exist. Installing it now.
Error: 3 errors occurred:
	* admission webhook "default.machineset.machine.openshift.io" denied the request: json: cannot unmarshal object into Go struct field MachineSpec.spec.template.spec.taints of type []v1.Taint
	* admission webhook "default.machineset.machine.openshift.io" denied the request: json: cannot unmarshal object into Go struct field MachineSpec.spec.template.spec.taints of type []v1.Taint
	* admission webhook "default.machineset.machine.openshift.io" denied the request: json: cannot unmarshal object into Go struct field MachineSpec.spec.template.spec.taints of type []v1.Taint

@davidkarlsen
Copy link
Contributor Author

@diana-sari that was probably the case before this PR as well?
What version is your cluster?

@davidkarlsen
Copy link
Contributor Author

see what happens if you change this default:

machineTaints: {}

to:

machineTaints: []

@diana-sari
Copy link
Contributor

yeah i actually am seeing similar error from main branch, this is a fresh git clone and following steps from the readme:

➜  helm-charts git:(main) helm upgrade --install -n openshift-machine-api \
  infra mobb/aro-machinesets
Error: UPGRADE FAILED: failed to create resource: admission webhook "default.machineset.machine.openshift.io" denied the request: json: cannot unmarshal object into Go struct field MachineSpec.spec.template.spec.taints of type []v1.Taint

@davidkarlsen
Copy link
Contributor Author

try taints as an empty list.
what version is your cluster?
perhaps this PR can be merged anyways as it is not the one introducing the error.

@diana-sari
Copy link
Contributor

while this PR might not be related to the error, we do need to investigate this further so please bear with us, and actually i'll be out this week so i'll leave it to Paul for time being (and i already DM him so hopefully he'll take a look at this when he gets the chance), appreciate your patience in advance!

@davidkarlsen
Copy link
Contributor Author

I can reproduce your error:

helm upgrade --install -n openshift-machine-api test .
Release "test" does not exist. Installing it now.
Error: 3 errors occurred:
        * admission webhook "default.machineset.machine.openshift.io" denied the request: json: cannot unmarshal object into Go struct field MachineSpec.spec.template.spec.taints of type []v1.Taint
        * admission webhook "default.machineset.machine.openshift.io" denied the request: json: cannot unmarshal object into Go struct field MachineSpec.spec.template.spec.taints of type []v1.Taint
        * admission webhook "default.machineset.machine.openshift.io" denied the request: json: cannot unmarshal object into Go struct field MachineSpec.spec.template.spec.taints of type []v1.Taint


 aro-machinesets  oc version
Client Version: 4.16.5
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: 4.14.33
Kubernetes Version: v1.27.15+6147456

and if I change the default values as I suggested previously, it will pass:

helm upgrade --install -n openshift-machine-api test .
Release "test" has been upgraded. Happy Helming!
NAME: test
LAST DEPLOYED: Wed Aug 14 15:22:50 2024
NAMESPACE: openshift-machine-api
STATUS: deployed
REVISION: 2
TEST SUITE: None
 aro-machinesets  git diff

diff --git a/charts/aro-machinesets/values.yaml b/charts/aro-machinesets/values.yaml
index c6436f8..d672ffc 100644
--- a/charts/aro-machinesets/values.yaml
+++ b/charts/aro-machinesets/values.yaml
@@ -16,7 +16,7 @@ machineLabels: {}
   # uncomment this to have infra-nodes
   # node-role.kubernetes.io/infra: ""

-machineTaints: {}
+machineTaints: []
   # uncomment this to have dedicated infra-nodes with taints
   # - key: node-role.kubernetes.io/infra
   #  effect: NoSchedule

@davidkarlsen
Copy link
Contributor Author

@diana-sari old problem fixed in this PR. CC @paulczar

@davidkarlsen
Copy link
Contributor Author

@diana-sari ? @paulczar ? Are there any other maintainers? Is this project still actively maintained?

@paulczar
Copy link
Contributor

sorry @davidkarlsen we've got a mix of vacations and illness on the maintainer group. I'm looking at this today.

@paulczar paulczar merged commit 5d2ed62 into rh-mobb:main Aug 21, 2024
1 check passed
@davidkarlsen davidkarlsen deleted the feature/accel-nw branch August 21, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants