Migrated leader election to lease API #120

NikhilSharmaWe · 2022-02-03T13:19:59Z

Fixes #111

Action required: Updated leader election to use Endpoints + Lease. All provisioners based on this library must have RBAC permissions to create/update Lease objects in coordination.k8s.io/v1 API.

NikhilSharmaWe · 2022-02-04T05:37:41Z

@aojea Why is the change here in wrong. I asked wojtekt (on slack) about how to approach this issue, he said that we have to just reuse the existing infrastructure now. So he suggested to change endpoints to enpointsleases first and later to leases.

So then why tests are failing here ?

Also can you make me understand the reason for this change @snOm3ad made in his PR and do we need this change here too ?

aojea · 2022-02-04T06:52:58Z

E0203 13:21:18.190816   12050 runtime.go:78] Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer dereference)
goroutine 47 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic(0x150ae40, 0x247a6e0)
	/home/prow/go/pkg/mod/k8s.io/[email protected]/pkg/util/runtime/runtime.go:74 +0xa3

The client-go vresion seems to old , 0.19.1, it has to bump it to a versions with the endpointleases

NikhilSharmaWe · 2022-02-04T06:59:44Z

@aojea Ok so do we need someone's permission before bumping the version (who is well informed about the repo) or we can do it ourselves to the latest version of https://pkg.go.dev/k8s.io/apimachinery which is v0.23.3?

aojea · 2022-02-04T07:23:44Z

you have to update in the go.mod, see an example

a4a426b

NikhilSharmaWe · 2022-02-04T08:26:31Z

@aojea

I ran go get k8s.io/apimachinery in the repo. It update go.mod and go.sum file, still jobs are failing. What can be the reason ?

Apologies for asking for help time and time again.

aojea · 2022-02-04T08:53:50Z

check the job logs, it gives you hints

../../../pkg/mod/sigs.k8s.io/[email protected]/internal/golang/encoding/json/encode.go:1255:18: sf.IsExported undefined (type reflect.StructField has no field or method IsExported)
note: module requires Go 1.16
# golang.org/x/net/http2
../../../pkg/mod/golang.org/x/[email protected]/http2/transport.go:417:45: undefined: os.ErrDeadlineExceeded
note: module requires Go 1.17

I think that you can run those tests manually and test locally too

NikhilSharmaWe · 2022-02-06T09:33:13Z

@aojea I am not able to make any sense of these logs and make out what is the cause of these jobs failing. Also you mentioned running these tests manually and locally, are you talking about running test/e2e/test.sh file locally.

aojea · 2022-02-06T10:01:31Z

I pasted the logs errorrs

../../../pkg/mod/golang.org/x/[email protected]/http2/transport.go:417:45: undefined: os.ErrDeadlineExceeded
note: module requires Go 1.17

it is failing because the golang dependencies are not correct

jsafrane · 2022-02-07T09:12:21Z

go.mod

 github.com/prometheus/client_golang v1.5.1
 github.com/prometheus/client_model v0.2.0
 golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d // indirect
 golang.org/x/time v0.0.0-20191024005414-555d28b269f0
 k8s.io/api v0.19.1
- k8s.io/apimachinery v0.19.1
+ k8s.io/apimachinery v0.23.3
 k8s.io/client-go v0.19.1


you should bump this one too, to have all k8s libs from the same ver.

@jsafrane As @aojea mentioned by seeing the failing logs after bumping k8s.io/client-go, we need to new golang version for io/fs package but we don't have an idea where it is configured.

Do you have any idea about this ? Please inform.

aojea · 2022-02-07T21:08:42Z

you are progressing, it fails now in

sigs.k8s.io/sig-storage-lib-external-provisioner/v8/controller imports
	k8s.io/client-go/kubernetes imports
	k8s.io/client-go/rest imports
	k8s.io/client-go/plugin/pkg/client/auth/exec imports
	io/fs: malformed module path "io/fs": missing dot in first path element
make: *** [Makefile:18: dep] Error 1

NikhilSharmaWe · 2022-02-08T07:15:02Z

@aojea I am not sure about what to here.

As you mentioned this error is shown io/fs: malformed module path "io/fs": missing dot in first path element. So what we have to do here actually ?

Do we have to correct the module path manually somewhere or we have to import some dependencies to fix it ?

aojea · 2022-02-08T07:23:42Z

A quick search with that error in Google shows it requires a newer golang version ... I don't know where is that configured in this repo

jsafrane · 2022-02-09T10:07:51Z

That's a good question, I think its here: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes-sigs/sig-storage-lib-external-provisioner/sig-storage-lib-external-provisioner.yaml

NikhilSharmaWe · 2022-02-09T17:51:44Z

/retest

NikhilSharmaWe · 2022-02-09T17:59:42Z

@aojea, @jsafrane
Changed the version of go image in sig-storage-lib-external-provisioner.yaml to a newer version, still jobs are failing.

But now they are not showing any logs, build_log.txt is empty for pull-sig-storage-lib-external-provisioner-build and when I try to open details for pull-sig-storage-lib-external-provisioner-unit, it is just loading without showing any information about the fail.

What could be the reason behind it ?

NikhilSharmaWe · 2022-02-10T11:17:19Z

@aojea and @jsafrane Could you please give your thoughts on the previous comment.

aojea · 2022-02-10T11:26:09Z

@aojea and @jsafrane Could you please give your thoughts on the previous comment.

try to build it locally with golang 1.17, and check if there are more changes needed to the go.mod ... because of that, it sasy go1.16 now

NikhilSharmaWe · 2022-02-10T13:08:33Z

@aojea I have tried many times previously to build the repo locally but it seems like my system does not meet the minimum requirements. Is there any other way?

And what do you mean by ... because of that, it sasy go1.16 now?

aojea · 2022-02-10T15:35:21Z

@aojea I have tried many times previously to build the repo locally but it seems like my system does not meet the minimum requirements.

what requirements?

And what do you mean by ... because of that, it sasy go1.16 now?

sig-storage-lib-external-provisioner/go.mod

Line 3 in a35bec6

go 1.16

jsafrane · 2022-02-10T17:02:44Z

it is just loading without showing any information about the fail.

The logs are too big, click on Artifacts on the top right to get to a text file.

humblec · 2022-02-10T17:12:19Z

@NikhilSharmaWe run go test locally and check it pass?

yonatankahana · 2022-05-31T13:36:21Z

@humblec Could you please help me what is the issue here. The script for gofmt is failing, I changed the code for it but still it is giving error.

@NikhilSharmaWe the problem is that the CI using golang >=1.17 where the build tag syntax changed.

for example, mount/mountinfo_unsupported.go:

// +build !windows,!linux,!freebsd,!solaris freebsd,!cgo solaris,!cgo

should now be:

//go:build (!windows && !linux && !freebsd && !solaris) || (freebsd && !cgo) || (solaris && !cgo)

better to fix it automatically in the entire repository by running gofmt -w . (with golang >=1.17 of course)

NikhilSharmaWe · 2022-05-31T13:39:16Z

@yonatankahana Yes I also did that before but after that I was getting error related to that. Trying it again. Also do we need to change the go version in go.mod.

yonatankahana · 2022-05-31T15:55:33Z

@NikhilSharmaWe

now its failing but from a different reason. the script repo-infra/verify/boilerplate/boilerplate.py don't know about the //go:build syntax and it keep failing the test for both files you've fixed, you need to enhance the regex: go_build_constraints to find both kinds of tags, something like ^(//(go:| \+)build.*\n)+\n.

NikhilSharmaWe · 2022-05-31T17:00:05Z

@yonatankahana If the the script repo-infra/verify/boilerplate/boilerplate.py don't know about the //go:build syntax then why are we adding it since the constraint is already added through // +build syntax. I believe it is due to when we upgrade the go version.

But I am not able to understand what type of tags are we searching for here. Could you please explain.
And apologies for spending too much time on this.

yonatankahana · 2022-05-31T17:14:50Z

But I am not able to understand what type of tags are we searching for here. Could you please explain.
And apologies for spending too much time on this.

@NikhilSharmaWe
no need to apologize, it's team work :)

the // +build tags are deprecated and replaced by //go:build tags.
the script boilerplate.py verify that every file with code (.go, .py, Makefile, etc...) have proper license header.
to do that, as can be seen in L84 he wants to remove build tags from the top of Go files. to do that, he uses regex pattern that remove any line starts with // +build, but he don't know to remove lines starts with //go:build yet.
changing the regex in L185 from ^(// \+build.*\n)+\n to ^(//(go:| \+)build.*\n)+\n should detect both kinds and the test will pass.
don't forget to edit the comment in L184 as well.

yonatankahana · 2022-05-31T17:26:02Z

@NikhilSharmaWe

ok, next. now he complains about missing python, maybe try to change all occurrences of python to python3? its a guess... but worth the try

yonatankahana · 2022-05-31T17:27:28Z

repo-infra/verify/boilerplate/boilerplate.py

@@ -181,8 +181,8 @@ def get_regexs():
 # dates can be 2014, 2015, 2016, ..., CURRENT_YEAR, company holder names can be anything
 years = range(2014, date.today().year + 1)
 regexs["date"] = re.compile( '(%s)' % "|".join(map(lambda l: str(l), years)) )
- # strip // +build \n\n build constraints
- regexs["go_build_constraints"] = re.compile(r"^(// \+build.*\n)+\n", re.MULTILINE)
+ # strip (// go:build \n\n) and (// +build \n\n) build constraints


there should be no space between // and go:build

yonatankahana · 2022-05-31T17:37:22Z

@NikhilSharmaWe ok now the actual go tests are failing.

running go test ./controller reproduce it for me on my machine. sounds like a real mistake in the code, looks like #120 (comment) or something similar.

yonatankahana · 2022-05-31T17:42:30Z

controller/controller.go

@@ -865,7 +865,7 @@ func (ctrl *ProvisionController) Run(ctx context.Context) {
 go ctrl.volumeStore.Run(ctx, DefaultThreadiness)

 if ctrl.leaderElection {
- rl, err := resourcelock.New("endpoints",
+ rl, err := resourcelock.New("endpointsleases",


you can use the constant resourcelock.EndpointsLeasesResourceLock here

yonatankahana · 2022-05-31T18:12:16Z

@NikhilSharmaWe

try to change controller.go L872 to:

ctrl.client.CoordinationV1(),

yonatankahana

i can confirm that i've checked it with nfs-subdir-external-provisioner and it looks good.

@wongma7, @jsafrane - can you take a look at it?

wongma7 · 2022-05-31T19:55:30Z

/lgtm
/approve

k8s-ci-robot · 2022-05-31T19:55:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: NikhilSharmaWe, wongma7, yonatankahana

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~~ [wongma7]

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

humblec · 2022-06-01T04:12:24Z

@yonatankahana++ for helping and getting this IN!!

kubernetes-sigs/sig-storage-lib-external-provisioner#120

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 3, 2022

k8s-ci-robot requested a review from wongma7 February 3, 2022 13:20

NikhilSharmaWe force-pushed the lease branch from e1ea7ac to f9bc17b Compare February 4, 2022 07:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 4, 2022

jsafrane reviewed Feb 7, 2022

View reviewed changes

NikhilSharmaWe force-pushed the lease branch from f9bc17b to febe6b7 Compare February 7, 2022 10:59

NikhilSharmaWe requested a review from jsafrane February 7, 2022 11:01

NikhilSharmaWe mentioned this pull request Feb 9, 2022

Changed Golang image version from 1.13.10 to 1.17.1 for sig-storage-lib-external-provisioner.yaml kubernetes/test-infra#25178

Merged

NikhilSharmaWe force-pushed the lease branch from 69d6264 to 0bb2d84 Compare May 31, 2022 12:32

NikhilSharmaWe force-pushed the lease branch from 0bb2d84 to 7dc766f Compare May 31, 2022 13:41

NikhilSharmaWe force-pushed the lease branch from 7dc766f to 194bdd2 Compare May 31, 2022 17:21

yonatankahana suggested changes May 31, 2022

View reviewed changes

NikhilSharmaWe force-pushed the lease branch from 194bdd2 to 1dc72fe Compare May 31, 2022 17:29

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2022

NikhilSharmaWe force-pushed the lease branch from 1dc72fe to dc7d5b0 Compare May 31, 2022 17:30

yonatankahana reviewed May 31, 2022

View reviewed changes

NikhilSharmaWe force-pushed the lease branch from dc7d5b0 to 0969cdb Compare May 31, 2022 17:49

Migrated leader election to lease API

e0c7be1

NikhilSharmaWe force-pushed the lease branch from 0969cdb to e0c7be1 Compare May 31, 2022 18:14

yonatankahana approved these changes May 31, 2022

View reviewed changes

k8s-ci-robot assigned wongma7 May 31, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2022

k8s-ci-robot merged commit a4ce2c1 into kubernetes-sigs:master May 31, 2022

robertvolkmann added a commit to metal-stack/csi-lvm that referenced this pull request Mar 20, 2024

Add necessary permission for leader election with Leases

e4fe2cf

kubernetes-sigs/sig-storage-lib-external-provisioner#120

robertvolkmann mentioned this pull request Mar 20, 2024

Add necessary permission for leader election with Leases metal-stack/csi-lvm#63

Merged

Migrated leader election to lease API #120

Migrated leader election to lease API #120

Conversation

NikhilSharmaWe commented Feb 3, 2022 • edited by jsafrane Loading

NikhilSharmaWe commented Feb 4, 2022 • edited Loading

aojea commented Feb 4, 2022

NikhilSharmaWe commented Feb 4, 2022 • edited Loading

aojea commented Feb 4, 2022

NikhilSharmaWe commented Feb 4, 2022 • edited Loading

aojea commented Feb 4, 2022

NikhilSharmaWe commented Feb 6, 2022 • edited Loading

aojea commented Feb 6, 2022

jsafrane Feb 7, 2022

Choose a reason for hiding this comment

NikhilSharmaWe Feb 8, 2022

Choose a reason for hiding this comment

aojea commented Feb 7, 2022

NikhilSharmaWe commented Feb 8, 2022 • edited Loading

aojea commented Feb 8, 2022

jsafrane commented Feb 9, 2022

NikhilSharmaWe commented Feb 9, 2022

NikhilSharmaWe commented Feb 9, 2022 • edited Loading

NikhilSharmaWe commented Feb 10, 2022 • edited Loading

aojea commented Feb 10, 2022

NikhilSharmaWe commented Feb 10, 2022 • edited Loading

aojea commented Feb 10, 2022

jsafrane commented Feb 10, 2022

humblec commented Feb 10, 2022

yonatankahana commented May 31, 2022

NikhilSharmaWe commented May 31, 2022

yonatankahana commented May 31, 2022 • edited Loading

NikhilSharmaWe commented May 31, 2022

yonatankahana commented May 31, 2022 • edited Loading

yonatankahana commented May 31, 2022 • edited Loading

yonatankahana May 31, 2022

Choose a reason for hiding this comment

yonatankahana commented May 31, 2022

yonatankahana May 31, 2022

Choose a reason for hiding this comment

yonatankahana commented May 31, 2022

yonatankahana left a comment

Choose a reason for hiding this comment

wongma7 commented May 31, 2022

k8s-ci-robot commented May 31, 2022

humblec commented Jun 1, 2022

NikhilSharmaWe commented Feb 3, 2022 •

edited by jsafrane

Loading

NikhilSharmaWe commented Feb 4, 2022 •

edited

Loading

NikhilSharmaWe commented Feb 4, 2022 •

edited

Loading

NikhilSharmaWe commented Feb 4, 2022 •

edited

Loading

NikhilSharmaWe commented Feb 6, 2022 •

edited

Loading

NikhilSharmaWe commented Feb 8, 2022 •

edited

Loading

NikhilSharmaWe commented Feb 9, 2022 •

edited

Loading

NikhilSharmaWe commented Feb 10, 2022 •

edited

Loading

NikhilSharmaWe commented Feb 10, 2022 •

edited

Loading

yonatankahana commented May 31, 2022 •

edited

Loading

yonatankahana commented May 31, 2022 •

edited

Loading

yonatankahana commented May 31, 2022 •

edited

Loading