Use containerd as source of truth #21
Conversation
What about restarting faas-containerd (tasks stay), or restarting the host (tasks die)? Have you tried a scale to zero? replicas should be 0 if not running, so the scale to zero pauses them.. and you should see them unpause when invoking through faasd and the gateway. |
|
I checked out the PR, built it and deployed with CNI 0.8.4 on x86_64, this is what I got |
|
This is caused due to a lack of proper error handling, if a function returns two things - a result and an error, then you cannot use that result if there was an error. I'd suggest the following: func (s *ServiceMap) Update() {
s.lock.Lock()
defer s.lock.Unlock()
containers, _ := s.client.Containers(s.context)
for _, k := range containers {
id := k.ID()
s.containers[id] = k
t, err := k.Task(s.context, nil)
if err != nil {
log.Printf("No task for %s\n", id)
} else {
svc, statusErr := t.Status(s.context)
if statusErr != nil && svc.Status == "running" {
s.servicesPID[id] = t.Pid()
// Get container IP address
ip, ipErr := getIP(DefaultBridgeName, int(t.Pid()))
if ipErr != nil {
s.services[id] = ip
}
}
}
}
}If t doesn't exist, then update replicas to 0. |
|
After fixing that, I get: faasd after merging openfaas/faasd#21 shows: |
Feel free to apply this patch |
|
My original suggestion was to remove the service map entirely and go directly to containerd all the time, then once we would have had that stable, I would have re-written the cache and optimisations. |
|
@carlosedp please can you reply to the comments I've left for you, and see if you have addressed or have any questions? |
|
/set title: Use containerd as source of truth |
derek
bot
changed the title
|
@carlosedp this will need a rebase now due to the merge of #26 |
|
Working on this already rebased on #26 |
|
I'm not seeing any changes for |
Removed ServiceMap struct and changed all operations to be performed on containerd. All status for the running and stopped tasks are read by the current operation being executed. Signed-off-by: Carlos de Paula <[email protected]>
|
Force-pushed new commit where all ServiceMap struct has been removed and all state comes from containerd. |
| if err != nil { | ||
| return errors.Wrapf(err, "Unable to get task for container %s: %s", name, err) | ||
| log.Printf("[Delete] container %s does not have task\n", name) |
There was a problem hiding this comment.
Should probably say a task, but how about: Unable to find a task for container: %s\n
| image: image.Name(), | ||
| } | ||
| replicas := 0 | ||
| task, err := c.Task(ctx, nil) |
There was a problem hiding this comment.
If the task is in a frozen state, the replicas should also be 0, this could be fixed in a follow-up PR
| // Task for container exists | ||
| svc, err := task.Status(ctx) | ||
| if err != nil { | ||
| return Function{}, fmt.Errorf("Unable to get task status for container: %s", name, err) |
There was a problem hiding this comment.
In Go, errors should start with lowercase
There was a problem hiding this comment.
Small bug here too, 2 args, but only one %s
| } | ||
|
|
||
| func (i *InvokeResolver) Resolve(functionName string) (url.URL, error) { | ||
| log.Printf("Resolve: %q\n", functionName) | ||
|
|
||
| serviceIP := i.serviceMap.Get(functionName) | ||
| if serviceIP == nil { | ||
| fun, err := GetFunction(i.client, functionName) |
There was a problem hiding this comment.
can you write function or fn, vs fun? Just a nit
|
I'm going to merge and ask for you to work out anything else that needs a change in HEAD / master. |
| return f, nil | ||
|
|
||
| } | ||
| return Function{}, fmt.Errorf("Unable to find function %s: %s", name, err) |
There was a problem hiding this comment.
Possible typo, %s: should normally be:
function: NAME
In Go, errors should start with lower-case. Ref: #21 Signed-off-by: Alex Ellis (OpenFaaS Ltd) <[email protected]>
Description
This allow persisting state between restarts by reading containers from
containerd on certain task status.
Motivation and Context
Currently faas-containerd does not persist data between restarts so deployed functions cannot be used if faas-containerd process is restarted.
Addresses issue #20 .
How Has This Been Tested?
Verified the basic service functions to Update, Add, Get and List the containers. Still work-in-progress.
Types of changes
Checklist:
Commits:
git commit -sfor the Developer Certificate of Origin (DCO)Code:
Docs: