Refactor container watching out of raw handler into its own inteface / package #1263
Conversation
|
it contains 2 parts
|
manager/manager.go
Outdated
| } | ||
|
|
||
| // As we are proposing having multiple ways to detect a container (in addition to cgroups) | ||
| // need the ability to delete |
There was a problem hiding this comment.
I don't understand this comment.
There was a problem hiding this comment.
so currently we detect containers via raw cgroups (what I'll call the "raw" method). I'm proposing the ability to detect containers via other methods, such as the rkt api service.
if a raw handler was already created for a cgroup path for a rkt container (via raw detection), we want the rkt path to overwrite/update it to the rkt handler. Or at least that's what I thought you wanted. i.e. the raw handler will be destroyed and a rkt handler will take its place. all in one function to ensure atomic nature.
if the rkt handler is created first, the raw handler wont be created as createContainer() skips if the cgroup is already a key in the map.
There was a problem hiding this comment.
Ok, update the comment with that explanation? :)
I think the comment is not a proper sentence.
There was a problem hiding this comment.
done
// Enables overwriting an existing containerData/Handler object for a given containerName
// createContainer just returns if a given containerName has a handler already
// Ex: rkt handler will want to take priority over the raw handler, but the raw handler might be created first
|
I don't like passing around the detection type. I think it would be better if factories are registered per detection mechanism instead. Rather than registering the factories against a global (https://github.com/google/cadvisor/blob/master/container/factory.go#L76), we could create a "ContainerWatcher" interface, something like: type ContainerWatcher interface {
Start()
Stop()
RegisterFactory(factory ContainerHandlerFactory)
} |
|
ok, I'll try to work someting up tomorrow, it will basically mirror the existing factory system. |
|
so this commit I'm not sure is exactly where you want it, but instead of passing the detectionType all the way through it's just passed to container.NewContainerHandler (i.e. the factory method). When we register factories we say what type of detection they support (I used a slice so they can technically support more than one, but unsure that's really an issue) And I expanded the SubcontainerEvent to include the detection type, that way I hope we can just have multiple "detectors" writing to the same channel. Next commit will add a new event called Update which will use the update path from the original commit |
|
@timstclair @vishh so lots of surgery, but this moves the raw cgroup container watcher to its own package and enables registering other watchers with the manager to use the same channel to write on. I have a polling rkt implementation here https://github.com/sjpotter/cadvisor/tree/new-handler-detection-with-rkt/ |
container/container.go
Outdated
| Name string | ||
|
|
||
| //who detected it | ||
| DetectionType string |
There was a problem hiding this comment.
I still don't like having "detection type". It just feels messy and not the right abstraction. Why is it necessary? I don't think container factories should care how the container was detected, their job is to just create the handler for it. It should be up to the manager (or a child module) to ensure that the correct factories are hooked up to the right detection sources.
There was a problem hiding this comment.
I'm trying to minimize the code duplication, so only have them all going through the same event loop which goes to the same factory loop.
However, since we are dealing with the same container names if we don't distinguish where the container name came from, rkt wouldn't be able to determine if it was for it or not and would have to wait to make sure. it's just labeling which watcher found it.
I'm not convinced this is right, but I'm trying to keep the code simple and not overly complicated
|
@k8s-bot test this |
manager/watcher/raw/raw.go
Outdated
|
|
||
| // Maintain the watch for the new or deleted container. | ||
| switch { | ||
| case eventType == container.SubcontainerAdd: |
container/container.go
Outdated
| // The full container name of the container where the event occurred. | ||
| Name string | ||
|
|
||
| //who detected it |
There was a problem hiding this comment.
More description on the comment?
manager/manager.go
Outdated
| } | ||
|
|
||
| // Register for new subcontainers. | ||
| eventsChannel := make(chan watcher.SubcontainerEvent, 16) |
There was a problem hiding this comment.
How is the channel size (16) been decided? @sjpotter
There was a problem hiding this comment.
Ok, I saw that you copied from the old code.
|
LGTM overall, with some nits. So this PR splits the watcher from the container handler, right? @sjpotter |
|
@yifan-gu correct, this is just the refactor, the rkt support will be a separate drop. It will be a small rework of the handler along with a new "watcher" mechanism that use the rkt api service. |
manager/manager.go
Outdated
| case event.EventType == watcher.SubcontainerDelete: | ||
| err = self.destroyContainer(event.Name) | ||
| case event.EventType == watcher.SubcontainerOverride: | ||
| err = self.overrideContainer(event.Name, event.WatchSource) |
There was a problem hiding this comment.
I have some concerns around how override is handled, but it might make more sense once the rkt watcher is added. Can we just remove the override event & method from this PR, and I'll review it when it's actually used?
There was a problem hiding this comment.
removed the event for now
manager/manager.go
Outdated
|
|
||
| if containerData.handler.String() != "raw" { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Why is this check needed?
There was a problem hiding this comment.
Its now removed in this branch and is part of the rkt addons branch, but the reason is to only allow overriding a raw handler.
i.e. cgroup gets a raw handler, then we say it should get a rkt handler. it just to enforce. If you dont think its neccessary can remove it
|
Ok, I still don't like passing around the |
manager/manager.go
Outdated
| // Ex: rkt handler will want to take priority over the raw handler, but the raw handler might be created first | ||
|
|
||
| // Only allow raw handler to be overriden | ||
| func (m *manager) overrideContainer(containerName string, watchSource watcher.ContainerWatchSource) error { |
There was a problem hiding this comment.
I thought I said this elsewhere, but can't find the comment.... Can we remove overrrideContainer for now (since it's unused), so that I can review it when I can see how it's actually used?
There was a problem hiding this comment.
you did, and I did, and just pushed it.
sjpotter
changed the title
manager/manager.go
Outdated
| for _, watcher := range self.containerWatchers { | ||
| err := watcher.Stop() | ||
| if err != nil { | ||
| errors = append(errors, fmt.Sprintf("%v", err)) |
container/container.go
Outdated
| Start() | ||
|
|
||
| //Name of handler | ||
| String() string |
There was a problem hiding this comment.
we could for now, but its needed for identifying handlers later on
There was a problem hiding this comment.
Let's add it when it's needed.
|
@timstclair Are we able to get those rkt support PRs in and bump cadvisor in 7 days? |
|
Yes. Please make sure that any remaining issues / PRs are marked with the kubernetes v1.3 milestone |
|
Once this pr is merged i have another pr with the rkt implementation ready Testing it with k8s now.
|
5 participants
So this PR is about trying to enable other methods of identifying container creation that can run in parallel with cgroup based detection as well as enabling one to override a cgroup based detection handler (either to operate outside the cgroup hierarchy, as might be case with VMs) or simply override the cgroup based handler.
idea is to use get it so the we an determine when rkt pods are created not via cgroup creation but via the rkt api service itself.