feat(argo-rollouts): Add configurable deployment strategy for Argo Rollouts controller #2412
Conversation
nedal87
changed the title
| {{- if eq .Values.controller.deploymentStrategy.type "RollingUpdate" }} | ||
| strategy: | ||
| {{- toYaml .Values.controller.deploymentStrategy | nindent 4 }} | ||
| {{- else }} | ||
| strategy: | ||
| type: Recreate | ||
| {{- end }} |
There was a problem hiding this comment.
Hi @nedal87 , thank you for your PR.
Did you investigate why this is hardcoded as Recreate originally ? I am not sure about this case, but some resources are not designed for RollingUpdate .
If the upstream's original kustomize is RollingUpdate , then it sounds safe to provide other choices but upstream's manifest is Recreate
Can you please check if it's safe to provide other strategy options in upstream?
There was a problem hiding this comment.
Not sure for now but at the point of March 2019, it should be Recreate . 👀
argoproj/argo-rollouts#38
There was a problem hiding this comment.
Hi @yu-croco Thank you for looking into this PR.
I understand that the initial decision to use the "Recreate" strategy was based on the assumption that controllers operate as the sole modifiers of resources as mentioned in the issue argoproj/argo-rollouts/issues/38
In my investigation, I didn't find specific functionality issues associated with using the "RollingUpdate" strategy. I tested that locally and could see that the deployment behave normally when restarting, updating or scaling the Argo rollout controller deployment. Are there any specific concerns regarding the usage of RollingUpdate deployment strategy?
There was a problem hiding this comment.
Are there any specific concerns regarding the usage of RollingUpdate deployment strategy?
Aside from the ISSUE above, I don't have any concerns.
Since we follow upstream's manifest and not familiar with the detail of feature, can you please check in upstream for in case?
There was a problem hiding this comment.
Thank you for your feedback, I will reach out to the upstream to verify and proceed from there.
There was a problem hiding this comment.
Thank you! When we are sure about this, LGTM 🚀
There was a problem hiding this comment.
Hi @nedal87 ,
I investigated and revealed that controller can be RollingUpdate surely, so I made PR to upstream (it's already merged into master).
argoproj/argo-rollouts#3334
When upstream releases the PR as specific version, let's change the default type as RollingUpdate in this PR.
There was a problem hiding this comment.
Hi @yu-croco that's great news. Thanks a lot! Updating the PR to have the RollingUpdate by default.
…louts controller Signed-off-by: Nedal Eskaif <[email protected]>
nedal87
force-pushed
the
feat/neskaif/add-custom-spec.strategy-for-deployments
branch
from
02f5fc1
to
89f989a
Compare
jmeridth
added
the
awaiting-upstream
|
Hello folks!. Thanks for the contribution. Any idea when this change will be published? |
Hi @CarlosLanderas , we can merge when upstream merge argoproj/argo-rollouts#3334 into specific new version because we follow upstresm's manifest. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
yu-croco
added
the
on-hold
|
This is resolved in #2763 . |
Added configurable deployment strategy for the Argo Rollouts controller. Users can now specify either "Recreate" or "RollingUpdate" as the deployment strategy in the values.yaml file. This enhancement provides users with flexibility in choosing the deployment strategy that best suits their needs.
Checklist: