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

Auto-drain #6

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Auto-drain #6

wants to merge 17 commits into from

Conversation

cenkalti
Copy link
Member

@cenkalti cenkalti commented Dec 3, 2020

No description provided.

* master:
  fix drain test
  do not check tempfile on head
  fix docker setup
  fix fid path parsing
  add rabbitmq addon
  check tempfile on patch
@cenkalti cenkalti self-assigned this Dec 3, 2020
@coveralls
Copy link

coveralls commented Dec 3, 2020

Coverage Status

Coverage decreased (-1.4%) to 46.963% when pulling 71f5023 on balance into 6b6d2a8 on master.

@cenkalti cenkalti changed the title Auto-drain [WIP] Auto-drain Dec 4, 2020
@cenkalti cenkalti requested review from BatuAksoy and muraty December 4, 2020 00:30
autodrain.go Outdated Show resolved Hide resolved
@cenkalti cenkalti requested a review from BatuAksoy December 5, 2020 12:47
@cenkalti
Copy link
Member Author

cenkalti commented Dec 5, 2020

@BatuAksoy made some changes after your review.

@muraty PR is ready now.

autodrain.go Show resolved Hide resolved
autodrain.go Outdated Show resolved Hide resolved
func (s *Server) autoDrain() {
defer close(s.autoDrainStopped)

// Drainer is written for "efes drain" command but can be reused here because we need same functionality here.
Copy link
Member

Choose a reason for hiding this comment

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

I guess we won't need this comment anymore.

@@ -204,34 +202,6 @@ func main() {
return nil
},
},
{
Name: "drain",
Copy link
Member

Choose a reason for hiding this comment

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

Since we delete this, we won't need Run function of Drainer struct anymore, right?

Copy link
Member

Choose a reason for hiding this comment

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

Except drain_test.go file.

// Auto-drain period is a window that only certain number of servers are allowed to run auto-drain operations.
// When the period advances to the next one, servers currently running auto-drain will stop and some other
// servers are going to run auto-drain.
AutoDrainRunPeriod Duration `toml:"auto_drain_run_period"`
Copy link
Member

Choose a reason for hiding this comment

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

@cenkalti how to find appropriate value for this config?

Let's say;

  • We have 100 servers
  • 50 of them exceeds AutoDrainThreshold
  • AutoDrainDeviceRatio is set to 10

In this case, if we set AutoDrainRunPeriod as 1 hour, 5 servers will be drained every 1 hour. So, auto-drain will be finished in 10 hours for 50 servers. Is the calculation correct?

If there is small difference for the servers those exceeds AutoDrainThreshold and those don't, 1 hour might be a high value. Maybe we should use less than that? Or should we find appropriate AutoDrainRunPeriod config with run & monitor method?

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.

4 participants