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

API: add console ringbuffer extension #1871

Merged
merged 8 commits into from Nov 8, 2017

Conversation

3 participants
@brauner
Member

brauner commented Oct 22, 2017

Closes #1870.

Signed-off-by: Christian Brauner christian.brauner@ubuntu.com

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Oct 22, 2017

Member

I think we call the API function console_log() and give it a struct console_log as second argument. I'll push an RFC on top of this patchset soon.

Member

brauner commented Oct 22, 2017

I think we call the API function console_log() and give it a struct console_log as second argument. I'll push an RFC on top of this patchset soon.

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Oct 22, 2017

Member

Ok, so I've changed the API extension quite a lot in a commit I pushed on top of this patchset. Please don't merge this yet until I a) removed the lxc-console patches and b) I squashed the commits once we all agree that the layout for the new API extension is fine. Here's the TL;DR:

  • new function is:
    int (*console_log)(struct lxc_container *c, struct console_log *log)
  • new struct is added
struct console_log {
	/* Clear the console log. */
	bool clear;

	/* Retrieve the console log. */
	bool read;

	/* This specifies the maximum size to read from the ringbuffer. Setting
	 * it to 0 means that the a read can be as big as the whole ringbuffer.
	 * On return callers can check how many bytes were actually read.
	 * If "read" and "clear" are set to false and a non-zero value is
	 * specified then up to "read_max" bytes of data will be discarded from
	 * the ringbuffer.
	 */
	uint64_t *read_max;

	/* Data that was read from the ringbuffer. If "read_max" is 0 on return
	 * "data" will be NULL.
	 */
	char *data;
};

I think the struct makes sense since it will allows us to easily expand or deprecate operations on the ringbuffer.
Having the ringbuffer contents be returned in the struct enables us to return meaningful errors via the return value of console_log(). For example, callers can check for -ENODATA which I find quite useful. It also allows the API to do sensible logging outside of the actual command infrastructure. I prefer this very much since it keeps it apap (as performant as possible :)).

Member

brauner commented Oct 22, 2017

Ok, so I've changed the API extension quite a lot in a commit I pushed on top of this patchset. Please don't merge this yet until I a) removed the lxc-console patches and b) I squashed the commits once we all agree that the layout for the new API extension is fine. Here's the TL;DR:

  • new function is:
    int (*console_log)(struct lxc_container *c, struct console_log *log)
  • new struct is added
struct console_log {
	/* Clear the console log. */
	bool clear;

	/* Retrieve the console log. */
	bool read;

	/* This specifies the maximum size to read from the ringbuffer. Setting
	 * it to 0 means that the a read can be as big as the whole ringbuffer.
	 * On return callers can check how many bytes were actually read.
	 * If "read" and "clear" are set to false and a non-zero value is
	 * specified then up to "read_max" bytes of data will be discarded from
	 * the ringbuffer.
	 */
	uint64_t *read_max;

	/* Data that was read from the ringbuffer. If "read_max" is 0 on return
	 * "data" will be NULL.
	 */
	char *data;
};

I think the struct makes sense since it will allows us to easily expand or deprecate operations on the ringbuffer.
Having the ringbuffer contents be returned in the struct enables us to return meaningful errors via the return value of console_log(). For example, callers can check for -ENODATA which I find quite useful. It also allows the API to do sensible logging outside of the actual command infrastructure. I prefer this very much since it keeps it apap (as performant as possible :)).

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Oct 22, 2017

Member

See here for an illustration:

asciicast

Member

brauner commented Oct 22, 2017

See here for an illustration:

asciicast

brauner added a commit to brauner/lxc that referenced this pull request Oct 22, 2017

commands: add LXC_CMD_CONSOLE_LOG
Closes  lxc#1871.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner added a commit to brauner/lxc that referenced this pull request Oct 22, 2017

lxccontainer: add console_log() API extension
Closes lxc#1871.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner added a commit to brauner/lxc that referenced this pull request Oct 22, 2017

commands: add LXC_CMD_CONSOLE_LOG
Closes  lxc#1871.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner added a commit to brauner/lxc that referenced this pull request Oct 22, 2017

lxccontainer: add console_log() API extension
Closes lxc#1871.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner added a commit to brauner/lxc that referenced this pull request Oct 22, 2017

commands: add LXC_CMD_CONSOLE_LOG
Closes  lxc#1871.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

brauner added a commit to brauner/lxc that referenced this pull request Oct 22, 2017

lxccontainer: add console_log() API extension
Closes lxc#1871.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
@hallyn

This comment has been minimized.

Show comment
Hide comment
@hallyn

hallyn Oct 28, 2017

Member

