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

Jump to bottom

Have leader election use namespace from env var #3209

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Have leader election use namespace from env var #3209

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 9 additions & 3 deletions cmd/controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ const (
kubeconfigFlag = "kubeconfig"
allocationBatchWaitTime = "allocation-batch-wait-time"
defaultResync = 30 * time.Second
podNamespace = "pod-namespace"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would just call this lease-namespace and make it clear it's the namespace to use for the controller lease. The helm chart can make sure it's aligned appropriately.

Strictly speaking, I don't think this needs to be in the same namespace as the pod - I think the controller just needs rights to it and for it to exist, which was the issue, AIUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would this be confusing since the namespace is the same one as the controller pods? Or should there be another env variable for leader election?

Copy link
Member

Choose a reason for hiding this comment

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

As a note, this doesn't (and probably shouldn't?) come from helm. We can use the downward API to get this information from K8s directly:

https://kubernetes.io/docs/concepts/workloads/pods/downward-api/

Copy link
Member

Choose a reason for hiding this comment

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

Just thinking, someone could do a yaml install to a different namespace, and expect it to work. If we use the downward API, this handles that scenario.

Copy link
Contributor Author

@chiayi chiayi Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it should be already using the downward api, looks very similar from the doc: https://github.com/googleforgames/agones/blob/main/install/helm/agones/templates/controller.yaml#L159-L162 is there a big difference between exposing it as a env variable vs a downward-api volume?

Copy link
Member

Choose a reason for hiding this comment

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

Oh sweet - I didn't realise it was already there.

If it's already there as an env var, then you are good to go!

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is already plumbed into the deployment then I have no problem with the existing name - AIUI you're basically consuming an env variable that we already set here, so I guess my lease-namespace comment is ignorable.

leaderElectionFlag = "leader-election"
)

Expand Down Expand Up @@ -249,7 +250,7 @@ func main() {
// This allows the controller to not fail health check during install when there is replication
go runRunner(server)

whenLeader(ctx, cancel, logger, ctlConf.LeaderElection, kubeClient, func(ctx context.Context) {
whenLeader(ctx, cancel, logger, ctlConf.LeaderElection, kubeClient, ctlConf.PodNamespace, func(ctx context.Context) {
kubeInformerFactory.Start(ctx.Done())
agonesInformerFactory.Start(ctx.Done())

Expand Down Expand Up @@ -282,6 +283,7 @@ func parseEnvFlags() config {
viper.SetDefault(enableStackdriverMetricsFlag, false)
viper.SetDefault(stackdriverLabels, "")
viper.SetDefault(allocationBatchWaitTime, 500*time.Millisecond)
viper.SetDefault(podNamespace, "agones-system")
viper.SetDefault(leaderElectionFlag, false)

viper.SetDefault(projectIDFlag, "")
Expand Down Expand Up @@ -315,6 +317,7 @@ func parseEnvFlags() config {
pflag.Int32(logSizeLimitMBFlag, 1000, "Log file size limit in MB")
pflag.String(logLevelFlag, viper.GetString(logLevelFlag), "Agones Log level")
pflag.Duration(allocationBatchWaitTime, viper.GetDuration(allocationBatchWaitTime), "Flag to configure the waiting period between allocations batches")
pflag.String(podNamespace, viper.GetString(podNamespace), "namespace of current pod")
pflag.Bool(leaderElectionFlag, viper.GetBool(leaderElectionFlag), "Flag to enable/disable leader election for controller pod")
cloudproduct.BindFlags()
runtime.FeaturesBindFlags()
Expand Down Expand Up @@ -344,6 +347,7 @@ func parseEnvFlags() config {
runtime.Must(viper.BindEnv(logDirFlag))
runtime.Must(viper.BindEnv(logSizeLimitMBFlag))
runtime.Must(viper.BindEnv(allocationBatchWaitTime))
runtime.Must(viper.BindEnv(podNamespace))
runtime.Must(viper.BindEnv(leaderElectionFlag))
runtime.Must(viper.BindPFlags(pflag.CommandLine))
runtime.Must(cloudproduct.BindEnv())
Expand Down Expand Up @@ -395,6 +399,7 @@ func parseEnvFlags() config {
LogSizeLimitMB: int(viper.GetInt32(logSizeLimitMBFlag)),
StackdriverLabels: viper.GetString(stackdriverLabels),
AllocationBatchWaitTime: viper.GetDuration(allocationBatchWaitTime),
PodNamespace: viper.GetString(podNamespace),
LeaderElection: viper.GetBool(leaderElectionFlag),
}
}
Expand Down Expand Up @@ -424,6 +429,7 @@ type config struct {
LogLevel string
LogSizeLimitMB int
AllocationBatchWaitTime time.Duration
PodNamespace string
LeaderElection bool
}

Expand Down Expand Up @@ -451,7 +457,7 @@ type httpServer struct {
http.ServeMux
}

func whenLeader(ctx context.Context, cancel context.CancelFunc, logger *logrus.Entry, doLeaderElection bool, kubeClient *kubernetes.Clientset, start func(_ context.Context)) {
func whenLeader(ctx context.Context, cancel context.CancelFunc, logger *logrus.Entry, doLeaderElection bool, kubeClient *kubernetes.Clientset, namespace string, start func(_ context.Context)) {
if !doLeaderElection {
start(ctx)
return
Expand All @@ -462,7 +468,7 @@ func whenLeader(ctx context.Context, cancel context.CancelFunc, logger *logrus.E
lock := &resourcelock.LeaseLock{
LeaseMeta: metav1.ObjectMeta{
Name: "agones-controller-lock",
Namespace: "agones-system",
Namespace: namespace,
},
Client: kubeClient.CoordinationV1(),
LockConfig: resourcelock.ResourceLockConfig{
Expand Down