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

Manually create user log files #3903

Merged
merged 4 commits into from
Jul 31, 2018
Merged

Conversation

dubee
Copy link
Member

@dubee dubee commented Jul 25, 2018

Description

Do not rely on LogRotatorSink to create user log files. Instead manually create the user log files with custom permissions.

Related issue and scope

  • I opened an issue to propose and discuss this change (#????)

My changes affect the following components

  • API
  • Controller
  • Message Bus (e.g., Kafka)
  • Loadbalancer
  • Invoker
  • Intrinsic actions (e.g., sequences, conductors)
  • Data stores (e.g., CouchDB)
  • Tests
  • Deployment
  • CLI
  • General tooling
  • Documentation

Types of changes

  • Bug fix (generally a non-breaking change which closes an issue).
  • Enhancement or new feature (adds new functionality).
  • Breaking change (a bug fix or enhancement which changes existing behavior).

Checklist:

  • I signed an Apache CLA.
  • I reviewed the style guides and followed the recommendations (Travis CI will check :).
  • I added tests to cover my changes.
  • My changes require further changes to the documentation.
  • I updated the documentation where necessary.

@codecov-io
Copy link

codecov-io commented Jul 25, 2018

Codecov Report

Merging #3903 into master will decrease coverage by 4.65%.
The diff coverage is 11.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3903      +/-   ##
==========================================
- Coverage    75.9%   71.25%   -4.66%     
==========================================
  Files         147      145       -2     
  Lines        7048     7045       -3     
  Branches      422      436      +14     
==========================================
- Hits         5350     5020     -330     
- Misses       1698     2025     +327
Impacted Files Coverage Δ
...rpool/logging/DockerToActivationFileLogStore.scala 68.08% <11.11%> (-7.53%) ⬇️
...core/database/cosmosdb/RxObservableImplicits.scala 0% <0%> (-100%) ⬇️
...core/database/cosmosdb/CosmosDBArtifactStore.scala 0% <0%> (-95.1%) ⬇️
...sk/core/database/cosmosdb/CosmosDBViewMapper.scala 0% <0%> (-92.6%) ⬇️
...whisk/core/database/cosmosdb/CosmosDBSupport.scala 0% <0%> (-81.82%) ⬇️
...abase/cosmosdb/CosmosDBArtifactStoreProvider.scala 0% <0%> (-58.83%) ⬇️
...la/whisk/core/database/cosmosdb/CosmosDBUtil.scala 92% <0%> (-4%) ⬇️
...n/scala/src/main/scala/whisk/common/DateUtil.scala
...scala/src/main/scala/whisk/common/TimingUtil.scala

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7000687...bb94f0d. Read the comment docs.

@@ -75,6 +85,8 @@ class DockerToActivationFileLogStore(system: ActorSystem, destinationDirectory:
* once the defined limit is reached.
*/
val bufferSize = 100.MB
val perms = java.util.EnumSet.of(OWNER_READ, OWNER_WRITE, GROUP_READ, GROUP_WRITE, OTHERS_READ, OTHERS_WRITE)
val attr = PosixFilePermissions.asFileAttribute(perms)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more restrictive set of permissions?

Copy link
Member Author

Choose a reason for hiding this comment

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

The permissions in the current code base are too restrictive. Perhaps the OTHERS_WRITE can be removed here though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Permissions from current code base are OWNER_READ, OWNER_WRITE, GROUP_READ , OTHERS_READ. We need GROUP_WRITE for a log issue.

@dubee dubee force-pushed the user-log-create branch 2 times, most recently from d4699a2 to 0d5b204 Compare July 25, 2018 21:04
@dubee
Copy link
Member Author

dubee commented Jul 26, 2018

PG1 3181 🔵

val logFilePath = destinationDirectory.resolve(s"userlogs-${Instant.now.toEpochMilli}.log")
logging.info(this, s"Rotating log file to '$logFilePath'")
Files.createFile(logFilePath, attr)
Files.setPosixFilePermissions(logFilePath, perms)
Copy link
Contributor

Choose a reason for hiding this comment

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

Both of these can throw an IOException. I'm assuming such an error would cause the wrapping RetrySink to trigger a restart? Should we make sure the message is logged accordingly?

These two lines seem to be redundant, aren't the parameters already passed in the createFile call?

Copy link
Member Author

@dubee dubee Jul 30, 2018

Choose a reason for hiding this comment

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

createFile does not properly set the file permissions, relying on setPosixFilePermissions instead.

I threw an error from inside the LogRotatorSink function, the RestartSink did indeed attempt to recreate the log file.

@dubee dubee force-pushed the user-log-create branch 2 times, most recently from e6f7ea2 to c3c7d29 Compare July 30, 2018 18:29
@dubee dubee added review Review for this PR has been requested and yet needs to be done. awaits-reviewer The reviewer needs to respond to comments from contributer. labels Jul 30, 2018
Copy link
Contributor

@markusthoemmes markusthoemmes left a comment

Choose a reason for hiding this comment

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

LGTM

@markusthoemmes markusthoemmes merged commit bcd32ed into apache:master Jul 31, 2018
BillZong pushed a commit to BillZong/openwhisk that referenced this pull request Nov 18, 2019
Do not rely on LogRotatorSink to create user log files. Instead manually create the user log files with custom permissions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits-reviewer The reviewer needs to respond to comments from contributer. review Review for this PR has been requested and yet needs to be done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants