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

Fix InAccel FPGA Operator #265

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Fix InAccel FPGA Operator #265

merged 2 commits into from
Dec 3, 2021

Conversation

eliaskoromilas
Copy link
Contributor

@eliaskoromilas eliaskoromilas commented Nov 26, 2021

We encountered a problem with the last InAccel FPGA Operator downstream release. The issue is related to the way rancher/charts-build-scripts handles chart dependencies and the produced downstream chart.

More specifically make prepare replaces all dependency.repository URLs with local paths but also removes the dependency.version fields. For example in our case:

<   repository: https://kubernetes-sigs.github.io/node-feature-discovery/charts
<   version: 0.9.0
---
>   repository: file://./charts/node-feature-discovery

But it seems that Helm ignores dependencies without a version field and instead just deploys everything under charts directory as a subchart (and not as a dependency), which means two things:

  1. dependency.alias field is not taken into account, which is problematic if configuration is passed through values.yaml
  2. dependency.condition is also ignored, which makes it impossible to disable the dependency (if needed, in a use-case)

As a workaround in this fix we restore the dependency.version field through the Chart.yaml.patch, but I believe that this should eventually be fixed in the charts-build-scripts. Do you want me to open a related issue there, too?

To replicate the issue run the following command and you will notice that every fpga-discovery related template appears although we disabled the dependency.

helm template --set fpga-discovery.enabled=false https://github.com/rancher/partner-charts/blob/main-source/assets/fpga-operator/fpga-operator-2.6.101.tgz?raw=true

Then run the same command but this time using the new chart package that just has the dependency.version field restored:

helm template --set fpga-discovery.enabled=false https://github.com/eliaskoromilas/partner-charts/blob/fpga-operator/assets/fpga-operator/fpga-operator-2.6.102.tgz?raw=true

Now the condition works but also the fpga-discovery configuration is applied by Helm.

Signed-off-by: Elias Koromilas <elias.koromilas@gmail.com>
Signed-off-by: Elias Koromilas <elias.koromilas@gmail.com>
Copy link
Contributor

@alex-isv alex-isv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deployment verified on RKE

image

@samuelattwood
Copy link
Contributor

@eliaskoromilas Thanks for the detailed report. I am able to replicate the issue and agree that seems like that should be fixed in the build scripts themselves. I see you opened rancher/charts-build-scripts#68 so hopefully we will have this addressed soon.

Fixing it in the patch file is a good workaround for the time being.

@samuelattwood samuelattwood merged commit 02de929 into rancher:main-source Dec 3, 2021
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