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

Progressive delivery of applications using ApplicationSets #845

Merged
merged 4 commits into from
Jan 26, 2023
Merged

Progressive delivery of applications using ApplicationSets #845

merged 4 commits into from
Jan 26, 2023

Conversation

iam-veeramalla
Copy link
Collaborator

What type of PR is this?

/kind enhancement

What does this PR do / why we need it:
This is an experimental, alpha-quality feature that allows you to control the order in which the ApplicationSet controller will create or update the Applications owned by an ApplicationSet resource. It may be removed in future releases or modified in backwards-incompatible ways.

The feature only interacts with the health of managed Applications. It is NOT intended to support direct integrations with other Rollout controllers (such as the native ReplicaSet controller or Argo Rollouts).
Progressive Rollouts watch for the managed Application resources to become "Healthy" before proceeding to the next stage.

Enabling Progressive Rollouts
As an experimental feature, progressive rollouts must be explicitly enabled, in one of the below 3 ways.

  1. Pass --enable-progressive-rollouts to the ApplicationSet controller args.
    To achieve this using the operator, Users can add the above argument in the Argo CD CR
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: example-argocd
  labels:
    example: basic
spec:
  applicationSet:
    extraCommandArgs:
     - --enable-progressive-rollouts
  1. Set ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_PROGRESSIVE_ROLLOUTS=true in the ApplicationSet controller environment variables in the below mentioned way.
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: example-argocd
  labels:
    example: basic
spec:
  applicationSet:
    env:
     - name: ARGOCD_APPLICATIONSET_CONTROLLER_ENABLE_PROGRESSIVE_ROLLOUTS
       value: "true"
  1. Set applicationsetcontroller.enable.progressive.rollouts: true in the ArgoCD ConfigMap.
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
  name: example-argocd
  labels:
    example: basic
spec:
   extraConfig:
     applicationsetcontroller.enable.progressive.rollouts: true

Have you updated the necessary documentation?

  • Documentation update is required by this PR.
  • Documentation has been updated.

How to test changes / Special notes to the reviewer:

  1. Run the operator manually using make install run
  2. Once the operator is running, submit any one of the 3 Argo CD CRs mentioned above.
  3. The expected behavior for each of the example is provided above.

@jaideepr97
Copy link
Collaborator

Thanks @iam-veeramalla

Do we need to support 3 different ways of enabling the same feature?

If upstream specifies env var or configmap entry I think setting that field in the cr should maybe do that under the hood rather than exposing so many ways to the user

I'm also wondering how we would handle conflicts if places have contradicting config for this

@iam-veeramalla
Copy link
Collaborator Author

ting that fiel

Hi @jaideepr97 , the implementation is same as upstream.

Upstream also supports all the three ways and three are completely unique

@jaideepr97
Copy link
Collaborator

jaideepr97 commented Jan 24, 2023

@iam-veeramalla I understand that
I'm saying as an operator I think we can try and make things more user friendly
what if we just had a toggle under .spec.applicationSets.enableProgressiveRollout that set all these fields to true under the hood?

Unless we don't want to make it a first class field in the CRD because it's an alpha feature and maybe removed later on?

@iam-veeramalla
Copy link
Collaborator Author

@iam-veeramalla I understand that I'm saying as an operator I think we can try and make things more user friendly what if we just had a toggle under .spec.applicationSets.enableProgressiveRollout that set all these fields to true under the hood?

@jaideepr97 Thanks for the suggestion.
The idea here is to implement the extraCommandArgs and env support so that in the future, we don't need to add more fields to the Argo CD CR w.r.t command arguments or env variable for ApplicationSets.
It might look like we are adding too many place holders for this PR but we are actually not. The applicationset controller lacks these options which are already supported by controller, server, repo-server and other components.

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
@jaideepr97
Copy link
Collaborator

@iam-veeramalla could we add unit tests as well?

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
@iam-veeramalla
Copy link
Collaborator Author

@jaideepr97 added new tests and removed an unintentional change. PTAL, Thanks :)

Comment on lines +102 to +118
### Add Command Arguments to ApplicationSets Controller

Below example shows how a user can add command arguments to the ApplicationSet controller.

``` yaml
apiVersion: argoproj.io/v1alpha1
kind: ArgoCD
metadata:
name: example-argocd
labels:
example: applicationset
spec:
applicationSet:
extraCommandArgs:
- --foo
- bar
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

@iam-veeramalla the docs only talk about being able to add extra args to appset deployment
do we also want to specifically mention that they can pass --enable-progressive-rollouts to enable progressive rollouts in appsets here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should keep it very generic. In the release notes text when we describe the feature, we can add a point for this.

@jaideepr97
Copy link
Collaborator

LGTM thanks @iam-veeramalla

@iam-veeramalla iam-veeramalla merged commit 2371eeb into argoproj-labs:master Jan 26, 2023
ciiay pushed a commit to ciiay/argocd-operator that referenced this pull request Mar 21, 2023
…labs#845)

* appSet progressive delivery for applications

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

* fix: update comments

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

* fix: add docs

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

* feat: add tests

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>

Signed-off-by: iam-veeramalla <abhishek.veeramalla@gmail.com>
Signed-off-by: Yi Cai <yicai@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants