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

Add intercepter and writer #2

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

nicolasgere
Copy link

@nicolasgere nicolasgere commented Jan 29, 2025

Add an interceptor at the fuse level, it would push a message to a buffered chan, to avoid as much as possible having any impact. The writer which listen to the chan, will write every 1mb OR 5 seconds

@nicolasgere nicolasgere force-pushed the nicolas/test-intercept branch from 9328e2d to 2733e74 Compare February 6, 2025 03:34
@nicolasgere nicolasgere changed the title add more things Add intercepter and writer Feb 6, 2025
Copy link

@ujjwal ujjwal left a comment

Choose a reason for hiding this comment

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

LGTM. Couple of minor comments. How do we plan to maintain this with mainline stargz, should we be storing our changes as a diff/patch file that is applied on top of mainline?

)

func DataToRawLine(info *RecorderInfo, path string) (b []byte, err error) {
d := map[string]string{
Copy link

Choose a reason for hiding this comment

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

Can we capture this as an interface?

r.LayerSha = layerDigest
r.ImageTag, ok = labels["containerd.io/snapshot/remote/stargz.reference"]
if !ok {
return r, fmt.Errorf("containerd.io/snapshot/remote/stargz.reference not found")
Copy link

Choose a reason for hiding this comment

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

nit - maybe these can be ErrReferenceNotFound

// Create a singleton with a sync.Once to avoid concurrent creation of the same object.
sync.OnceFunc(func() {
var err error
b10Writer, err = NewBufferedWriter("/var/b10/log", 1024*1024, 5*time.Second, 10000)
Copy link

Choose a reason for hiding this comment

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

parameterize these? Do we need to worry about log file rotation?

Copy link
Author

Choose a reason for hiding this comment

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

I need to check more in detail how does fluentd does log rotation

)

// Buffered writer that writes to a file in a buffered manner, it need to be a singleton
type BufferedWriter struct {
Copy link

@ujjwal ujjwal Feb 6, 2025

Choose a reason for hiding this comment

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

Is this functionality handled by a logger? Would using a structured logger like zero log make this code simpler? Seems like flushing, singleton, mutual exclusion semantics are handled by loggers.

Copy link
Author

Choose a reason for hiding this comment

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

I can check which logger they use, and if it provide the functionality we need

Copy link

@bolasim bolasim left a comment

Choose a reason for hiding this comment

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

I'm not as familiar with the writer/fluentd mechanism but the snapshotter/recorder part looks good.

@@ -214,6 +214,9 @@ func (r *reader) Clone(sr *io.SectionReader) (metadata.Reader, error) {
decompressor: r.decompressor,
}, nil
}
func (r *reader) GetName(id uint32) string {
Copy link

Choose a reason for hiding this comment

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

Why do we need this?

fs/layer/node.go Outdated
@@ -352,6 +356,7 @@ func (n *node) Open(ctx context.Context, flags uint32) (fh fusefs.FileHandle, fu
ra: ra,
fd: -1,
}
n.recorder.Append(n.fs.r.Metadata().GetName(n.id))
Copy link

Choose a reason for hiding this comment

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

Can you add a comment on why do it here as opposed to in OpenFile?

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