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

Add compatibility workaround for A1 instance family #1805

Merged
merged 1 commit into from
Oct 27, 2023
Merged

Add compatibility workaround for A1 instance family #1805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ ARG TARGETARCH
ARG VERSION
RUN OS=$TARGETOS ARCH=$TARGETARCH make $TARGETOS/$TARGETARCH

FROM public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-csi-ebs:latest-al23 AS linux-amazon
FROM public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-csi-ebs:latest-al23 AS linux-al2023
COPY --from=builder /go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/bin/aws-ebs-csi-driver /bin/aws-ebs-csi-driver
ENTRYPOINT ["/bin/aws-ebs-csi-driver"]

FROM public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-csi-ebs:latest-al2 AS linux-al2
COPY --from=builder /go/src/github.com/kubernetes-sigs/aws-ebs-csi-driver/bin/aws-ebs-csi-driver /bin/aws-ebs-csi-driver
ENTRYPOINT ["/bin/aws-ebs-csi-driver"]

Expand Down
6 changes: 3 additions & 3 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ OUTPUT_TYPE?=docker

OS?=linux
ARCH?=amd64
OSVERSION?=amazon
OSVERSION?=al2023

ALL_OS?=linux windows
ALL_ARCH_linux?=amd64 arm64
ALL_OSVERSION_linux?=amazon
ALL_OSVERSION_linux?=al2023
ALL_OS_ARCH_OSVERSION_linux=$(foreach arch, $(ALL_ARCH_linux), $(foreach osversion, ${ALL_OSVERSION_linux}, linux-$(arch)-${osversion}))

ALL_ARCH_windows?=amd64
Expand Down Expand Up @@ -86,7 +86,7 @@ create-manifest: all-image-registry
.PHONY: all-image-docker
all-image-docker: $(addprefix sub-image-docker-,$(ALL_OS_ARCH_OSVERSION_linux))
.PHONY: all-image-registry
all-image-registry: $(addprefix sub-image-registry-,$(ALL_OS_ARCH_OSVERSION))
all-image-registry: sub-image-registry-linux-arm64-al2 $(addprefix sub-image-registry-,$(ALL_OS_ARCH_OSVERSION))

sub-image-%:
$(MAKE) OUTPUT_TYPE=$(call word-hyphen,$*,1) OS=$(call word-hyphen,$*,2) ARCH=$(call word-hyphen,$*,3) OSVERSION=$(call word-hyphen,$*,4) image
Expand Down
1 change: 1 addition & 0 deletions charts/aws-ebs-csi-driver/templates/_node-windows.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{{- define "node-windows" }}
{{- if .Values.node.enableWindows }}
---
kind: DaemonSet
apiVersion: apps/v1
metadata:
Expand Down
1 change: 1 addition & 0 deletions charts/aws-ebs-csi-driver/templates/_node.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{{- define "node" }}
{{- if or (eq (default true .Values.node.enableLinux) true) }}
---
kind: DaemonSet
apiVersion: apps/v1
metadata:
Expand Down
1 change: 0 additions & 1 deletion charts/aws-ebs-csi-driver/templates/node-windows.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,5 @@
"node" (deepCopy $.Values.node | mustMerge $values)
)
}}
---
{{- include "node-windows" (deepCopy $ | mustMerge $args) -}}
{{- end }}
34 changes: 33 additions & 1 deletion charts/aws-ebs-csi-driver/templates/node.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,38 @@
"node" (deepCopy $.Values.node | mustMerge $values)
)
}}
---
{{- include "node" (deepCopy $ | mustMerge $args) -}}
{{- end }}
{{- if .Values.a1CompatibilityDaemonSet }}
{{$args := dict
"NodeName" "ebs-csi-node-a1compat"
"Values" (dict
"image" (dict
"tag" (printf "%s-a1compat" (default (printf "v%s" .Chart.AppVersion) (.Values.image.tag | toString)))
)
"node" (dict
"affinity" (dict
"nodeAffinity" (dict
"requiredDuringSchedulingIgnoredDuringExecution" (dict
"nodeSelectorTerms" (list
(dict "matchExpressions" (list
(dict
"key" "eks.amazonaws.com/compute-type"
"operator" "NotIn"
"values" (list "fargate")
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this Fargate-specific check necessary?

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'm not sure if it's absolutely necessary here, in order to make the minimal change necessary I pulled it from our defaults: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/charts/aws-ebs-csi-driver/values.yaml#L319-L327
(so the resulting change is the default + the a1 restriction, rather than the default replaced by the a1 restriction)

Even if it's not actually necessary, it definitely won't hurt, and considering we don't control what EKS fargate does it would be bad if in some future they started vending nodes with an a1 "instance type" and we tried to run on them.

)
(dict
"key" "node.kubernetes.io/instance-type"
"operator" "In"
"values" (list "a1.medium" "a1.large" "a1.xlarge" "a1.2xlarge" "a1.4xlarge")
)
))
)
)
)
)
)
)
}}
{{- include "node" (deepCopy $ | mustMerge $args) -}}
{{- end }}
11 changes: 11 additions & 0 deletions charts/aws-ebs-csi-driver/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,14 @@ node:
operator: NotIn
values:
- fargate
- key: node.kubernetes.io/instance-type
operator: NotIn
values:
- a1.medium
- a1.large
- a1.xlarge
- a1.2xlarge
- a1.4xlarge
nodeSelector: {}
podAnnotations: {}
podLabels: {}
Expand Down Expand Up @@ -391,6 +399,9 @@ additionalDaemonSets:
# node.kubernetes.io/instance-type: c5.large
# volumeAttachLimit: 15

# Enable compatibility for the A1 instance family via use of an AL2-based image in a separate DaemonSet
# a1CompatibilityDaemonSet: true

storageClasses: []
# Add StorageClass resources like:
# - name: ebs-sc
Expand Down
8 changes: 8 additions & 0 deletions deploy/kubernetes/base/node.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ spec:
operator: NotIn
values:
- fargate
- key: node.kubernetes.io/instance-type
operator: NotIn
values:
- a1.medium
- a1.large
- a1.xlarge
- a1.2xlarge
- a1.4xlarge
nodeSelector:
kubernetes.io/os: linux
serviceAccountName: ebs-csi-node-sa
Expand Down
4 changes: 2 additions & 2 deletions hack/e2e/ecr.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ function ecr_build_and_push() {
docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
make all-push
else
IMAGE=${IMAGE_NAME} TAG=${IMAGE_TAG} OS=linux ARCH=amd64 OSVERSION=amazon make image
docker tag "${IMAGE_NAME}":"${IMAGE_TAG}"-linux-amd64-amazon "${IMAGE_NAME}":"${IMAGE_TAG}"
IMAGE=${IMAGE_NAME} TAG=${IMAGE_TAG} OS=linux ARCH=amd64 OSVERSION=al2023 make image
docker tag "${IMAGE_NAME}":"${IMAGE_TAG}"-linux-amd64-al2023 "${IMAGE_NAME}":"${IMAGE_TAG}"
Comment on lines +30 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

(Out of scope for this PR, but it would be better if this extracted the information from the resulting image, instead of passing it in. Having it hardcoded here and in the Makefile is tech debt that will bite us eventually.)

Copy link
Contributor

@AndrewSirenko AndrewSirenko Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Adding to our 'optimize makefile & run.sh task' backlog task.

docker push "${IMAGE_NAME}":"${IMAGE_TAG}"
fi
fi
Expand Down
Loading