(Hi - I'm assuming this has been on hold for Prague (given the last commit title) - please ping me when ready)

Member

hallyn commented Oct 28, 2017

(Hi - I'm assuming this has been on hold for Prague (given the last commit title) - please ping me when ready)

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Oct 28, 2017

Member

@hallyn, actually it is ready for review. :)

Member

brauner commented Oct 28, 2017

@hallyn, actually it is ready for review. :)

@brauner brauner changed the title from RFC: API: add console ringbuffer extension to API: add console ringbuffer extension Oct 28, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 2, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 2, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 2, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 2, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 2, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 2, 2017

brauner added some commits Oct 22, 2017

commands: non-functional changes
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
lxccontainer: add console_log() API extension
commands: add LXC_CMD_CONSOLE_LOG

Closes #1870.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
console: non-functional changes
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
console: move ringbuffer into lxc_console_create()
This makes the whole setup more flexible.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
console: write ringbuffer to disk
When users request that the container keep a console ringbuffer we will not
continously write to the on-disk logfile as mirroring the contents of the
in-memory ringbuffer on-disk is costly and complicated. Instead, we dump the
ringbuffer contents on-disk when the container stops or fails to start. This
way users can still diagnose problems or retrieve the last contents of the
ringbuffer on-disk.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
console: non-functional changes
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
console: add "write_logfile" to console_log struct
If a console log file was specified this flag indicates whether the contents of
the ringbuffer should be written to the logfile when a request is sent to the
ringbuffer.

Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
tests: add tests for console_log()
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@lxc lxc deleted a comment from lxc-jenkins Nov 7, 2017

@stgraber

This comment has been minimized.

Show comment
Hide comment
@stgraber
Member

stgraber commented Nov 7, 2017

@hallyn

This comment has been minimized.

Show comment
Hide comment
@hallyn

hallyn Nov 7, 2017

Member

Present.

(looking)

Member

hallyn commented Nov 7, 2017

Present.

(looking)

@hallyn

This comment has been minimized.

Show comment
Hide comment
@hallyn

hallyn Nov 7, 2017

Member

One thing about this API is that - if I don't miss something - it is easy to have a situation where

buffer has 200 bytes
user asks to read 100 bytes and clear the buffer
second 100 bytes are lost.

I'm probably misreading.

Member

hallyn commented Nov 7, 2017

One thing about this API is that - if I don't miss something - it is easy to have a situation where

buffer has 200 bytes
user asks to read 100 bytes and clear the buffer
second 100 bytes are lost.

I'm probably misreading.

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Nov 7, 2017

Member

If the user sets clear = true the ringbuffer gets wiped completely even if the user just requests to read 100 bytes. If the user only wants to discard 100 bytes after the read then clear should simply be left unset. What would you prefer?

Member

brauner commented Nov 7, 2017

If the user sets clear = true the ringbuffer gets wiped completely even if the user just requests to read 100 bytes. If the user only wants to discard 100 bytes after the read then clear should simply be left unset. What would you prefer?

@hallyn

This comment has been minimized.

Show comment
Hide comment
@hallyn

hallyn Nov 8, 2017

Member

Sorry, I'm not thinking about this quite right. I need to look through and understand what happens with multiple concurrent readers. Will comment after I grok that again.

Member

hallyn commented Nov 8, 2017

Sorry, I'm not thinking about this quite right. I need to look through and understand what happens with multiple concurrent readers. Will comment after I grok that again.

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Nov 8, 2017

Member

No problem, I can implement the LXD side without this extension.

Member

brauner commented Nov 8, 2017

No problem, I can implement the LXD side without this extension.

@hallyn

This comment has been minimized.

Show comment
Hide comment
@hallyn

hallyn Nov 8, 2017

Member

... so is there any point to ever calling ringbuf_clear()? since doing a read advances the read pointer.

Member

hallyn commented Nov 8, 2017

... so is there any point to ever calling ringbuf_clear()? since doing a read advances the read pointer.

@brauner

This comment has been minimized.

Show comment
Hide comment
@brauner

brauner Nov 8, 2017

Member

If you want to clear the complete buffer without reading from it.

Member

brauner commented Nov 8, 2017

If you want to clear the complete buffer without reading from it.

@hallyn

This comment has been minimized.

Show comment
Hide comment
@hallyn

hallyn Nov 8, 2017

Member

Ok. I'm good with it - thanks.

Member

hallyn commented Nov 8, 2017

Ok. I'm good with it - thanks.

@hallyn hallyn merged commit 5cc2545 into lxc:master Nov 8, 2017

4 checks passed

Branch target Branch target is correct
Details
Signed-off-by All commits signed-off
Details
Testsuite Testsuite passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment