Skip to content

fix terminal addons #166

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix terminal addons #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f3af5b
fix terminal addons
rushmash91 Aug 4, 2025
356d989
Merge branch 'main' into feature/addon-retry-logic-for-failed-states
rushmash91 Aug 4, 2025
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: 3 additions & 3 deletions apis/v1alpha1/ack-generate-metadata.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
ack_generate_info:
build_date: "2025-07-30T22:50:35Z"
build_hash: b2dc0f44e0b08f041de14c3944a5cc005ba97c8f
build_date: "2025-08-06T18:46:04Z"
build_hash: b4fbf4e427daaef74ed873aac01e4a9ca68fb479
go_version: go1.24.5
version: v0.50.0
version: v0.50.0-3-gb4fbf4e
api_directory_checksum: 2b5e65a1d5f0a032d51209f925b714aff4b6dc96
api_version: v1alpha1
aws_sdk_go_version: v1.37.0
Expand Down
4 changes: 2 additions & 2 deletions helm/templates/caches-role-binding.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: {{ include "ack-eks-controller.app.fullname" . }}-namespace-caches
name: {{ include "ack-eks-controller.app.fullname" . }}-namespaces-cache
labels:
app.kubernetes.io/name: {{ include "ack-eks-controller.app.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
Expand All @@ -12,7 +12,7 @@ metadata:
roleRef:
kind: ClusterRole
apiGroup: rbac.authorization.k8s.io
name: {{ include "ack-eks-controller.app.fullname" . }}-namespace-caches
name: {{ include "ack-eks-controller.app.fullname" . }}-namespaces-cache
subjects:
- kind: ServiceAccount
name: {{ include "ack-eks-controller.service-account.name" . }}
Expand Down
2 changes: 1 addition & 1 deletion helm/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ spec:
secretName: {{ .Values.aws.credentials.secretName }}
{{- end }}
{{- if .Values.deployment.extraVolumes }}
{{ toYaml .Values.deployment.extraVolumes | indent 8 }}
{{- toYaml .Values.deployment.extraVolumes | nindent 8 }}
{{- end }}
{{- end }}
{{- with .Values.deployment.strategy }}
Expand Down
12 changes: 10 additions & 2 deletions pkg/resource/addon/hooks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ var (
var (
// TerminalStatuses defines the list of statuses that are terminal for an addon
TerminalStatuses = []string{
StatusCreateFailed,
StatusUpdateFailed,
StatusDeleteFailed,
// Still not sure if we should consider DEGRADED as terminal
// StatusDegraded,
Expand Down Expand Up @@ -101,6 +99,16 @@ func addonHasTerminalStatus(r *resource) bool {
return false
}

// addonInFailedState returns true if the supplied addon is in a failed state
// that requires retry (CREATE_FAILED or UPDATE_FAILED)
func addonInFailedState(r *resource) bool {
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
func addonInFailedState(r *resource) bool {
func addonHasFailed(r *resource) bool {

if r.ko.Status.Status == nil {
return false
}
cs := *r.ko.Status.Status
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: cs isn't a very descriptive name. Maybe something like addonStatus?

return cs == StatusCreateFailed || cs == StatusUpdateFailed
}

// requeueWaitUntilCanModify returns a `ackrequeue.RequeueNeededAfter` struct
// explaining the addon cannot be modified until it reaches an active status.
func requeueWaitUntilCanModify(r *resource) *ackrequeue.RequeueNeededAfter {
Expand Down
16 changes: 14 additions & 2 deletions pkg/resource/addon/sdk.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion pkg/resource/nodegroup/sdk.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 18 additions & 6 deletions templates/hooks/addons/sdk_update_pre_build_request.go.tpl
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

We should probably do the inFailedState check in delta..if not, sdkUpdate will never be triggered.
To trigger sdkUpdate there needs to be a diff in "spec".

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
ackcondition.SetSynced(latest, corev1.ConditionFalse, &msg, nil)
return latest, requeueWaitWhileDeleting
}
if !addonActive(latest) {

// Check if addon is in a failed state that requires retry
inFailedState := addonInFailedState(latest)

if !addonActive(latest) && !inFailedState {
msg := "Addon is in '" + *latest.ko.Status.Status + "' status"
ackcondition.SetSynced(latest, corev1.ConditionFalse, &msg, nil)
if addonHasTerminalStatus(latest) {
Expand All @@ -13,16 +17,24 @@
return latest, requeueWaitUntilCanModify(latest)
}

// If addon is in failed state, we need to force an update regardless of delta
if inFailedState {
msg := "Addon is in '" + *latest.ko.Status.Status + "' status, attempting recovery"
ackcondition.SetSynced(latest, corev1.ConditionFalse, &msg, nil)
}
Comment on lines +20 to +24
Copy link
Member

Choose a reason for hiding this comment

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

we can add this one in sdkFind as well


if delta.DifferentAt("Spec.Tags") {
err := syncTags(
ctx, rm.sdkapi, rm.metrics,
string(*desired.ko.Status.ACKResourceMetadata.ARN),
ctx, rm.sdkapi, rm.metrics,
string(*desired.ko.Status.ACKResourceMetadata.ARN),
aws.ToStringMap(desired.ko.Spec.Tags), aws.ToStringMap(latest.ko.Spec.Tags),
)
if err != nil {
return nil, err
}
}
if !delta.DifferentExcept("Spec.Tags"){
return desired, nil
}
// If addon is in failed state, always attempt update to recover
// Otherwise, check if there are differences to update
if !inFailedState && !delta.DifferentExcept("Spec.Tags") {
Copy link
Member

Choose a reason for hiding this comment

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

we probably won't need inFailedState check here if we add a new diff in delta

return desired, nil
}