feature: init and add base pouchd logs API #1298
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1298 +/- ##
========================================
+ Coverage 16.42% 17.42% +1%
========================================
Files 183 188 +5
Lines 11309 11753 +444
========================================
+ Hits 1857 2048 +191
- Misses 9316 9557 +241
- Partials 136 148 +12
|
apis/types/log_config.go
Outdated
|
|
||
| // type | ||
| Type string `json:"Type,omitempty"` | ||
| } |
There was a problem hiding this comment.
In moby, the LogConfig has a type of
// LogConfig represents the default log configuration.
// It includes json tags to deserialize configuration from a file
// using the same names that the flags in the command line use.
type LogConfig struct {
Type string `json:"log-driver,omitempty"`
Config map[string]string `json:"log-opts,omitempty"`
}
The json naming of the fields are different, log-driver rather than Type, log-opts rather than Config.
Do you think we need to keep compatible? @fuweid
There was a problem hiding this comment.
Make senses. Will update it.
apis/server/router.go
Outdated
| // will hang and cause goroutine leak. | ||
| func withCancelHandler(h handler) handler { | ||
| return func(ctx context.Context, rw http.ResponseWriter, req *http.Request) error { | ||
| if notifier, ok := rw.(http.CloseNotifier); ok { |
There was a problem hiding this comment.
How about using return fast to make less ident?
if notifier, ok := rw.(http.CloseNotifier); !ok {
return h(ctx, rw, req)
}
var cancel context.CancelFunc
ctx, cancel = context.WithCancel(ctx)
......
return h(ctx, rw, req)
There was a problem hiding this comment.
Agree. Will update it.
|
|
||
| // JSONLogFile is uses to log the container's stdout and stderr. | ||
| type JSONLogFile struct { | ||
| mu sync.Mutex |
There was a problem hiding this comment.
I am wondering this lock will detect what kind of critical section.
At least, in my opinion, a f *os.File does not need locked. This is not an instance allocated in memory and shared within several goroutine. It is specific filesystem and io resource to guarantee the concurrent writing of the file. WDYT? @fuweid
Correct me if I missed anything.
In addition, @shaloulcy found that in the definition of os.File, it is already has a lock there to guarantee concurrency things in internal/poll/fd_unix.go:
// Write implements io.Writer.
func (fd *FD) Write(p []byte) (int, error) {
if err := fd.writeLock(); err != nil {
return 0, err
}
defer fd.writeUnlock()
if err := fd.pd.prepareWrite(fd.isFile); err != nil {
return 0, err
}
var nn int
There was a problem hiding this comment.
I'm still wondering the case that if we need file.Name() related action. The case still needs the lock. Other cases, like Write and Close, need the lock to make sure the concurrent issue. I think we can keep the lock here. After make the container.io stable and high performance, we will remove the lock if we don't need it. WDYT?
daemon/logger/jsonfile/utils.go
Outdated
| "time" | ||
|
|
||
| "github.com/alibaba/pouch/daemon/logger" | ||
| "github.com/sirupsen/logrus" |
There was a problem hiding this comment.
Add a blank line at line 12, and remove the blank line in L13.
daemon/logger/jsonfile/utils.go
Outdated
|
|
||
| const ( | ||
| blockSize = 1024 | ||
| eol = '\n' |
There was a problem hiding this comment.
how about using endOfLine intead of eol which seems to be less readable. 😄
| // NOTE: created container doesn't create IO. | ||
| if c.IsCreated() { | ||
| msgCh := make(chan *logger.LogMessage, 1) | ||
| close(msgCh) |
There was a problem hiding this comment.
I cannot quite follow the above two line of code. Why to initialize a channel and close it immediately?
There was a problem hiding this comment.
The created container doesn't have the log file in current design.
For this case, we should fast return with closed channel. The receiver receives from closed channel and it will break the dead loop and finish the writeLogStream.
I just hold the change scope in the log side and don't want to change the logic of initialize container in this commit. Does it make senses? Maybe I can add the TODO to explain why I do.
pkg/httputils/http_utils.go
Outdated
| "net/http" | ||
| "strings" | ||
|
|
||
| "github.com/alibaba/pouch/apis/types" |
There was a problem hiding this comment.
I do not think that a package in pkg like httputils should import packages from alibaba/pouch. WDYT
Otherwise, it is not so common that we need to split it out of pkg and locate it in project pouch/..., like logs_utils.go .
There was a problem hiding this comment.
I think we can put it into the apis package level. It's only used in logs API.
| @@ -2131,6 +2115,27 @@ definitions: | |||
| HostConfig: | |||
| $ref: "#/definitions/HostConfig" | |||
|
|
|||
| LogConfig: | |||
There was a problem hiding this comment.
I am afraid we still need to update the routes of /containers/{id}/logs: in this file, since the status code in the code from manager are more than that has been defined in /containers/{id}/logs:.
For example, 400.
There was a problem hiding this comment.
Make senses. Will do
|
I left some comments there, while it is still not so easy to do the review work. |
In this commit, pouchd will redirect the stdout/stderr into the
/var/lib/pouch/containers/{id}/json.log. However, we don't redirect the
exec log into the json.log.
This commit doesn't contains compress log and logrotate functionality
and also doesn't support show details in the stream.
Signed-off-by: Wei Fu <fhfuwei@163.com>
|
@allencloud , I have updated the code and please take a look. Thanks |
|
LGTM, let's kick the ball. |
pouchrobot
added
the
LGTM
Signed-off-by: Wei Fu fhfuwei@163.com
Ⅰ. Describe what this PR did
pouchd will redirect the container's stdout/stderr into the
/var/lib/pouch/containers/{id}/json.log. However, we don't redirect the exec log into the json.log.However, this commit doesn't contains
compressandlogrotatefunctionality. And Haven't support show details in the stream yet. We will upgrade the functionality in the future.Ⅱ. Does this pull request fix one issue?
NONE.
Ⅲ. Describe how you did it
Using the
containeriopkg to redirect the stdout/stderr into thejson.log. Since theraw-filein thecontaineriopkg is like thejson.log, in order to avoid duplicate data, I remove theraw-fileoption in the pkg.The important parts are
tailFileandfollowFile.tailFile
Before reading log, we must use the
tailLineto seek the offset in the file. With the offset, we can set it to file and read it.followFIle
I use
github.com/fsnotify/fsnotifyto watch the file. But it has limitation about theRemoveevent. In order to make it work, I useos.Statto check the file if the watcher has timeout. Workaround.... sad :( ...Besides this, limit of current design of
containerio, we cannot get the event when the container has been stopped. I have to use the timeout-check the status of container. sad :( :(....+2Ⅳ. Describe how to verify it
first, we run the following command:
Since we don't add the related cli, we can use docker to check
and
and
if you don't want to use docker, you can use curl:
Ⅴ. Special notes for reviews
pouch cli will be done in the following PR.
And please read the code comment to get more detail. Thanks