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 containers log saver extension #192

Merged
merged 4 commits into from
Mar 22, 2021

Conversation

Tiberivs
Copy link
Contributor

Add logs extension for collecting logs from containers.
Needed for networkservicemesh/integration-k8s-kind#18.

Signed-off-by: Anatoly Loskutnikov anatoly.loskutnikov@gmail.com

extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
panic(err)
}

if len(result) > 0 {
Copy link
Member

@denis-tingaikin denis-tingaikin Mar 11, 2021

Choose a reason for hiding this comment

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

Also, we should check that it is not only spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is no make sense, because each log entry contains timestamp at least. Moreover it can significantly increase processing time without obvious advantage.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that kuberntes parses our logs... Please see https://kubernetes.io/docs/concepts/cluster-administration/logging/

Moreover, we cannot guarantee that all apps in our examples will use our logging system.

Moreover it can significantly increase processing time without obvious advantage.

How we can increase processing time with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, of course we can improve filtering. But I think fully whitespaced log lines produced by NSM is an undesirable behavior and rather should be fixed, not filtered.

Copy link
Member

Choose a reason for hiding this comment

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

Note: We'll test also the external clients and we don't know what they can do :)

I also think that check on whitespaces can be faster than save a file with whitespaces.

This can be considered in a separate PR.

@Tiberivs Tiberivs force-pushed the save-logs branch 2 times, most recently from d986d7c to 192669d Compare March 12, 2021 11:26
extensions/base/suite.go Outdated Show resolved Hide resolved
extensions/base/suite.go Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/multisuite/suite.go Outdated Show resolved Hide resolved
Comment on lines 33 to 37
func (s *Suite) WithSuits(ifaces ...interface{}) {
s.suits = make([]interface{}, len(ifaces))
for i := range ifaces {
s.suits[i] = ifaces[i]
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func (s *Suite) WithSuits(ifaces ...interface{}) {
s.suits = make([]interface{}, len(ifaces))
for i := range ifaces {
s.suits[i] = ifaces[i]
}
}
func (s *Suite) New(suits ...suite.TestingSuite) *Suite {
return &Suite{
suits: suits
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@Tiberivs Tiberivs force-pushed the save-logs branch 4 times, most recently from 3f6db58 to c5117a0 Compare March 18, 2021 10:21
Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

This looks better, but there are things that can be improved.

extensions/logs/suite.go Outdated Show resolved Hide resolved
extensions/multisuite/suite.go Outdated Show resolved Hide resolved
extensions/logs/suite.go Outdated Show resolved Hide resolved
Comment on lines 66 to 81
func init() {
if err := envconfig.Usage("", &config); err != nil {
panic(err)
}

if err := envconfig.Process("", &config); err != nil {
panic(err)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use once and do lazy initialization. Also, we can do there setup for the worker pool.

extensions/logs/suite.go Outdated Show resolved Hide resolved

testStartTime time.Time
nsmContainers map[types.UID]bool
logQueue chan logItem
Copy link
Member

@denis-tingaikin denis-tingaikin Mar 18, 2021

Choose a reason for hiding this comment

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

Should we use one worker pool for all suits?

@Tiberivs Tiberivs force-pushed the save-logs branch 2 times, most recently from 0fe2a2e to 635c386 Compare March 19, 2021 04:05
Signed-off-by: Anatoly Loskutnikov <anatoly.loskutnikov@gmail.com>
Signed-off-by: Anatoly Loskutnikov <anatoly.loskutnikov@gmail.com>
Signed-off-by: Anatoly Loskutnikov <anatoly.loskutnikov@gmail.com>
Signed-off-by: Anatoly Loskutnikov <anatoly.loskutnikov@gmail.com>
@denis-tingaikin denis-tingaikin merged commit 5dea1d6 into networkservicemesh:main Mar 22, 2021
nsmbot pushed a commit that referenced this pull request Mar 23, 2021
…ployments-k8s@main networkservicemesh/deployments-k8s#

networkservicemesh/deployments-k8s PR link: https://github.com/networkservicemesh/deployments-k8s/pull/

networkservicemesh/deployments-k8s commit message:
commit 5dea1d6
Author: Denis Tingaikin <49399980+denis-tingaikin@users.noreply.github.com>
Date:   Mon Mar 22 15:01:40 2021 +0700

    Merge pull request #192 from Tiberivs/save-logs

    Add containers log saver extension

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
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.

2 participants