Skip to content
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

container: support user defined logs endpoint #578

Merged
merged 25 commits into from
Mar 24, 2020
Merged

container: support user defined logs endpoint #578

merged 25 commits into from
Mar 24, 2020

Conversation

maxux
Copy link
Contributor

@maxux maxux commented Mar 16, 2020

This PR allows user to forward container stdout/stderr to external endpoint. Right now, only redis is supported. You can define on container reservation a new field logs which contains information where to push logs:

        "logs": [
          {
            "type": "redis",
            "data": {
              "endpoint": "redis://10.241.0.232:6370",
              "channel": "debug"
            }
          }
        ],

This endpoint is a reachable redis, and channel is a pub/sub where logs will be sent (one line at a time).

@maxux maxux changed the title Container logs container: support user defined logs endpoint Mar 18, 2020
@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #578 into master will decrease coverage by 0.24%.
The diff coverage is 6.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #578      +/-   ##
==========================================
- Coverage   35.57%   35.33%   -0.25%     
==========================================
  Files          74       74              
  Lines        5374     5419      +45     
==========================================
+ Hits         1912     1915       +3     
- Misses       3189     3229      +40     
- Partials      273      275       +2     
Impacted Files Coverage Δ
pkg/container/container.go 0.00% <0.00%> (ø)
pkg/provision/converter.go 43.71% <6.66%> (-3.66%) ⬇️
pkg/gedis/converter.go 24.66% <7.69%> (-1.62%) ⬇️
pkg/provision/container.go 60.68% <100.00%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aeca858...ec419e8. Read the comment docs.

Copy link
Member

@muhamadazmy muhamadazmy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent PR. but I have one comment that requires a change (regarding schema)

pkg/logger/logger_redis.go Outdated Show resolved Hide resolved
Copy link
Contributor

@zaibon zaibon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the PR overall looks ok.
I think there is a way to make things a bit simpler though.
Instead of adding a new ContainerLogger interface, we could just use the WithStream method.
This methods takes any io.Writer. This means any new logger implementation should just implement the io.Writer interface.

The result would be something like

mw := io.MultiWriter(
	logFile,
	redisLogger,
       // any other object that implement io.Writer...
)
container.NewTask(ctx,cio.NewCreator(cio.WithStreams(nil, mw, mw)))

pkg/logger/logger.go Outdated Show resolved Hide resolved
pkg/container/container.go Outdated Show resolved Hide resolved
pkg/container/container.go Outdated Show resolved Hide resolved
pkg/logger/logger_console.go Outdated Show resolved Hide resolved
pkg/logger/logger_file.go Outdated Show resolved Hide resolved
pkg/logger/logger_redis.go Outdated Show resolved Hide resolved
pkg/logger/logger_redis.go Outdated Show resolved Hide resolved
pkg/logger/logger_file.go Outdated Show resolved Hide resolved
pkg/logger/loggers.go Outdated Show resolved Hide resolved
tools/tfuser/main.go Show resolved Hide resolved
Replace logs implementation, switching from custom internal handling to
support WithStreams that containerd supports out-of-box. New
implementation leverage this to io.Write class and remove the internal
write process.
@maxux
Copy link
Contributor Author

maxux commented Mar 23, 2020

Latest implementation use the MultiWriter that @zaibon suggested, reducing amount of code and complexity. Thanks :)

Still some improvement to fix and everything will be updated.

- Move pkg/logger to pkg/container/logger since logger package are only
for container purpose

- Update name of struct inside logger package for shorter names
  - ContainerLoggerConsole -> Console
  - ContainerLoggerFile    -> File
  - ContainerLoggerRedis   -> Redis

  - NewContainerLoggers       -> NewLoggers
  - NewContainerLoggerFile    -> NewFile
  - NewContainerLoggerRedis   -> NewRedis
  - NewContainerLoggerConsole -> NewConsole

- Update provision to replace endpoint/channel with redis url for stdout
and stderr

- Update tfuser to support new provision fields
  - Removing --logs and --channel
  - Adding --stdout and --stderr options

- Argument are now always using redis://host:port/channel format
@maxux
Copy link
Contributor Author

maxux commented Mar 24, 2020

From 0d6d56f commit message:

  • Move pkg/logger to pkg/container/logger since logger package are only for container purpose
  • Update name of struct inside logger package for shorter names
    • ContainerLoggerConsole -> Console
    • ContainerLoggerFile -> File
    • ContainerLoggerRedis -> Redis
    • NewContainerLoggers -> NewLoggers
    • NewContainerLoggerFile -> NewFile
    • NewContainerLoggerRedis -> NewRedis
    • NewContainerLoggerConsole -> NewConsole
  • Update provision to replace endpoint/channel with redis url for stdout and stderr
  • Update tfuser to support new provision fields
    • Removing --logs and --channel
    • Adding --stdout and --stderr options
  • Argument are now always using redis://host:port/channel format

@zaibon zaibon merged commit 952dd5c into master Mar 24, 2020
@zaibon zaibon deleted the container-logs branch March 24, 2020 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants