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

Change logserver Deployment update strategy to Recreate #13423

Merged
merged 6 commits into from May 22, 2021

Conversation

jiacheliu3
Copy link
Contributor

Addresses #13422

@jiacheliu3
Copy link
Contributor Author

FYI @nirav-chotai

@codecov-commenter
Copy link

codecov-commenter commented May 15, 2021

Codecov Report

Merging #13423 (eb4a9ea) into master (9f5673e) will decrease coverage by 0.24%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #13423      +/-   ##
============================================
- Coverage     43.80%   43.56%   -0.25%     
+ Complexity     8956     8952       -4     
============================================
  Files          1361     1366       +5     
  Lines         78630    79316     +686     
  Branches       9546     9631      +85     
============================================
+ Hits          34443    34553     +110     
- Misses        41277    41846     +569     
- Partials       2910     2917       +7     
Impacted Files Coverage Δ Complexity Δ
...a/alluxio/exception/status/CancelledException.java 0.00% <0.00%> (-42.86%) 0.00% <0.00%> (-2.00%)
...e/common/src/main/java/alluxio/AbstractClient.java 59.86% <0.00%> (-12.50%) 20.00% <0.00%> (-4.00%)
.../src/main/java/alluxio/retry/TimeBoundedRetry.java 84.61% <0.00%> (-11.54%) 7.00% <0.00%> (ø%)
.../alluxio/master/journal/AbstractJournalSystem.java 51.21% <0.00%> (-10.55%) 7.00% <0.00%> (ø%)
...uxio/master/FaultTolerantAlluxioMasterProcess.java 48.95% <0.00%> (-8.34%) 10.00% <0.00%> (-4.00%)
...on/src/main/java/alluxio/collections/LockPool.java 74.03% <0.00%> (-6.74%) 15.00% <0.00%> (-1.00%)
...thentication/AuthenticatedChannelServerDriver.java 85.71% <0.00%> (-5.36%) 11.00% <0.00%> (-1.00%)
...ain/java/alluxio/worker/block/BlockMasterSync.java 60.65% <0.00%> (-4.92%) 6.00% <0.00%> (ø%)
...c/main/java/alluxio/heartbeat/HeartbeatThread.java 70.00% <0.00%> (-3.34%) 5.00% <0.00%> (-1.00%)
...c/main/java/alluxio/proxy/AlluxioProxyProcess.java 62.79% <0.00%> (-2.33%) 8.00% <0.00%> (-1.00%)
... and 43 more

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 9f5673e...eb4a9ea. Read the comment docs.

Copy link
Contributor

@madanadit madanadit left a comment

Choose a reason for hiding this comment

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

Thx!

@nirav-chotai
Copy link
Contributor

@jiacheliu3 , this should not be hardcoded but an option within values.yaml with the default being Recreate

Note should be added:

  • If you're using PVC with RWO access mode, then keep the strategy to Recreate. In this case, logserver will be down for some time and logs will be lost for that period of time.
  • If you're using PVC with RWX access mode, then keep the strategy to RollingUpdate In this case, logserver will be up and running all the time, so logs will not be lost even while updating logserver.

In our DEV environment, I am planning to keep Recreate strategy with CephRBD and RWO access mode. However, in the PROD environment, I am going to use RollingUpdate strategy with CephFS and RWX access mode.

Copy link
Contributor

@ZhuTopher ZhuTopher left a comment

Choose a reason for hiding this comment

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

LGTM

@LuQQiu
Copy link
Contributor

LuQQiu commented May 20, 2021

@jiacheliu3 can this PR be merged?

@jiacheliu3
Copy link
Contributor Author

@jiacheliu3 can this PR be merged?

@LuQQiu As Nirav mentioned, I think it makes sense to make this a configuration instead of hard code. I will do that and request for another round of reviews.

@jiacheliu3
Copy link
Contributor Author

@ZhuTopher @madanadit Please take another look after I extracted the field to a configuration. Thanks!

Copy link
Contributor

@ZhuTopher ZhuTopher left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@madanadit madanadit left a comment

Choose a reason for hiding this comment

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

Thanks @jiacheliu3

@jiacheliu3
Copy link
Contributor Author

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit fac8df1 into Alluxio:master May 22, 2021
yuzhu pushed a commit to yuzhu/alluxio that referenced this pull request Jun 5, 2021
Addresses Alluxio#13422

pr-link: Alluxio#13423
change-id: cid-dd83b35b1e2e3a3a8fdbd8a7a475cbceef794397
Xenorith pushed a commit to Xenorith/alluxio that referenced this pull request Jun 5, 2021
Addresses Alluxio#13422

pr-link: Alluxio#13423
change-id: cid-dd83b35b1e2e3a3a8fdbd8a7a475cbceef794397
alluxio-bot pushed a commit that referenced this pull request Jun 8, 2021
### What changes were proposed in this pull request?
This PR proposes some additions to the [Alluxio k8s
guide](https://docs.alluxio.io/os/user/stable/en/deploy/Running-Alluxio-On-Kubernetes.html)
to reflect some of the recent additions to the Alluxio Helm chart:

- `serviceAccount`: #13297
- `tolerations`: #13214
- `hostAliases`: #13226
- `strategy`: #13423

### Why are the changes needed?
Doc validation

### Does this PR introduce _any_ user-facing change?
Yes, the documentation.

pr-link: #13579
change-id: cid-cd456e708547bb121cdab5df1de7599860e6e089
alluxio-bot pushed a commit that referenced this pull request Jun 8, 2021
### What changes were proposed in this pull request?
This PR proposes some additions to the [Alluxio k8s
guide](https://docs.alluxio.io/os/user/stable/en/deploy/Running-Alluxio-On-Kubernetes.html)
to reflect some of the recent additions to the Alluxio Helm chart:

- `serviceAccount`: #13297
- `tolerations`: #13214
- `hostAliases`: #13226
- `strategy`: #13423

### Why are the changes needed?
Doc validation

### Does this PR introduce _any_ user-facing change?
Yes, the documentation.

pr-link: #13579
change-id: cid-cd456e708547bb121cdab5df1de7599860e6e089
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.

None yet

7 participants