Repository: knative/pkg
Pull request #1452 (2020-06-27 18:48:33)

Some cleanups around LE
- make funcs and types not expected to be used outside private (we can always make them public, should the need arise)
- add some more checks in tests
- fix some strings

/assign @yanweiguo @mattmoor 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1452#" title="Author self-approved">vagababov</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **evankanderson**
You can assign the PR to them by writing `/assign @evankanderson` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["evankanderson"]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1452#" title="Author self-approved">vagababov</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor**
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[leaderelection/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1452/pull-knative-pkg-go-coverage/1276951574723170304/artifacts/line-cov.html#file0) | 75.8% | 77.1% | 1.4
[leaderelection/context.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1452/pull-knative-pkg-go-coverage/1276951574723170304/artifacts/line-cov.html#file1) | 84.2% | 86.2% | 2.0

knative-prow-robot: @vagababov: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 0b3b20433bc99261a371c6da8f01bdab36d34680 | [link](https://prow.knative.dev/view/gcs/knative-prow/pr-logs/pull/knative_pkg/1452/pull-knative-pkg-unit-tests/1276951574727364610) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1452). [Your PR dashboard](https://gubernator.knative.dev/pr/vagababov).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[leaderelection/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1452/pull-knative-pkg-go-coverage/1276953838993018883/artifacts/line-cov.html#file0) | 75.8% | 77.1% | 1.4
[leaderelection/context.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1452/pull-knative-pkg-go-coverage/1276953838993018883/artifacts/line-cov.html#file1) | 84.2% | 86.2% | 2.0

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1452#pullrequestreview-438718938" title="Approved">mattmoor</a>*, *<a href="https://github.com/knative/pkg/pull/1452#" title="Author self-approved">vagababov</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/pkg/blob/master/OWNERS)~~ [mattmoor]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[leaderelection/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1452/pull-knative-pkg-go-coverage/1276957110839545857/artifacts/line-cov.html#file0) | 75.8% | 80.0% | 4.2
[leaderelection/context.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1452/pull-knative-pkg-go-coverage/1276957110839545857/artifacts/line-cov.html#file1) | 84.2% | 86.2% | 2.0


Patch:
diff --git a/leaderelection/config.go b/leaderelection/config.go
index 9002affb7..8e0728e1f 100644
--- a/leaderelection/config.go
+++ b/leaderelection/config.go
@@ -19,6 +19,8 @@ package leaderelection
 import (
 	"fmt"
 	"os"
+	"strconv"
+	"strings"
 	"time"
 
 	"github.com/kelseyhightower/envconfig"
@@ -127,25 +129,39 @@ type ComponentConfig struct {
 	RetryPeriod   time.Duration
 }
 
-// StatefulSetConfig represents the required information for a StatefulSet service.
-type StatefulSetConfig struct {
-	StatefulSetName string
-	ServiceName     string `envconfig:"STATEFUL_SERVICE_NAME" required:"true"`
-	Port            string `envconfig:"STATEFUL_SERVICE_PORT" default:"80"`
-	Protocol        string `envconfig:"STATEFUL_SERVICE_PROTOCOL" default:"http"`
+// statefulSetID is a envconfig Decodable controller ordinal and name.
+type statefulSetID struct {
+	ssName  string
+	ordinal int
 }
 
-func NewStatefulSetConfig() (*StatefulSetConfig, error) {
-	var ssc StatefulSetConfig
-	ssn, _, err := ParseControllerOrdinal()
-	if err != nil {
-		return nil, err
+func (ssID *statefulSetID) Decode(v string) error {
+	if i := strings.LastIndex(v, "-"); i != -1 {
+		ui, err := strconv.ParseUint(v[i+1:], 10, 64)
+		ssID.ordinal = int(ui)
+		ssID.ssName = v[:i]
+		return err
 	}
-	ssc.StatefulSetName = ssn
-	if err := envconfig.Process("", &ssc); err != nil {
+	return fmt.Errorf("%q is not a valid stateful set controller ordinal", v)
+}
+
+var _ envconfig.Decoder = (*statefulSetID)(nil)
+
+// statefulSetConfig represents the required information for a StatefulSet service.
+type statefulSetConfig struct {
+	StatefulSetID statefulSetID `envconfig:"STATEFUL_CONTROLLER_ORDINAL" required:"true"`
+	ServiceName   string        `envconfig:"STATEFUL_SERVICE_NAME" required:"true"`
+	Port          string        `envconfig:"STATEFUL_SERVICE_PORT" default:"80"`
+	Protocol      string        `envconfig:"STATEFUL_SERVICE_PROTOCOL" default:"http"`
+}
+
+// newStatefulSetConfig builds a stateful set LE config.
+func newStatefulSetConfig() (*statefulSetConfig, error) {
+	ssc := &statefulSetConfig{}
+	if err := envconfig.Process("", ssc); err != nil {
 		return nil, err
 	}
-	return &ssc, nil
+	return ssc, nil
 }
 
 func defaultComponentConfig(name string) ComponentConfig {
diff --git a/leaderelection/context.go b/leaderelection/context.go
index 3322b1b87..57b0586fb 100644
--- a/leaderelection/context.go
+++ b/leaderelection/context.go
@@ -36,10 +36,13 @@ import (
 // WithDynamicLeaderElectorBuilder sets up the statefulset elector based on environment,
 // falling back on the standard elector.
 func WithDynamicLeaderElectorBuilder(ctx context.Context, kc kubernetes.Interface, cc ComponentConfig) context.Context {
-	ssc, err := NewStatefulSetConfig()
+	logger := logging.FromContext(ctx)
+	ssc, err := newStatefulSetConfig()
 	if err == nil {
-		return WithStatefulSetLeaderElectorBuilder(ctx, cc, *ssc)
+		logger.Info("Running with StatefulSet leader election")
+		return withStatefulSetElectorBuilder(ctx, cc, *ssc)
 	}
+	logger.Info("Running with Standard leader election")
 	return WithStandardLeaderElectorBuilder(ctx, kc, cc)
 }
 
@@ -53,10 +56,10 @@ func WithStandardLeaderElectorBuilder(ctx context.Context, kc kubernetes.Interfa
 	})
 }
 
-// WithStatefulSetLeaderElectorBuilder infuses a context with the ability to build
+// withStatefulSetElectorBuilder infuses a context with the ability to build
 // Electors which are assigned leadership based on the StatefulSet ordinal from
 // the provided component configuration.
-func WithStatefulSetLeaderElectorBuilder(ctx context.Context, cc ComponentConfig, ssc StatefulSetConfig) context.Context {
+func withStatefulSetElectorBuilder(ctx context.Context, cc ComponentConfig, ssc statefulSetConfig) context.Context {
 	return context.WithValue(ctx, builderKey{}, &statefulSetBuilder{
 		lec: cc,
 		ssc: ssc,
@@ -81,9 +84,9 @@ func BuildElector(ctx context.Context, la reconciler.LeaderAware, name string, e
 	if val := ctx.Value(builderKey{}); val != nil {
 		switch builder := val.(type) {
 		case *standardBuilder:
-			return builder.BuildElector(ctx, la, name, enq)
+			return builder.buildElector(ctx, la, name, enq)
 		case *statefulSetBuilder:
-			return builder.BuildElector(ctx, la, enq)
+			return builder.buildElector(ctx, la, enq)
 		}
 	}
 
@@ -101,7 +104,8 @@ type standardBuilder struct {
 	lec ComponentConfig
 }
 
-func (b *standardBuilder) BuildElector(ctx context.Context, la reconciler.LeaderAware, name string, enq func(reconciler.Bucket, types.NamespacedName)) (Elector, error) {
+func (b *standardBuilder) buildElector(ctx context.Context, la reconciler.LeaderAware,
+	name string, enq func(reconciler.Bucket, types.NamespacedName)) (Elector, error) {
 	logger := logging.FromContext(ctx)
 
 	id, err := UniqueID()
@@ -153,8 +157,7 @@ func (b *standardBuilder) BuildElector(ctx context.Context, la reconciler.Leader
 				},
 			},
 			ReleaseOnCancel: true,
-
-			Name: rl.Identity(),
+			Name:            rl.Identity(),
 		})
 		if err != nil {
 			return nil, err
@@ -170,26 +173,22 @@ func (b *standardBuilder) BuildElector(ctx context.Context, la reconciler.Leader
 
 type statefulSetBuilder struct {
 	lec ComponentConfig
-	ssc StatefulSetConfig
+	ssc statefulSetConfig
 }
 
-func (b *statefulSetBuilder) BuildElector(ctx context.Context, la reconciler.LeaderAware, enq func(reconciler.Bucket, types.NamespacedName)) (Elector, error) {
+func (b *statefulSetBuilder) buildElector(ctx context.Context, la reconciler.LeaderAware, enq func(reconciler.Bucket, types.NamespacedName)) (Elector, error) {
 	logger := logging.FromContext(ctx)
-
-	ordinal, err := ControllerOrdinal()
-	if err != nil {
-		return nil, err
-	}
-
-	logger.Infof("%s will run in StatefulSet ordinal assignement mode with ordinal %d", b.lec.Component, ordinal)
+	ordinal := uint32(b.ssc.StatefulSetID.ordinal)
+	logger.Infof("%s will run in StatefulSet ordinal assignement mode with ordinal %d",
+		b.lec.Component, ordinal)
 
 	return &unopposedElector{
 		bkt: &bucket{
 			// The name is the full pod DNS of the owner pod of this bucket.
 			name: fmt.Sprintf("%s://%s-%d.%s.%s.svc.%s:%s", b.ssc.Protocol,
-				b.ssc.StatefulSetName, ordinal, b.ssc.ServiceName,
+				b.ssc.StatefulSetID.ssName, ordinal, b.ssc.ServiceName,
 				system.Namespace(), network.GetClusterDomainName(), b.ssc.Port),
-			index: uint32(ordinal),
+			index: ordinal,
 			total: b.lec.Buckets,
 		},
 		la:  la,
diff --git a/leaderelection/ordinal.go b/leaderelection/ordinal.go
index 28f70f734..6ed957385 100644
--- a/leaderelection/ordinal.go
+++ b/leaderelection/ordinal.go
@@ -16,31 +16,12 @@ limitations under the License.
 
 package leaderelection
 
-import (
-	"fmt"
-	"os"
-	"strconv"
-	"strings"
-)
-
-// If run a process on Kubernetes, the value of this environment variable
-// should be set to the pod name via the downward API.
-const controllerOrdinalEnv = "CONTROLLER_ORDINAL"
-
-// ParseControllerOrdinal returns the parts of the StatefulSet's pod name.
-func ParseControllerOrdinal() (string, uint64, error) {
-	v := os.Getenv(controllerOrdinalEnv)
-	if i := strings.LastIndex(v, "-"); i != -1 {
-		ui, err := strconv.ParseUint(v[i+1:], 10, 64)
-		return v[:i], ui, err
-	}
-
-	return "", 0, fmt.Errorf("ordinal not found in %s=%s", controllerOrdinalEnv, v)
-}
-
 // ControllerOrdinal tries to get ordinal from the pod name of a StatefulSet,
 // which is provided from the environment variable CONTROLLER_ORDINAL.
-func ControllerOrdinal() (uint64, error) {
-	_, ui, err := ParseControllerOrdinal()
-	return ui, err
+func ControllerOrdinal() (int, error) {
+	ssc, err := newStatefulSetConfig()
+	if err != nil {
+		return 0, err
+	}
+	return ssc.StatefulSetID.ordinal, nil
 }


Test patch:
diff --git a/leaderelection/context_test.go b/leaderelection/context_test.go
index e124d58ce..601ccac54 100644
--- a/leaderelection/context_test.go
+++ b/leaderelection/context_test.go
@@ -25,6 +25,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	fakekube "k8s.io/client-go/kubernetes/fake"
@@ -35,7 +36,7 @@ import (
 
 func TestWithBuilder(t *testing.T) {
 	cc := ComponentConfig{
-		Component:     "component",
+		Component:     "the-component",
 		LeaderElect:   true,
 		Buckets:       1,
 		ResourceLock:  "leases",
@@ -148,11 +149,11 @@ func TestWithBuilder(t *testing.T) {
 
 func TestWithStatefulSetBuilder(t *testing.T) {
 	cc := ComponentConfig{
-		Component:   "component",
+		Component:   "the-component",
 		LeaderElect: true,
 		Buckets:     1,
 	}
-	podDNS := "ws://as-0.autoscaler.knative-testing.svc.cluster.local:8080"
+	const podDNS = "ws://as-42.autoscaler.knative-testing.svc.cluster.local:8080"
 	ctx := context.Background()
 
 	promoted := make(chan struct{})
@@ -164,20 +165,20 @@ func TestWithStatefulSetBuilder(t *testing.T) {
 	}
 	enq := func(reconciler.Bucket, types.NamespacedName) {}
 
-	if os.Setenv(controllerOrdinalEnv, "as-0") != nil {
-		t.Fatalf("fail to set env var %s=%s", controllerOrdinalEnv, "as-0")
+	if os.Setenv(controllerOrdinalEnv, "as-42") != nil {
+		t.Fatalf("Failed to set env var %s=%s", controllerOrdinalEnv, "as-42")
 	}
 	defer os.Unsetenv(controllerOrdinalEnv)
 	if os.Setenv("STATEFUL_SERVICE_NAME", "autoscaler") != nil {
-		t.Fatalf("fail to set env var %s=%s", "STATEFUL_SERVICE_NAME", "autoscaler")
+		t.Fatalf("Failed to set env var %s=%s", "STATEFUL_SERVICE_NAME", "autoscaler")
 	}
 	defer os.Unsetenv("STATEFUL_SERVICE_NAME")
 	if os.Setenv("STATEFUL_SERVICE_PORT", "8080") != nil {
-		t.Fatalf("fail to set env var %s=%s", "STATEFUL_SERVICE_PORT", "8080")
+		t.Fatalf("Failed to set env var %s=%s", "STATEFUL_SERVICE_PORT", "8080")
 	}
 	defer os.Unsetenv("STATEFUL_SERVICE_PORT")
 	if os.Setenv("STATEFUL_SERVICE_PROTOCOL", "ws") != nil {
-		t.Fatalf("fail to set env var %s=%s", "STATEFUL_SERVICE_PROTOCOL", "ws")
+		t.Fatalf("Failed to set env var %s=%s", "STATEFUL_SERVICE_PROTOCOL", "ws")
 	}
 	defer os.Unsetenv("STATEFUL_SERVICE_PROTOCOL")
 
@@ -186,6 +187,25 @@ func TestWithStatefulSetBuilder(t *testing.T) {
 		t.Error("HasLeaderElection() = false, wanted true")
 	}
 
+	b := ctx.Value(builderKey{})
+	ssb, ok := b.(*statefulSetBuilder)
+	if !ok || ssb == nil {
+		t.Fatal("StatefulSetBuilder not found on context")
+	}
+	want := statefulSetConfig{
+		StatefulSetID: statefulSetID{
+			ssName:  "as",
+			ordinal: 42,
+		},
+		ServiceName: "autoscaler",
+		Port:        "8080",
+		Protocol:    "ws",
+	}
+	if !cmp.Equal(ssb.ssc, want, cmp.AllowUnexported(statefulSetID{})) {
+		t.Errorf("StatefulSetConfig = %#v, want: %#v,diff(-want,+got)\n%s", ssb.ssc, want,
+			cmp.Diff(want, ssb.ssc, cmp.AllowUnexported(statefulSetID{})))
+	}
+
 	le, err := BuildElector(ctx, laf, "name", enq)
 	if err != nil {
 		t.Fatalf("BuildElector() = %v", err)
diff --git a/leaderelection/ordinal_test.go b/leaderelection/ordinal_test.go
index ac1c12a69..d91bcfbd4 100644
--- a/leaderelection/ordinal_test.go
+++ b/leaderelection/ordinal_test.go
@@ -17,34 +17,34 @@ limitations under the License.
 package leaderelection
 
 import (
-	"fmt"
 	"os"
 	"testing"
 )
 
+const controllerOrdinalEnv = "STATEFUL_CONTROLLER_ORDINAL"
+
 func TestControllerOrdinal(t *testing.T) {
 	testCases := []struct {
 		testname    string
 		podName     string
 		wantName    string
-		wantOrdinal uint64
-		err         error
+		wantOrdinal int
+		wantErr     bool
 	}{{
 		testname: "NotSet",
-		err:      fmt.Errorf("ordinal not found in %s=", controllerOrdinalEnv),
+		wantErr:  true,
 	}, {
 		testname: "NoHyphen",
 		podName:  "as",
-		err:      fmt.Errorf("ordinal not found in %s=as", controllerOrdinalEnv),
+		wantErr:  true,
 	}, {
 		testname: "InvalidOrdinal",
 		podName:  "as-invalid",
-		err:      fmt.Errorf(`strconv.ParseUint: parsing "invalid": invalid syntax`),
+		wantErr:  true,
 	}, {
-		testname:    "ValidName",
-		podName:     "as-0",
-		wantName:    "as",
-		wantOrdinal: 0,
+		testname: "ValidName",
+		podName:  "as-0",
+		wantName: "as",
 	}, {
 		testname:    "ValidName",
 		podName:     "as-1",
@@ -59,28 +59,17 @@ func TestControllerOrdinal(t *testing.T) {
 				if os.Setenv(controllerOrdinalEnv, tt.podName) != nil {
 					t.Fatalf("fail to set env var %s=%s", controllerOrdinalEnv, tt.podName)
 				}
+				os.Setenv("STATEFUL_SERVICE_NAME", "n'importe quoi")
+				os.Setenv("STATEFUL_SERVICE_PORT", "1299")
+				os.Setenv("STATEFUL_SERVICE_PROTOCOL", "n'importe quoi")
 			}
 
 			gotOrdinal, gotOrdinalErr := ControllerOrdinal()
-			if tt.err != nil {
-				if gotOrdinalErr == nil || gotOrdinalErr.Error() != tt.err.Error() {
-					t.Errorf("got %v, want = %v, ", gotOrdinalErr, tt.err)
-				}
-			} else if gotOrdinalErr != nil {
-				t.Error("ControllerOrdinal() = ", gotOrdinalErr)
-			} else if gotOrdinal != tt.wantOrdinal {
-				t.Errorf("ControllerOrdinal() = %d, want = %d", gotOrdinal, tt.wantOrdinal)
+			if (gotOrdinalErr != nil) != tt.wantErr {
+				t.Fatalf("Err = %v, wantErr = %v", gotOrdinalErr, tt.wantErr)
 			}
-
-			gotName, _, gotNameErr := ParseControllerOrdinal()
-			if tt.err != nil {
-				if gotNameErr == nil || gotNameErr.Error() != tt.err.Error() {
-					t.Errorf("got %v, want = %v, ", gotNameErr, tt.err)
-				}
-			} else if gotNameErr != nil {
-				t.Error("ParseControllerOrdinal() = ", gotNameErr)
-			} else if gotName != tt.wantName {
-				t.Errorf("ParseControllerOrdinal() = %s, want = %s", gotName, tt.wantName)
+			if gotOrdinal != tt.wantOrdinal {
+				t.Errorf("ControllerOrdinal() = %d, want = %d", gotOrdinal, tt.wantOrdinal)
 			}
 		})
 	}