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

RollingSync strategy for multiple ApplicationSets #14458

Open
alexymantha opened this issue Jul 11, 2023 · 10 comments · May be fixed by #15313
Open

RollingSync strategy for multiple ApplicationSets #14458

alexymantha opened this issue Jul 11, 2023 · 10 comments · May be fixed by #15313
Assignees
Labels
appset/progressive-syncs Issues related to the ApplicationSet progressive syncs feature. enhancement New feature or request

Comments

@alexymantha
Copy link
Member

alexymantha commented Jul 11, 2023

Summary

Add a way to define a RollingSync strategy in one place and refer to it in different ApplicationSets

Motivation

The RollingSync strategy for Progressive Sync requires the steps to be defined in the ApplicationSet. This leads to a lot of duplication because there are often common patterns for strategy, especially across a team's ApplicationSets. It would be a good QoL improvement if we could define the strategy in one place and refer to it in other ApplicationSets.

Proposal

A CRD to define the strategy:

apiVersion: argoproj.io/v1alpha1
kind: SyncStrategy
metadata:
  name: region-strategy
spec:
  type: RollingSync
  steps:
    - matchExpressions:
      - key: region 
        operator: In
        values:
          - apac
    - matchExpressions:
      - key: region
        operator: In
        values:
          - emea
        maxUpdate: 10%
    - matchExpressions:
      - key: region
        operator: In
        values:
          - na-west
        maxUpdate: 100%

In the ApplicationSet, refer to the resource instead:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: guestbook
spec:
  generators:
  - list:
      elements:
      - cluster: cluster1
        region: apac
      - cluster: cluster2
        region: emea
      - cluster: cluster3 
        region: na-west
  strategyRef:
	name: region-strategy
    namespace: argocd
@alexymantha alexymantha added the enhancement New feature or request label Jul 11, 2023
@alexymantha
Copy link
Member Author

Since this is specific to the Progressive Sync feature, can someone add the appset/progressive-sync label please?

@crenshaw-dev
Copy link
Member

Sounds cool. With 2.8 introducing applicationsets-in-any-namespace, we probably need something like stragegyRef: {name: xyz, namespace abc}, with the namespace assumed to be the appset controller's namespace if none is specified.

Should we move the type field into the CRD as well? Seems like those are tightly-coupled.

@crenshaw-dev crenshaw-dev added the appset/progressive-syncs Issues related to the ApplicationSet progressive syncs feature. label Jul 11, 2023
@alexymantha
Copy link
Member Author

You're right, forgot about the namespace. I edited the issue with your suggestion and changed the kind from RollingSyncStrategy to SyncStrategy it makes more sense if it also contains the type.

@wmgroot
Copy link
Contributor

wmgroot commented Aug 7, 2023

One alternative to needing the namespace for reference is to add namespace and cluster-scoped CRDs for re-usable sync strategies. Being able to reference namespace-scoped strategies in other namespaces could be useful, since they wouldn't require cluster-scoped permissions to apply. 🤔

In general I believe k8s leans toward the former approach for security best-practices with managing multi-tenant clusters.

@alexymantha
Copy link
Member Author

Just to make sure I'm understanding properly: you're suggesting something similar to ClusterRole/Role where we would have a cluster-scoped ClusterSyncStrategy and a namespaced SyncStrategy?

If so, I think it you're right, it would be more aligned with what k8s is already doing.

@wmgroot
Copy link
Contributor

wmgroot commented Aug 8, 2023

Yes, exactly. This suggestion was provided by the skyscanner folks that built the original concept that Progressive Sync is based on.

https://github.com/Skyscanner/applicationset-progressive-sync

@gordonswing
Copy link

Well-demanded feature for us!

@uri-lightblocks
Copy link

Looking forward to have it soon

@dijareoracle
Copy link

Would be great if this got implemented as we need this exactly in our setup

@agaudreault
Copy link
Member

I think this might be superseded in some part by #17755 and #17506, but I see how it would still be desirable to have a rolling sync with applications that have a large number of clusters.

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. enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants