-
Notifications
You must be signed in to change notification settings - Fork 152
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
ActionSet handling perf tweaks #1650
Conversation
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
3123cad
to
4d54af2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I will let someone else have a look as well. thank you, good find 💯
cc @ihcsim
4d54af2
to
4dea2e0
Compare
@ihcsim Could you please take a look at these changes too? |
093a47c
to
6773589
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ed-shilo 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this. I left some questions and comments. If needed, we can address them in a follow-up PR.
if err := c.onAddActionSet(v); err != nil { | ||
log.Error().WithError(err).Print("Callback onAddActionSet() failed") | ||
} | ||
t, ctx := c.LoadOrStoreTomb(context.Background(), v.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know these are legacy code, but I am not sure what we gain by storing the tombs. Not like there is much savings from re-use, since the actionsets will have different names. FWIW, it also doesn't look like it's expensive to create a tomb
anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand, originally the tomb was used for graceful action set deletion. In fact each submitted goroutine doesn't listen for tomb signals, but I suppose it's better to leave them wrapped with the tomb.
} | ||
|
||
func (c *Controller) onAddActionSet(ctx context.Context, t *tomb.Tomb, as *crv1alpha1.ActionSet) error { | ||
as, err := c.crClient.CrV1alpha1().ActionSets(as.GetNamespace()).Get(ctx, as.GetName(), v1.GetOptions{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why this call is necessary, since we are already passing in as
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it's unnecessary, need to remove
bpMap := make(map[string]*crv1alpha1.Blueprint) | ||
for _, bp := range bps.Items { | ||
bpMap[bp.Name] = bp | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of re-listing and storing all blueprints for a single actionset? An actionset is mapped to just one blueprint.
Also, how about just use the K8s client-go informers
and its Lister()
method to retrieve the needed blueprint and maintain the cache, instead of doing it ourselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where this is validated, but if I'm not mistaken according to the code actions in an actionset may have associations with different blueprints.
Using an informer will add need to wait after adding a blueprint and before adding an actions due to cache resync. Do we want such behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really @ed-shilo. An actionset can point to just one blueprint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created blueprints not being available immediately might be a concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blueprint informer watches for new blueprints and updates its indexer accordingly. Try add a new blueprint, the blueprint informer should respond immediately. I am not sure why we are using the low level cache
directly here, but it looks like we are already using client-go caching mechanism.
Re-sync applies to existing resources, when client-go tries to reconcile any differences between the cache and the data in its store.
@ed-shilo You are right about the 1:n mapping between actionset and blueprint. It looks like an actionset can have multiple actions, each of which can point to whichever blueprint it wants, even though downstream normally associate one actionset to one blueprint. Not sure why the API is designed that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ihcsim tried to use lister for getting blueprints, but as I see a newly created blueprint goes into cache after some small time. In tests this is observed in failing of attempts to add an action set linked with an already added blueprint due to cache miss. Are we OK with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, even though we don't use different blueprints for separate actions in an actionset, it does look like the code allows it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that caching isn't a major contributor to the bottleneck. If anything, I will be suspicious of code like this where every phase hits a recursive reflection call. It can get expensive if there are sophisticated Go template statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it formally assumed that it should have the same value in case of multiple actions?
No, we can't assume that anymore. If the API let user do multiple blueprints, we have to support it.
Re: missing cache, did you use the informers lister or the cache lister. Looks like our code is using the cache
one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.logAndErrorEvent(ctx, "Could not get blueprint:", "Blueprint not specified", err, as) | ||
break | ||
} | ||
bp, ok := bps[a.Blueprint] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend using the client-go informers
to retrieve the blueprint from its cache, instead of managing the cache ourselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question about adding need to wait between bp and as creation.
if err != nil { | ||
return errors.WithStack(err) | ||
} | ||
if err := validate.ActionSet(as); err != nil { | ||
return err | ||
} | ||
bps, err := c.createBpCache(ctx, as.GetNamespace()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will get expensive during a controller restart or if the actionset get re-queued. See #1267. IIRC, on restart, the controller will call onAddActionSet()
on all existing actionset resources. With this change, the controller will create a map of all blueprints for every known actionset.
@ihcsim @viveksinghggits @pavannd1 I've opened the follow-up PR #1668, which addresses the rest of comments left after merge. I decided to return back direct retrievals of blueprints from the API server, as it seems that using caches brings much more problems in comparison with its contribution to overall workflow runtime. |
Change Overview
The proposed set of changes improves perf of Add ActionSet workflow handling in Kanister controller by fixing the following issues:
onAdd
event handler starts an ActionSet execution goroutines after prevalidation stage, which contains several API calls to K8S apiserver. These calls take several seconds before spawning a goroutine, and considering the fact that go-client cache callsonAdd
sequentially this adds significant time delay before starting ActionSets if multiple ones are submitted simultaneously.As a proof of perf improvement I measured execution time of several ActionSets submitted simultaneously depending on their count and phase count in a corresponding blueprint. A test ActionSet just performed single KubeExec call to minimize phase execution impact.
Results:
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan