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.

Sign up for GitHub

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

WIP: Added support for log policy and log rate limit in conmon #4663

Closed
wants to merge 2 commits into from
Closed

WIP: Added support for log policy and log rate limit in conmon #4663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad15f85
Added support for log policy and log rate limit in conmon
syedriko Dec 9, 2019
004258d
Added cross-log-opt validation for policy and rate-limit
syedriko Dec 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions libpod/container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,10 @@ type ContainerConfig struct {
CgroupParent string `json:"cgroupParent"`
// LogPath log location
LogPath string `json:"logPath"`
// LogPolicy logging policy
LogPolicy string `json:"logPolicy"`
// LogRateLimit
LogRateLimit string `json:"logRateLimit"`
// LogDriver driver for logs
LogDriver string `json:"logDriver"`
// File containing the conmon PID
Expand Down Expand Up @@ -719,6 +723,16 @@ func (c *Container) LogPath() string {
return c.config.LogPath
}

// LogPolicy returns the container's log policy
func (c *Container) LogPolicy() string {
return c.config.LogPolicy
}

// LogRateLimit returns the container's log rate limit
func (c *Container) LogRateLimit() string {
return c.config.LogRateLimit
}

// RestartPolicy returns the container's restart policy.
func (c *Container) RestartPolicy() string {
return c.config.RestartPolicy
Expand Down
6 changes: 6 additions & 0 deletions libpod/oci_conmon_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,12 @@ func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, p
}

args = append(args, "-l", logDriver)
if logPolicy := ctr.LogPolicy(); logPolicy != "" {
args = append(args, "--log-policy", logPolicy)
}
if logRateLimit := ctr.LogRateLimit(); logRateLimit != "" {
args = append(args, "--log-rate-limit", logRateLimit)
}
args = append(args, "--exit-dir", exitDir)
args = append(args, "--socket-dir-path", r.socketsDir)
if r.logSizeMax >= 0 {
Expand Down
42 changes: 42 additions & 0 deletions libpod/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@ import (
var (
NameRegex = regexp.MustCompile("^[a-zA-Z0-9][a-zA-Z0-9_.-]*$")
RegexError = errors.Wrapf(define.ErrInvalidArg, "names must match [a-zA-Z0-9][a-zA-Z0-9_.-]*")
LogRateLimitRegex = regexp.MustCompile(`^[[:digit:]]+[KMGT]?$`)
LogRateLimitRegexError = errors.Wrapf(define.ErrInvalidArg,
`log rate limit must be a number followed by an optional suffix K, M, G or T.`,
)
)

// Runtime Creation Options
Expand Down Expand Up @@ -1057,6 +1061,44 @@ func WithLogPath(path string) CtrCreateOption {
}
}

// WithLogPolicy sets the logging policy.
func WithLogPolicy(policy string) CtrCreateOption {
return func(ctr *Container) error {
if ctr.valid {
return define.ErrCtrFinalized
}
switch policy {
case "":
return errors.Wrapf(define.ErrInvalidArg, "log policy must be set")
case "backpressure", "drop", "ignore", "passthrough":
break
default:
return errors.Wrapf(define.ErrInvalidArg, "invalid log policy")
}
ctr.config.LogPolicy = policy
return nil
}
}

// WithLogRateLimit sets the logging rate limit.
func WithLogRateLimit(rateLimit string) CtrCreateOption {
return func(ctr *Container) error {
if ctr.valid {
return define.ErrCtrFinalized
}
if rateLimit == "" {
return errors.Wrapf(define.ErrInvalidArg, "log rate limit must be set")
Copy link

Choose a reason for hiding this comment

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

Do we need the rate limit specified if the policy is ignore or passthrough?

Copy link
Author

Choose a reason for hiding this comment

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

A rate limit sure makes no sense for ignore or passthrough (which is the current unrestricted behavior, btw). How do you think should the CLI behave here, error out in that case, ignore, warn?

Copy link

Choose a reason for hiding this comment

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

So with no changes by the user, they will get passthrough. So that case should work today, which I think this code as it stands will raise an error.

If they specify ignore or passthrough, then they should not specify a rate, and it should be an error if they do.

If they specify backpressure or drop, then they must specify the rate.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, makes perfect sense.

Copy link
Author

Choose a reason for hiding this comment

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

Added the checks to https://github.com/syedriko/libpod/blob/004258dd4343d055876137d490ca3b2cbff68f15/pkg/spec/createconfig.go#L290

Copy link

Choose a reason for hiding this comment

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

We're writing to files and the file writes block until the file is written, so the current default is "backpressure" with no rate-limit. I think you could simplify validation so that rate-limit is optional regardless of policy. The "ignore" policy trivially respects any rate-limit (even 0) There is no such thing as "passthrough" - the existing behaviour is backpressure with no rate-limit.

Copy link
Author

Choose a reason for hiding this comment

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

The semantics starts to get interesting if we consider where the backpressure comes from, from the code we're looking at, where the rate would sure apply, or downstream from this code. In that frame of reference, how would we implement drop without a rate limit, by making the log fd non-blocking, etc?

Copy link

Choose a reason for hiding this comment

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

A non-blocking partial write followed by a rate calculation I'd say.

The whole thing would be simpler as a poll loop with non-blocking reads/writes and a timer fd.

Copy link
Author

Choose a reason for hiding this comment

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

Without a rate limit there's no basis for calculation though. A simple implementation can attempt to write to the non-blocking log fd and drop on seeing EAGAIN. A more involved one could poll() the fd and flip a "drop" flag.

Copy link

Choose a reason for hiding this comment

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

For backpressure there's nothing to calculate, you just block on write and delay reading as a consequence - the downstream determines how fast you go. For drop what you describe is right, but I would add a small buffer to avoid dropping due to minor jitters in read/write rate. Then drop on read with buffer-full.

} else {
if LogRateLimitRegex.MatchString(rateLimit) {
ctr.config.LogRateLimit = rateLimit
return nil
} else {
return LogRateLimitRegexError
}
}
}
}

// WithNoCgroups disables the creation of CGroups for the new container.
func WithNoCgroups() CtrCreateOption {
return func(ctr *Container) error {
Expand Down
20 changes: 19 additions & 1 deletion pkg/spec/createconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,11 +282,29 @@ func (c *CreateConfig) getContainerCreateOptions(runtime *libpod.Runtime, pod *l
options = append(options, libpod.WithStopSignal(c.StopSignal))
options = append(options, libpod.WithStopTimeout(c.StopTimeout))

logPath := getLoggingPath(c.LogDriverOpt)
logPath := getLoggingOption(c.LogDriverOpt, "path")
if logPath != "" {
options = append(options, libpod.WithLogPath(logPath))
}

logPolicy := getLoggingOption(c.LogDriverOpt, "policy")
logRateLimit := getLoggingOption(c.LogDriverOpt, "rate-limit")
if (logPolicy == "" || logPolicy == "passthrough" || logPolicy == "ignore") && logRateLimit != "" {
if logPolicy == "" {
logPolicy = "passthrough"
}
return nil, errors.Wrapf(define.ErrInvalidArg, "Log rate limit is not supported for log policy %s", logPolicy)
}
if (logPolicy == "backpressure" || logPolicy == "drop") && logRateLimit == "" {
return nil, errors.Wrapf(define.ErrInvalidArg, "Log rate limit is not provided for log policy %s", logPolicy)
}
if logPolicy != "" {
options = append(options, libpod.WithLogPolicy(logPolicy))
}
if logRateLimit != "" {
options = append(options, libpod.WithLogRateLimit(logRateLimit))
}

if c.LogDriver != "" {
options = append(options, libpod.WithLogDriver(c.LogDriver))
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/spec/parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,11 +132,11 @@ func validateIOpsDevice(val string) (*throttleDevice, error) { //nolint
}, nil
}

func getLoggingPath(opts []string) string {
func getLoggingOption(opts []string, option string) string {
for _, opt := range opts {
arr := strings.SplitN(opt, "=", 2)
if len(arr) == 2 {
if strings.TrimSpace(arr[0]) == "path" {
if strings.TrimSpace(arr[0]) == option {
return strings.TrimSpace(arr[1])
}
}
Expand Down