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

feat(controller): Enhanced TTL controller scalability #4736

Merged
merged 4 commits into from Dec 15, 2020

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Dec 14, 2020

Signed-off-by: Alex Collins alex_collins@intuit.com

Checklist:

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@@ -10,8 +10,7 @@ You cannot horizontally scale the controller.

You can scale the controller vertically:

- If you have workflows with many steps, increase `--pod-workers`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't recommend changing --pod-workers anymore. It has no impact in any testing I've done.

@@ -129,9 +130,6 @@ func (c *Controller) enqueueWF(obj interface{}) {
log.Warnf("'%v' is not an unstructured", obj)
return
}
if un.GetDeletionTimestamp() != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

redundant now it is checked by FilterFunc on line 52

@@ -187,9 +192,14 @@ func (c *Controller) deleteWorkflow(key string) error {

err = c.wfclientset.ArgoprojV1alpha1().Workflows(wf.Namespace).Delete(wf.Name, &metav1.DeleteOptions{PropagationPolicy: commonutil.GetDeletePropagation()})
if err != nil {
return err
if apierr.IsNotFound(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've not seen this error apart from initial testing - prevented by line 155 I think, but a good bonus check

Signed-off-by: Alex Collins <alex_collins@intuit.com>
@alexec alexec marked this pull request as ready for review December 14, 2020 19:24
@alexec alexec changed the title feat(controller): Improve TTL controller scalability. feat(controller): Enhanced TTL controller scalability Dec 15, 2020
},
Handler: cache.ResourceEventHandlerFuncs{
AddFunc: controller.enqueueWF,
UpdateFunc: func(old, new interface{}) {
controller.enqueueWF(new)
},
DeleteFunc: controller.enqueueWF,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is invoked after the WF is deleted - so it is not needed - removing reduces the number of API requests by 1

test/e2e/functional_test.go Outdated Show resolved Hide resolved
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@@ -100,6 +101,7 @@ func NewRootCommand() *cobra.Command {
command.Flags().StringVar(&logLevel, "loglevel", "info", "Set the logging level. One of: debug|info|warn|error")
command.Flags().IntVar(&glogLevel, "gloglevel", 0, "Set the glog logging level")
command.Flags().IntVar(&workflowWorkers, "workflow-workers", 32, "Number of workflow workers")
command.Flags().IntVar(&workflowTTLWorkers, "workflow-ttl-workers", 4, "Number of workflow TTL workers")
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding this. This was my proposal during the prod incident. But We don't want to make too many changes

@alexec alexec merged commit 1a7ed73 into argoproj:master Dec 15, 2020
@alexec alexec deleted the ttl-wf branch December 15, 2020 03:24
@simster7 simster7 mentioned this pull request Dec 15, 2020
22 tasks
simster7 pushed a commit that referenced this pull request Jan 4, 2021
Signed-off-by: Alex Collins <alex_collins@intuit.com>
@agilgur5 agilgur5 added the area/controller Controller issues, panics label Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controller Controller issues, panics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants