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

Support of CreateLogGroup for awslogs #29504

Merged
merged 2 commits into from Jan 29, 2017

Conversation

Projects
None yet
6 participants
@yongtang
Member

yongtang commented Dec 17, 2016

- What I did

This fix tries to address the issue raised in #29334 where it was not possible to create log group for awslogs (CloudWatch) on-demand. Log group has to be created explicitly before container is running.

This behavior is inconsistent with AWS logs agent where log groups are always created as needed.

There were several concerns previously (See comments in #19617 and #29334):

  1. There is a limit of 500 log groups/account/region so resource might be exhausted if there is any typo or incorrect region.
  2. Logs are generated for every container so CreateLogGroup (or equally, DescribeLogGroups) might be called every time, which is redundant and potentially surprising.
  3. CreateLogStream and CreateLogGroup have different IAM policies.

- How I did it

This fix addresses the issue by add --log-opt awslogs-create-group=<true|false> which by default is false. It requires user to explicitly request that log groups be created as needed.

- How to verify it

Related unit test has been updated. And tests have also been done manually in AWS.

- Description for the changelog

Add --log-opt awslogs-create-group=<true|false> for awslogs (CloudWatch) to support creation of log groups as needed.

- A picture of a cute animal (not mandatory but encouraged)

ginger-kitten-33354-1920x1200

This fix fixes #29334.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 17, 2016

Member

/cc @consultantRR @samuelkarp @thaJeztah

Member

yongtang commented Dec 17, 2016

/cc @consultantRR @samuelkarp @thaJeztah

@samuelkarp

I think the code mostly looks reasonable, but there are a few questions we should answer:

  • Is it better to always call CreateLogGroup when awslogs-create-group=true and take a (very small) performance hit if the group already exists, or would it be better to attempt calling CreateLogStream first and only call CreateLogGroup if there is an error because the group doesn't exist? I think I would prefer the second option, since it won't require that you have the logs:CreateLogGroup permission in your IAM policy as long as the group already exists. On the other hand, if you're specifying awslogs-create-group=true and you don't have permission, that's a fairly sensible error to get.
  • The log stream allows templates. Should the log group also allow templates? I don't have a strong opinion here.
Show outdated Hide outdated daemon/logger/awslogs/cloudwatchlogs.go
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Dec 18, 2016

Member

Thanks @samuelkarp @consultantRR for the review. The PR has been updated with template supported and CreateLogStream called after CreateLogStream failure. The create() has also been moved. Please take a look and let me know if there are any issues.

Member

yongtang commented Dec 18, 2016

Thanks @samuelkarp @consultantRR for the review. The PR has been updated with template supported and CreateLogStream called after CreateLogStream failure. The create() has also been moved. Please take a look and let me know if there are any issues.

func (l *logStream) create() error {
if err := l.createLogStream(); err != nil {
if l.logCreateGroup {
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == resourceNotFoundCode {

This comment has been minimized.

@thaJeztah

thaJeztah Dec 27, 2016

Member

Is resourceNotFoundCode specific enough? Are there other possible reasons this code is returned? Alternatively, should we check if the log-group exists, and if not create it (I realise this would give a small performance penalty as well)

@thaJeztah

thaJeztah Dec 27, 2016

Member

Is resourceNotFoundCode specific enough? Are there other possible reasons this code is returned? Alternatively, should we check if the log-group exists, and if not create it (I realise this would give a small performance penalty as well)

This comment has been minimized.

@yongtang

yongtang Dec 28, 2016

Member

@thaJeztah I would assume resourceNotFoundCode (which maps to "ResourceNotFoundException" in AWS error) covers MORE reasons than log group does not exist. (I would imagine resource refers more than just log group).

Under current implementation

  1. Log stream creation, if succeed OK.
  2. If log stream creation fails with ResourceNotFoundException (multiple scenarios), attempt to create log group.
  3. If log group creation fails with ResourceAlreadyExistsException, this means log stream creation failure is not caused by log groups does not exist, retry log stream creation.

The steps above will make 1-3 AWS calls depending on different scenarios.

Alternatively, we could:

  1. Always check to see if log group exists with func (*CloudWatchLogs) DescribeLogGroups
  2. If log group exist, then go ahead with log stream creation.
  3. If log group does not exist, then go ahead with log group creation and log stream creation concurrently.

This approach will make 2-3 AWS calls depending on different scenarios.

It is hard to say which approach is better because they cover different cases. The overall average performance will depends on the ratio of different scenarios, and the cost of DescribeLogGroups.

Maybe @samuelkarp can provide some information about the likely performance preference?

@yongtang

yongtang Dec 28, 2016

Member

@thaJeztah I would assume resourceNotFoundCode (which maps to "ResourceNotFoundException" in AWS error) covers MORE reasons than log group does not exist. (I would imagine resource refers more than just log group).

Under current implementation

  1. Log stream creation, if succeed OK.
  2. If log stream creation fails with ResourceNotFoundException (multiple scenarios), attempt to create log group.
  3. If log group creation fails with ResourceAlreadyExistsException, this means log stream creation failure is not caused by log groups does not exist, retry log stream creation.

The steps above will make 1-3 AWS calls depending on different scenarios.

Alternatively, we could:

  1. Always check to see if log group exists with func (*CloudWatchLogs) DescribeLogGroups
  2. If log group exist, then go ahead with log stream creation.
  3. If log group does not exist, then go ahead with log group creation and log stream creation concurrently.

This approach will make 2-3 AWS calls depending on different scenarios.

It is hard to say which approach is better because they cover different cases. The overall average performance will depends on the ratio of different scenarios, and the cost of DescribeLogGroups.

Maybe @samuelkarp can provide some information about the likely performance preference?

This comment has been minimized.

@samuelkarp

samuelkarp Dec 29, 2016

Contributor

Calling DescribeLogGroups will require the logs:DescribeLogGroups permission in the user's IAM policy. I'd generally lean toward the current implementation as it will require fewer permissions; the only concern with the current implementation would be the possibility of engaging throttles on CreateLogGroup or CreateLogStream. I can reach out to the CloudWatch Logs team if you'd like.

@samuelkarp

samuelkarp Dec 29, 2016

Contributor

Calling DescribeLogGroups will require the logs:DescribeLogGroups permission in the user's IAM policy. I'd generally lean toward the current implementation as it will require fewer permissions; the only concern with the current implementation would be the possibility of engaging throttles on CreateLogGroup or CreateLogStream. I can reach out to the CloudWatch Logs team if you'd like.

This comment has been minimized.

@thaJeztah

thaJeztah Dec 29, 2016

Member

Calling DescribeLogGroups will require the logs:DescribeLogGroups permission in the user's IAM policy

Ah, right, so checking if a group exists actually requires different permissions than using a group. Hm, yes, In that case we should go with the current implementation.

I was looking if we could reduce the amount of nested if's, for example

	err := l.createLogStream()
	if err == nil {
		return nil
	}

But looking at that, I'm not sure it improves readability, so 😄

@thaJeztah

thaJeztah Dec 29, 2016

Member

Calling DescribeLogGroups will require the logs:DescribeLogGroups permission in the user's IAM policy

Ah, right, so checking if a group exists actually requires different permissions than using a group. Hm, yes, In that case we should go with the current implementation.

I was looking if we could reduce the amount of nested if's, for example

	err := l.createLogStream()
	if err == nil {
		return nil
	}

But looking at that, I'm not sure it improves readability, so 😄

This comment has been minimized.

@samuelkarp

samuelkarp Dec 30, 2016

Contributor

Ah, right, so checking if a group exists actually requires different permissions than using a group.

Yes. You can see the IAM permissions available for CloudWatch Logs in the developer guide. In general, AWS services tend to allow policies that specify individual APIs to access, so that different scenarios can be supported. One relevant example that fits both logs and metrics is write-only, so that a host which is producing metrics or producing logs cannot read metrics or logs from other resources in the same account.

@samuelkarp

samuelkarp Dec 30, 2016

Contributor

Ah, right, so checking if a group exists actually requires different permissions than using a group.

Yes. You can see the IAM permissions available for CloudWatch Logs in the developer guide. In general, AWS services tend to allow policies that specify individual APIs to access, so that different scenarios can be supported. One relevant example that fits both logs and metrics is write-only, so that a host which is producing metrics or producing logs cannot read metrics or logs from other resources in the same account.

@thaJeztah

LGTM, but left one suggestion for booleans (I think we use ParseBool in other locations, in which case it could be consistent)

Support of CreateLogGroup for awslogs
This fix tries to address the issue raised in 29344 where it was
not possible to create log group for awslogs (CloudWatch) on-demand.
Log group has to be created explicitly before container is running.

This behavior is inconsistent with AWS logs agent where log groups
are always created as needed.

There were several concerns previously (See comments in 19617 and 29344):
1. There is a limit of 500 log groups/account/region so resource might
be exhausted if there is any typo or incorrect region.
2. Logs are generated for every container so CreateLogGroup (or equally,
DescribeLogGroups) might be called every time, which is redundant and
potentially surprising.
3. CreateLogStream and CreateLogGroup have different IAM policies.

This fix addresses the issue by add `--log-opt awslogs-create-group`
which by default is `false`. It requires user to explicitly request
that log groups be created as needed.

Related unit test has been updated. And tests have also been done
manually in AWS.

This fix fixes 29334.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 27, 2017

Contributor

LGTM
@yongtang You also need to change contrib/completion/bash/docker and contrib/completion/zsh/_docker. I don't see real docs for this, so it's only completion.
Thanks!

Contributor

LK4D4 commented Jan 27, 2017

LGTM
@yongtang You also need to change contrib/completion/bash/docker and contrib/completion/zsh/_docker. I don't see real docs for this, so it's only completion.
Thanks!

Update bash and zsh completion for aws-create-group
This commit updates bash and zsh completion for aws-create-group.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 27, 2017

Member

Thanks @LK4D4. The PR has been updated with bash and zsh completion. Please take a look.

Member

yongtang commented Jan 27, 2017

Thanks @LK4D4. The PR has been updated with bash and zsh completion. Please take a look.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Jan 28, 2017

@thaJeztah thaJeztah merged commit 8820d0a into moby:master Jan 29, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30057 has succeeded
Details
janky Jenkins build Docker-PRs 38670 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9696 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 29, 2017

@ProbablyRusty

This comment has been minimized.

Show comment
Hide comment
@ProbablyRusty

ProbablyRusty Jan 29, 2017

Great job, folks.

ProbablyRusty commented Jan 29, 2017

Great job, folks.

@yongtang yongtang deleted the yongtang:29334-awslogs-CreateLogGroup branch Jan 29, 2017

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Feb 7, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to mistyhacks/docker.github.io that referenced this pull request Feb 24, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Mar 8, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Mar 17, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

JimGalasyn pushed a commit to docker/docker.github.io that referenced this pull request Mar 23, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Mar 30, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Mar 31, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Apr 5, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Apr 5, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block

mistyhacks added a commit to docker/docker.github.io that referenced this pull request Apr 6, 2017

Update documentation for CreateLogGroup support in awslogs (#1423)
* Update documentation for CreateLogGroup support in awslogs

This fix updates the documetation for CreateLogGroup support in awslogs

This fix is related to
moby/moby#29334
moby/moby#29504

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* Improved line wrapping of code block
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment