fix(app): Introduced flag to specify leader election behavious #2809

CharlesQQ · 2023-04-21T09:40:55Z

resolve: #2798

volcano-sh-bot · 2023-04-21T09:41:05Z

Welcome @CharlesQQ! It looks like this is your first PR to volcano-sh/volcano 🎉

lowang-bh · 2023-04-21T10:40:54Z

I think we should do some checks between those three parameters

CharlesQQ · 2023-05-04T01:47:46Z

cc @wangyang0616

wangyang0616 · 2023-05-04T02:28:38Z

I agree with @lowang-bh is suggestion that the input parameters need to be verified.

CharlesQQ · 2023-05-04T03:10:38Z

@lowang-bh @wangyang0616
there have the checks for three parameter in leaderelection package, so no need to check again.

volcano/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go

Lines 76 to 109 in 86ff941

    
           // NewLeaderElector creates a LeaderElector from a LeaderElectionConfig 
        
           func NewLeaderElector(lec LeaderElectionConfig) (*LeaderElector, error) { 
        
           	if lec.LeaseDuration <= lec.RenewDeadline { 
        
           		return nil, fmt.Errorf("leaseDuration must be greater than renewDeadline") 
        
           	} 
        
           	if lec.RenewDeadline <= time.Duration(JitterFactor*float64(lec.RetryPeriod)) { 
        
           		return nil, fmt.Errorf("renewDeadline must be greater than retryPeriod*JitterFactor") 
        
           	} 
        
           	if lec.LeaseDuration < 1 { 
        
           		return nil, fmt.Errorf("leaseDuration must be greater than zero") 
        
           	} 
        
           	if lec.RenewDeadline < 1 { 
        
           		return nil, fmt.Errorf("renewDeadline must be greater than zero") 
        
           	} 
        
           	if lec.RetryPeriod < 1 { 
        
           		return nil, fmt.Errorf("retryPeriod must be greater than zero") 
        
           	} 
        
           	if lec.Callbacks.OnStartedLeading == nil { 
        
           		return nil, fmt.Errorf("OnStartedLeading callback must not be nil") 
        
           	} 
        
           	if lec.Callbacks.OnStoppedLeading == nil { 
        
           		return nil, fmt.Errorf("OnStoppedLeading callback must not be nil") 
        
           	} 
        
           	if lec.Lock == nil { 
        
           		return nil, fmt.Errorf("Lock must not be nil.") 
        
           	} 
        
           	le := LeaderElector{ 
        
           		config:  lec, 
        
           		clock:   clock.RealClock{}, 
        
           		metrics: globalMetricsFactory.newLeaderMetrics(), 
        
           	} 
        
           	le.metrics.leaderOff(le.config.Name) 
        
           	return &le, nil

CharlesQQ · 2023-07-04T01:00:36Z

cc @wangyang0616

wangyang0616 · 2023-08-11T09:22:53Z

cmd/controller-manager/app/options/options.go

@@ -83,6 +93,19 @@ func (s *ServerOption) AddFlags(fs *pflag.FlagSet) {
 	fs.BoolVar(&s.EnableLeaderElection, "leader-elect", true, "Start a leader election client and gain leadership before "+
 		"executing the main loop. Enable this when running replicated vc-controller-manager for high availability; it is enabled by default")
 	fs.StringVar(&s.LockObjectNamespace, "lock-object-namespace", defaultLockObjectNamespace, "Define the namespace of the lock object; it is volcano-system by default")
+	fs.DurationVar(&s.LeaseDuration, "leader-elect-lease-duration", defaultElectionLeaseDuration, ""+


I'm thinking here mainly about the string here, whether to comply with the 120-character requirement, or to support legibility, is it easier to read if the two lines of text description information are closed?

I'm thinking here mainly about the string here, whether to comply with the 120-character requirement, or to support legibility, is it easier to read if the two lines of text description information are closed?

It's same as kubernetes comments, sames that comply with the 120-character requirement
https://github.com/kubernetes/kubernetes/blob/ea30d100f6b13a47b4153362698b89256077776c/staging/src/k8s.io/component-base/config/options/leaderelectionconfig.go#L30C1-L42

stale · 2023-12-15T05:17:07Z

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

CharlesQQ · 2024-03-25T03:01:18Z

/reopen

volcano-sh-bot · 2024-03-25T03:01:29Z

@CharlesQQ: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Monokaix · 2024-03-25T03:32:39Z

cmd/controller-manager/app/options/options.go

@@ -47,6 +54,9 @@ type ServerOption struct {
 	CaCertData           []byte
 	EnableLeaderElection bool


Leader election related parameters can be placed in an independent flagset, please refer to kubernets to make it more graceful.

@Monokaix updated! pls check again.

Monokaix · 2024-03-27T02:14:30Z

cmd/scheduler/app/options/options.go

@@ -195,10 +192,10 @@ func (s *ServerOption) ParseCAFiles(decryptFunc DecryptFunc) error {
 	return nil
 }

-// Default new and registry a default one


Why is this commented?

Why is this commented?

useless code, I will delete that.

Monokaix · 2024-03-27T02:18:51Z

pkg/util/util.go

@@ -32,3 +47,31 @@ func GenerateSchedulerName(schedulerNames []string) string {

 	return defaultSchedulerName
 }
+
+// BindLeaderElectionFlags binds the LeaderElectionConfiguration struct fields to a flagset
+func BindLeaderElectionFlags(l *config.LeaderElectionConfiguration, fs *pflag.FlagSet) {


How about import k8s.io/component-base/config/options/leaderelectionconfig.go directly?

How about import k8s.io/component-base/config/options/leaderelectionconfig.go directly?

en.., I want set default value in function BindLeaderElectionFlags; if not, must add a function to set LeaderElectionConfiguration default values.

But actually it's a copy from k8s: )

lowang-bh · 2024-03-27T02:24:16Z

cmd/controller-manager/app/options/options.go

-	CertData             []byte
-	KeyData              []byte
-	CaCertData           []byte
-	EnableLeaderElection bool


How about keeping the old one for better compatibility?

leader-elect flag is kept when using BindLeaderElectionFlags.

How about keeping the old one for better compatibility?

Use leaderElection flagSet instead, it's a leaderElectionOption field.

But lock-object-namespace flag seems has been removed.

But lock-object-namespace flag seems has been removed.

use the leader-elect-resource-namespace flag instead

So plz also update params in helm : )

I think we should better keep the old flag and mark it deprecated first.

So plz also update params in helm I think we should better keep the old flag and mark it deprecated first.

sure!

I think we should better keep the old flag and mark it deprecated first.

I also means keeping the old one and marking it is deprecated in recent release.

lowang-bh · 2024-03-27T02:27:08Z

cmd/scheduler/app/options/options.go

-	return s
-}
+//// Default new and registry a default one
+//func Default() *ServerOption {


Please keep it. It is used in other test packages.

Please keep it. It is used in other test packages.

all right!

Monokaix · 2024-03-27T02:29:26Z

cmd/controller-manager/app/options/options.go

 	"github.com/spf13/pflag"
+	"k8s.io/component-base/config"
+	"os"


Plz sort imports.

Plz sort imports.

done!

Monokaix · 2024-03-27T06:54:05Z

pkg/util/util.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/tools/leaderelection/resourcelock"
+	"k8s.io/component-base/config"
+	"time"


sort imports.

Monokaix · 2024-03-27T06:55:14Z

cmd/scheduler/app/options/options.go

@@ -147,7 +144,7 @@ func (s *ServerOption) AddFlags(fs *pflag.FlagSet) {

 // CheckOptionOrDie check lock-object-namespace when LeaderElection is enabled.
 func (s *ServerOption) CheckOptionOrDie() error {
-	if s.EnableLeaderElection && s.LockObjectNamespace == "" {
+	if s.LeaderElection.LeaderElect && s.LeaderElection.ResourceLock == "" {
 		return fmt.Errorf("lock-object-namespace must not be nil when LeaderElection is enabled")


Change the description too.

Monokaix · 2024-03-27T07:04:46Z

cmd/controller-manager/app/options/options.go

@@ -102,7 +100,7 @@ func (s *ServerOption) AddFlags(fs *pflag.FlagSet) {

 // CheckOptionOrDie checks the LockObjectNamespace.
 func (s *ServerOption) CheckOptionOrDie() error {
-	if s.EnableLeaderElection && s.LockObjectNamespace == "" {
+	if s.LeaderElection.LeaderElect && s.LeaderElection.ResourceLock == "" {


Here we can call ValidateLeaderElectionConfiguration to check all params.

All that is updated!

Monokaix · 2024-03-27T08:50:13Z

cmd/controller-manager/main.go

+
+	s.AddFlags(fs)
+	commonutil.LeaderElectionDefault(&s.LeaderElection)
+	componentbaseoptions.BindLeaderElectionFlags(&s.LeaderElection, fs)



Just mark LockObjectNamespace deprecated is not enough, we should set ResourceNamespace value to LockObjectNamespace when it's not empty: )

Just mark LockObjectNamespace deprecated is not enough, we should set ResourceNamespace value to LockObjectNamespace when it's not empty: )

All right！

lowang-bh · 2024-03-28T09:23:52Z

cmd/controller-manager/app/options/options.go

-	CertData             []byte
-	KeyData              []byte
-	CaCertData           []byte
-	EnableLeaderElection bool


I think we should better keep the old flag and mark it deprecated first.

I also means keeping the old one and marking it is deprecated in recent release.

lowang-bh · 2024-03-28T09:25:40Z

cmd/controller-manager/app/options/options.go

@@ -84,9 +89,7 @@ func (s *ServerOption) AddFlags(fs *pflag.FlagSet) {
 		"File containing the default x509 Certificate for HTTPS. (CA cert, if any, concatenated "+
 		"after server cert).")
 	fs.StringVar(&s.KeyFile, "tls-private-key-file", s.KeyFile, "File containing the default x509 private key matching --tls-cert-file.")
-	fs.BoolVar(&s.EnableLeaderElection, "leader-elect", true, "Start a leader election client and gain leadership before "+


why this is removed?

lowang-bh · 2024-03-28T09:26:26Z

cmd/scheduler/app/options/options.go

 	// volcano scheduler will ignore pods with scheduler names other than specified with the option
 	fs.StringArrayVar(&s.SchedulerNames, "scheduler-name", []string{defaultSchedulerName}, "vc-scheduler will handle pods whose .spec.SchedulerName is same as scheduler-name")
 	fs.StringVar(&s.SchedulerConf, "scheduler-conf", "", "The absolute path of scheduler configuration file")
 	fs.DurationVar(&s.SchedulePeriod, "schedule-period", defaultSchedulerPeriod, "The period between each scheduling cycle")
 	fs.StringVar(&s.DefaultQueue, "default-queue", defaultQueue, "The default queue name of the job")
-	fs.BoolVar(&s.EnableLeaderElection, "leader-elect", true,


why this is removed, how about deprecated？

New flagset contains this flag, so it's not removed autually.

Signed-off-by: chang.qiangqiang <chang.qiangqiang@immomo.com>

Monokaix · 2024-04-01T11:11:29Z

/lgtm

william-wang

/approve

volcano-sh-bot · 2024-04-01T12:05:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: william-wang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [william-wang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

volcano-sh-bot requested review from jiangkaihua and wpeng102 April 21, 2023 09:40

volcano-sh-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 21, 2023

CharlesQQ force-pushed the leader-elect branch from 29a09dc to cb101df Compare April 21, 2023 09:42

CharlesQQ changed the title ~~fix(app): tntroduced flag to specify leader election behavious~~ fix(app): Introduced flag to specify leader election behavious Apr 21, 2023

CharlesQQ force-pushed the leader-elect branch from cb101df to 79998c3 Compare April 21, 2023 09:47

CharlesQQ force-pushed the leader-elect branch from 79998c3 to 03b2f65 Compare June 30, 2023 10:39

CharlesQQ force-pushed the leader-elect branch from 03b2f65 to 19a0405 Compare August 8, 2023 10:34

wangyang0616 reviewed Aug 11, 2023

View reviewed changes

stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 15, 2023

stale bot closed this Mar 17, 2024

volcano-sh-bot reopened this Mar 25, 2024

stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 25, 2024

Monokaix reviewed Mar 25, 2024

View reviewed changes

CharlesQQ force-pushed the leader-elect branch from 6f48931 to 9bb40ee Compare March 25, 2024 04:06

CharlesQQ requested a review from Monokaix March 25, 2024 04:10

CharlesQQ force-pushed the leader-elect branch from 9bb40ee to 3c66531 Compare March 26, 2024 06:44

volcano-sh-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2024

Monokaix reviewed Mar 27, 2024

View reviewed changes

lowang-bh reviewed Mar 27, 2024

View reviewed changes

Monokaix reviewed Mar 27, 2024

View reviewed changes

CharlesQQ force-pushed the leader-elect branch 4 times, most recently from 7f4fff8 to 0e650a5 Compare March 27, 2024 03:15

Monokaix reviewed Mar 27, 2024

View reviewed changes

CharlesQQ force-pushed the leader-elect branch 2 times, most recently from e8bba62 to e095df9 Compare March 27, 2024 07:39

Monokaix reviewed Mar 27, 2024

View reviewed changes

CharlesQQ force-pushed the leader-elect branch 3 times, most recently from f6892ba to 2913c74 Compare March 28, 2024 03:46

lowang-bh reviewed Mar 28, 2024

View reviewed changes

CharlesQQ force-pushed the leader-elect branch from 849d9e7 to 6afc0b4 Compare April 1, 2024 07:01

fix(app): Introduced flag to specify leader election behavious

0640aa6

Signed-off-by: chang.qiangqiang <chang.qiangqiang@immomo.com>

CharlesQQ force-pushed the leader-elect branch from 6afc0b4 to 0640aa6 Compare April 1, 2024 07:03

volcano-sh-bot assigned Monokaix Apr 1, 2024

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2024

william-wang approved these changes Apr 1, 2024

View reviewed changes

volcano-sh-bot assigned william-wang Apr 1, 2024

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2024

volcano-sh-bot merged commit 1c8d95f into volcano-sh:master Apr 1, 2024
14 checks passed

SataQiu mentioned this pull request May 24, 2024

Mark --lock-object-namespace as deprecated and throw a warning when it is used #3483

Merged

		@@ -47,6 +54,9 @@ type ServerOption struct {
		CaCertData []byte
		EnableLeaderElection bool

fix(app): Introduced flag to specify leader election behavious #2809

fix(app): Introduced flag to specify leader election behavious #2809

Conversation

CharlesQQ commented Apr 21, 2023

volcano-sh-bot commented Apr 21, 2023

lowang-bh commented Apr 21, 2023

CharlesQQ commented May 4, 2023

wangyang0616 commented May 4, 2023

CharlesQQ commented May 4, 2023 • edited Loading

CharlesQQ commented Jul 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Dec 15, 2023

CharlesQQ commented Mar 25, 2024

volcano-sh-bot commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesQQ Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesQQ Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesQQ Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesQQ Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CharlesQQ Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monokaix commented Apr 1, 2024

william-wang left a comment

Choose a reason for hiding this comment

volcano-sh-bot commented Apr 1, 2024

CharlesQQ commented May 4, 2023 •

edited

Loading

CharlesQQ Mar 27, 2024 •

edited

Loading

CharlesQQ Mar 27, 2024 •

edited

Loading

CharlesQQ Mar 27, 2024 •

edited

Loading

CharlesQQ Mar 27, 2024 •

edited

Loading

CharlesQQ Mar 27, 2024 •

edited

Loading