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.

Sign up for GitHub

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

Jump to bottom

feat: Update EFA device plugin to use the EKS charts repo #26

Merged
merged 3 commits into from
Mar 16, 2024
Merged

feat: Update EFA device plugin to use the EKS charts repo #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
603c1ee
update efa plugin to use eks-charts repo
alanty Mar 12, 2024
1bef2cc
remove local copy of the efa device plugin chart
alanty Mar 12, 2024
a2d7bf3
re-add values from our local copy in the addon config
alanty Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions aws-efa-k8s-device-plugin.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
locals {
aws_efa_k8s_device_plugin_default_values = <<-EOT
tolerations:
- operator: Exists # DaemonSet is tolerant of any taints, regardless of the key or value of the taint.
- key: CriticalAddonsOnly
operator: Exists
EOT

aws_efa_k8s_device_plugin_merged_values_yaml = yamlencode(merge(
yamldecode(local.aws_efa_k8s_device_plugin_default_values),
try(yamldecode(var.aws_efa_k8s_device_plugin_helm_config.values[0]), {})
))
}


resource "helm_release" "aws_efa_k8s_device_plugin" {
count = var.enable_aws_efa_k8s_device_plugin ? 1 : 0

name = try(var.aws_efa_k8s_device_plugin_helm_config["name"], "aws-efa-k8s-device-plugin")
repository = try(var.aws_efa_k8s_device_plugin_helm_config["repository"], null)
chart = try(var.aws_efa_k8s_device_plugin_helm_config["chart"], "${path.module}/helm-charts/aws-efa-k8s-device-plugin")
version = try(var.aws_efa_k8s_device_plugin_helm_config["version"], "0.1.0")
repository = try(var.aws_efa_k8s_device_plugin_helm_config["repository"], "https://aws.github.io/eks-charts")
Copy link
Contributor

Choose a reason for hiding this comment

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

I like to use the upstream repo instead of our local one, have you validated that all the configs that we are using by default in the values.yaml file are used into the default chart values? I'm asking that because people could be relying in the default values. @vara-bonthu need you on that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats a good call, I haven't checked what those defaults are in the local copy. I'll double check those and make sure we aren't breaking anything.

We may have added tolerations that the upstream doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at a diff of the values:

  • Upstream Drops beta.kubernetes.io/instance-type from the label selector (Starting in v1.17, this label is deprecated in favor of node.kubernetes.io/instance-type.)
  • Upstream moves version from v0.3.3 to v0.4.4
  • upstream has added a requests block to the values:
resources:
   requests:
     cpu:     10m
     memory:  20Mi
  • our Local copy has a toleration to run everywhere, upstream values has an empty toleration config.
tolerations:
  - operator: Exists    # DaemonSet is tolerant of any taints, regardless of the key or value of the taint.
  - key: CriticalAddonsOnly
    operator: Exists

I'll add that toleration to our addon config but I think the other changes are ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to follow the way the Nvidia plugin configures those values

chart = try(var.aws_efa_k8s_device_plugin_helm_config["chart"], "aws-efa-k8s-device-plugin")
version = try(var.aws_efa_k8s_device_plugin_helm_config["version"], "v0.4.4")
timeout = try(var.aws_efa_k8s_device_plugin_helm_config["timeout"], 300)
values = try(var.aws_efa_k8s_device_plugin_helm_config["values"], null)
values = [local.aws_efa_k8s_device_plugin_merged_values_yaml]
create_namespace = try(var.aws_efa_k8s_device_plugin_helm_config["create_namespace"], false)
namespace = try(var.aws_efa_k8s_device_plugin_helm_config["namespace"], "kube-system")
lint = try(var.aws_efa_k8s_device_plugin_helm_config["lint"], false)
Expand Down
23 changes: 0 additions & 23 deletions helm-charts/aws-efa-k8s-device-plugin/.helmignore
View file
Open in desktop

This file was deleted.

6 changes: 0 additions & 6 deletions helm-charts/aws-efa-k8s-device-plugin/Chart.yaml
View file
Open in desktop

This file was deleted.

1 change: 0 additions & 1 deletion helm-charts/aws-efa-k8s-device-plugin/templates/NOTES.txt
View file
Open in desktop

This file was deleted.

62 changes: 0 additions & 62 deletions helm-charts/aws-efa-k8s-device-plugin/templates/_helpers.tpl
View file
Open in desktop

This file was deleted.

73 changes: 0 additions & 73 deletions helm-charts/aws-efa-k8s-device-plugin/templates/daemonset.yaml
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions helm-charts/aws-efa-k8s-device-plugin/templates/test/test-efa.yaml
View file
Open in desktop

This file was deleted.

98 changes: 0 additions & 98 deletions helm-charts/aws-efa-k8s-device-plugin/values.yaml
View file
Open in desktop

This file was deleted.

Loading