search

serverless / serverless-kubeless

This plugin enables support for Kubeless within the Serverless Framework.
Apache License 2.0
303 stars 80 forks source link

Better way to check when function pod is ready #230

Closed faxioman closed 3 years ago

faxioman commented 3 years ago

The current logic to check if a pod for a deployed function is running fails in these situations:

This pull request fixes these situations by checking the correct pod revision for the "current" replicaSet and by filtering evicted/succeded pods

faxioman commented 3 years ago

Checking the deployment was my first implementation... but reverted. You can't say when kubernetes will start to update the status fields for the deployment. waitForDeployment could exit immediately because it will find an updated deployment which is the old deployment. k8s add a pod-template-hash label to all pod of a replicaset, I think checking this label is the more consistent way to know when the new replicaset is the "current" replicaset. In newer version of k8s, from what I understand, replicaset is the entity which you can rely on to check the status of a deploy... but I could be wrong ;) Anyway yes, I need to fix the test :)

faxioman commented 3 years ago

@andresmgot if you think it's a better implementation, I could pass the current deployment revision to waitForDeployment and in waitForDeployment function check status.updatedReplicas for a deployment revision > than the older revision. What do you think?

PS. mmm, I think it's a better implementation too :)

faxioman commented 3 years ago

The only thing we lose switching from pod check to deployment check is the pod failure count. Is it acceptable for you @andresmgot ?

faxioman commented 3 years ago

@andresmgot switching from pod check to deployment check needs the rewriting of a lot of tests and the implementation of a logic of revision increment for the deployments in mock. Unfortunately, It is not possible for me at the moment to allocate the time necessary for this refactoring :( For now, I'll proceed with a fork of the plugin.

I'm really sorry about this.

faxioman commented 3 years ago

@andresmgot In my forked repo I've fixed all the tests based on this implementation (replicaset check). If you think it's worth it, I can submit a new pull request.

andresmgot commented 3 years ago

Sorry for the late reply. I understand that change the whole logic to check deployments rather than pods may be quite time consuming.

If you have already working (and tested) code which doesn't add breaking changes and leaves the code in a better state I am happy to merge it in this repository :) Thanks for the effort!