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

Jump to bottom

Make container logs available #1

Merged
merged 1 commit into from
Apr 24, 2018
Merged

Make container logs available #1

merged 1 commit into from
Apr 24, 2018

Conversation

johanneswuerbach
Copy link

Store and retrieve container logs using Cloudwatch Logs.

Regarding the overall design, should creating and configuring an CloudWatch Log Group be required or should logs only be stored and retrievable if this is configured?

johanneswuerbach
johanneswuerbach commented Apr 20, 2018
View reviewed changes
providers/aws/fargate/cluster.go Outdated
err = client.logs.GetLogEventsPages(&cloudwatchlogs.GetLogEventsInput{
Limit: aws.Int64(int64(tail)),
LogGroupName: aws.String(c.cloudWatchLogGroupName),
LogStreamName: describeResult.LogStreams[0].LogStreamName,
Copy link
Author

Choose a reason for hiding this comment

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

There shouldn't be more then one log stream per container, but maybe we should guard this better?

@johanneswuerbach
Copy link
Author

Configuring ECS to forward logs to CloudWatch required #3 in our case.

@johanneswuerbach johanneswuerbach mentioned this pull request Apr 21, 2018
Merged
johanneswuerbach
johanneswuerbach commented Apr 21, 2018
View reviewed changes
providers/aws/fargate/cluster.go Outdated
// Due to a issue in the aws-sdk last page is never true, but the we can stop
// as soon as no further results are returned
// See https://github.com/aws/aws-sdk-ruby/pull/730
return len(page.Events) > 0
Copy link
Author

Choose a reason for hiding this comment

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

Fixed for the SDK aws/aws-sdk-go#1908

@ofiliz
Copy link
Owner

ofiliz commented Apr 23, 2018

should creating and configuring an CloudWatch Log Group be required or should logs only be stored and retrievable if this is configured?

IMO log group should be optional. Logs should only be stored and retrievable if configured.

@ofiliz ofiliz self-assigned this Apr 23, 2018
@johanneswuerbach
Copy link
Author

IMO log group should be optional. Logs should only be stored and retrievable if configured.

Make sense and changed to be optional.

ofiliz
ofiliz reviewed Apr 24, 2018
View reviewed changes
providers/aws/config.go Outdated
@@ -38,6 +38,7 @@ const (
type providerConfig struct {
Region string
ClusterName string
CloudWatchLogGroupName string
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please move this new field to be between AssignPublicIPv4Address and PlatformVersion? I am trying to keep all networking related fields (subnets, SGs, public IP address) follow cluster name.

providers/aws/config.go Outdated
@@ -130,6 +131,7 @@ func (p *FargateProvider) loadConfig(r io.Reader) error {
p.securityGroups = config.SecurityGroups

p.clusterName = config.ClusterName
p.cloudWatchLogGroupName = config.CloudWatchLogGroupName
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Please move one line below, between publicIPv4Address and platformVersion.

providers/aws/fargate.toml Outdated
@@ -14,6 +14,10 @@ Region = "us-east-1"
# we recommend to create a dedicated Fargate cluster for each virtual-kubelet.
ClusterName = "fargate1"

# AWS CloudWatch Log Group Name used to store container logs. Optional.
# If omitted, no container logs will be stored.
CloudWatchLogGroupName = "/ecs/virtual-kubelet-logs"
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Move so that this new block is between AssignPublicIPv4Address and PlatformVersion.

providers/aws/fargate/cluster.go Outdated
@@ -31,6 +33,7 @@ type Cluster struct {
arn string
subnets []string
securityGroups []string
cloudWatchLogGroupName string
assignPublicIPv4Address bool
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Move between publicIP and platformVersion.

providers/aws/fargate/cluster.go Outdated
@@ -64,6 +67,7 @@ func NewCluster(config *ClusterConfig) (*Cluster, error) {
nodeName: config.NodeName,
subnets: config.Subnets,
securityGroups: config.SecurityGroups,
cloudWatchLogGroupName: config.CloudWatchLogGroupName,
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Move between publicIP and platformVersion.

providers/aws/provider.go Outdated
@@ -30,6 +30,7 @@ type FargateProvider struct {
cluster *fargate.Cluster
clusterName string
capacity capacity
cloudWatchLogGroupName string
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Move between publicIP and platformVersion.

providers/aws/fargate/cluster.go Outdated
@@ -19,6 +20,7 @@ type ClusterConfig struct {
NodeName string
Subnets []string
SecurityGroups []string
CloudWatchLogGroupName string
Copy link
Owner

Choose a reason for hiding this comment

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

Same here. Move between publicIP and platformVersion.

providers/aws/fargate/client.go Outdated
region string
svc *ecs.ECS
api ecsiface.ECSAPI
svcLogs *cloudwatchlogs.CloudWatchLogs
Copy link
Owner

Choose a reason for hiding this comment

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

We don't need to have a separate svcLogs since we'll be only using the "logs" API interface. Can you remove it, unless there is a good reason to keep it? I know I have one "svc" for ECS above, but that was because I was testing something and I'll remove that later alsol

providers/aws/fargate/client.go Outdated
svc *ecs.ECS
api ecsiface.ECSAPI
svcLogs *cloudwatchlogs.CloudWatchLogs
logs cloudwatchlogsiface.CloudWatchLogsAPI
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should call this "logsapi". Or "logsAPI" if go lint wants it uppercase.

providers/aws/fargate/client.go Outdated
@@ -37,10 +41,12 @@ func newClient(region string) (*Client, error) {
return nil, err
}

// Create the Fargate service client.
// Create the Fargate & Cloudwatch service client.
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of adding this to existing Fargate client block, can you please create a new block?
...
// Create the Fargate service client.
...

// Create the CloudWatch service client.
client.logsAPI = cloudwatchlogs.New(session)

@johanneswuerbach
Make container logs available
9b243e4 
Store and retrieve container logs using Cloudwatch Logs
ofiliz
ofiliz approved these changes Apr 24, 2018
View reviewed changes
Copy link
Owner

@ofiliz ofiliz left a comment

Choose a reason for hiding this comment

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

Thank you!

@ofiliz ofiliz merged commit 822181f into ofiliz:aws-fargate-provider Apr 24, 2018
@johanneswuerbach johanneswuerbach deleted the af-logs branch April 24, 2018 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofiliz ofiliz ofiliz approved these changes

Assignees

@ofiliz ofiliz

Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@johanneswuerbach @ofiliz