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

applicationset controller crashes when rollingSync steps missing #18817

Closed
ashutosh16 opened this issue Jun 25, 2024 · 2 comments · Fixed by #20357
Closed

applicationset controller crashes when rollingSync steps missing #18817

ashutosh16 opened this issue Jun 25, 2024 · 2 comments · Fixed by #20357
Assignees
Labels
appset/progressive-syncs Issues related to the ApplicationSet progressive syncs feature. bug/enhancement bug/in-triage This issue needs further triage to be correctly classified bug Something isn't working component:application-sets Bulk application management related component:core Syncing, diffing, cluster state cache type:bug

Comments

@ashutosh16
Copy link
Contributor

In applicationset, when using the rollingSync strategy, the user accidentally removed the steps that caused the controller start crashing.

To Reproduce
https://argo-cd.readthedocs.io/en/stable/operator-manual/applicationset/Progressive-Syncs/#rollingsync

set the
strategy:
rollingSync: {}

Expected behavior
The controller errored out the applicationset when incorrect configuration issue but the controller shouldn't never crash due to misconfiguration.

Logs

application-set step list:","time":"2024-06-25T14:34:24Z"}
{"applicationset":{"Namespace":"dev-","Name":"*-application-set"},"level":"info","msg":"Application allowed to sync before maxUpdate?: map[]","time":"2024-06-25T14:34:24Z"}
panic: runtime error: index out of range [0] with length 0 [recovered]
panic: runtime error: index out of range [0] with length 0
goroutine 191 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
/go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:119 +0x1e5
panic({0x3ab57e0?, 0xc001fa1740?})
/usr/local/go/src/runtime/panic.go:914 +0x21f

 msg: Application allowed to sync before maxUpdate?: map[]
@ashutosh16 ashutosh16 added the bug Something isn't working label Jun 25, 2024
@ashutosh16 ashutosh16 self-assigned this Jun 25, 2024
@ashutosh16 ashutosh16 added the appset/progressive-syncs Issues related to the ApplicationSet progressive syncs feature. label Jun 25, 2024
@alexmt alexmt added bug/in-triage This issue needs further triage to be correctly classified component:application-sets Bulk application management related type:bug labels Jun 25, 2024
@christianh814
Copy link
Member

I agree that it shouldn't crash, but I do wonder what the behavior should be. I think the lack of configuration (i.e. rollingSync: {}) should make the ApplicationSet controller "fall back" as if progressive syncs weren't enabled.

@alexmt alexmt added bug/enhancement component:core Syncing, diffing, cluster state cache labels Jun 25, 2024
@agaudreault
Copy link
Member

The strategy is configured as

  strategy:
    type: RollingSync
    rollingSync: {}

Probably the behavior should be the same as one step that does not match any applications.

  strategy:
    type: RollingSync
    rollingSync:
      steps:
        - matchExpressions:
            - key: foo
              operator: In
              values:
                - a-value-that-does-not-match-anything

Not sure what happens when applications are not selected by any matchExpression, are they updated last? If so, it ends up to the same behavior @christianh814 described, while following the specified type: RollingSync codepath.

Unless there is an existing validation that all applications must be part of at least one step, then I think this behavior makes sense.

The doc should be updated to explain the behavior when an app is not selected in any steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appset/progressive-syncs Issues related to the ApplicationSet progressive syncs feature. bug/enhancement bug/in-triage This issue needs further triage to be correctly classified bug Something isn't working component:application-sets Bulk application management related component:core Syncing, diffing, cluster state cache type:bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants