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

[Kubernetes leader election] Run leader elector at all times #4542

Merged
merged 18 commits into from
Apr 22, 2024
Merged

[Kubernetes leader election] Run leader elector at all times #4542

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
74f9da7
run leader elector at all times
constanca-m Apr 8, 2024
837afcc
add changelog fragment
constanca-m Apr 8, 2024
e56457f
Update changelog/fragments/1712583231-leader-election-issue.yaml
constanca-m Apr 8, 2024
f2399de
Reference agent issue instead of beats
constanca-m Apr 8, 2024
398fce6
Update internal/pkg/composable/providers/kubernetesleaderelection/kub…
constanca-m Apr 8, 2024
efa6d27
fix import order
constanca-m Apr 8, 2024
418026d
Add leader election unit tests
constanca-m Apr 9, 2024
d5d814d
Add leader election unit tests
constanca-m Apr 9, 2024
20b14e2
Fix lint errors
constanca-m Apr 9, 2024
2b00475
remove unit error
constanca-m Apr 9, 2024
6efdd1c
mage addLicenseHeaders
constanca-m Apr 9, 2024
ab26600
Merge branch 'main' into k8s-leader-election
constanca-m Apr 9, 2024
dafa82c
remove timeouts
constanca-m Apr 9, 2024
c4f8046
remove go routine comment
constanca-m Apr 9, 2024
3e34595
fix unit test
constanca-m Apr 9, 2024
f143a35
Add timeout
constanca-m Apr 15, 2024
b3cf472
Update changelog/fragments/1712583231-leader-election-issue.yaml
constanca-m Apr 18, 2024
90d435f
Update internal/pkg/composable/providers/kubernetesleaderelection/kub…
constanca-m Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions changelog/fragments/1712583231-leader-election-issue.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Kind can be one of:
# - breaking-change: a change to previously-documented behavior
# - deprecation: functionality that is being removed in a later release
# - bug-fix: fixes a problem in a previous version
# - enhancement: extends functionality but does not break or fix existing behavior
# - feature: new functionality
# - known-issue: problems that we are aware of in a given version
# - security: impacts on the security of a product or a user’s deployment.
# - upgrade: important information for someone upgrading from a prior version
# - other: does not fit into any of the other categories
kind: bug-fix

# Change summary; a 80ish characters long description of the change.
summary: Make sure leader elector is running at all times

# Long description; in case the summary is not enough to describe the change
# this field accommodate a description without length limits.
# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment.
#description:

# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
component: elastic-agent

# PR URL; optional; the PR number that added the changeset.
# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
# Please provide it if you are adding a fragment for a different PR.
pr: https://github.com/elastic/elastic-agent/pull/4542

# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
# If not present is automatically filled by the tooling with the issue linked to the PR number.
issue: https://github.com/elastic/beats/issues/38543
14 changes: 13 additions & 1 deletion internal/pkg/composable/providers/kubernetesleaderelection/kubernetes_leaderelection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,19 @@ func (p *contextProvider) Run(ctx context.Context, comm corecomp.ContextProvider
p.logger.Errorf("error while creating Leader Elector: %v", err)
}
p.logger.Debugf("Starting Leader Elector")
le.Run(comm)

runLeaderElector:
for {
le.Run(ctx)
Copy link
Member

Choose a reason for hiding this comment

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

If there is a cluster error resulting in the leader continuously losing the lease, will this result in attempts to acquire it as quickly as possible with no rate limit?

The implementation of Run I see is:

// Run starts the leader election loop. Run will not return
// before leader election loop is stopped by ctx or it has
// stopped holding the leader lease
func (le *LeaderElector) Run(ctx context.Context) {
	defer runtime.HandleCrash()
	defer func() {
		le.config.Callbacks.OnStoppedLeading()
	}()

	if !le.acquire(ctx) {
		return // ctx signalled done
	}
	ctx, cancel := context.WithCancel(ctx)
	defer cancel()
	go le.config.Callbacks.OnStartedLeading(ctx)
	le.renew(ctx)
}

There have been several escalations showing that failing to appropriately rate limit k8s control plane API calls like leader election can destabilize clusters.

What testing have we done to ensure this change won't cause issues like this?

Copy link
Contributor Author

@constanca-m constanca-m Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We have already been doing this. This change will not affect the number of calls, it just makes sure that at least 1 agent will be reporting metrics. The problem with the implementation now is that run goes like this:

  • Keeps trying to acquire the lease
  • Acquire the lease
  • Loose the lease and stop running
    All the while, all the other instances were trying to acquire the lease already.

We do not have many SDHs on this bug, which leads me to believe that it is rare for an agent to lose the lease. But we do have problems when agents stop reporting metrics, and the only way we knew how to restart that was to make the pod run again - that is, force run() to run again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are some parameters in the config regarding the lease:

elastic-agent/internal/pkg/composable/providers/kubernetesleaderelection/config.go

Lines 17 to 19 in 43cb148

LeaseDuration int `config:"leader_leaseduration"`
RenewDeadline int `config:"leader_renewdeadline"`
RetryPeriod int `config:"leader_retryperiod"`

If it becomes necessary to reduce the amount of times an agent tries to acquire it.

select {
case <-ctx.Done():
break runLeaderElector
default:
// Run returned because the lease was lost. Run the leader elector again, so this instance
// is still a candidate to get the lease.
}
}

p.logger.Debugf("Stopped Leader Elector")
return comm.Err()
}
Expand Down
Loading