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

Leader election lock name should include driver name #129

Merged
merged 2 commits into from
Jul 12, 2019
Merged

Leader election lock name should include driver name #129

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
94238a8
Leader election lock name should include driver name
zhucan Jun 19, 2019
e250652
failed unit test
zhucan Jun 19, 2019
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
8 changes: 5 additions & 3 deletions cmd/csi-snapshotter/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"
"os"
"os/signal"
"strings"
"time"

"google.golang.org/grpc"
Expand Down Expand Up @@ -73,8 +74,8 @@ var (
)

var (
version = "unknown"
leaderElectionLockName = "external-snapshotter-leader-election"
version = "unknown"
prefix = "external-snapshotter-leader"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@msau42 Is there any limitation on the length of the lockName? This prefix is a little longer. Other than that, I'm fine with it.

Copy link
Collaborator

@xing-yang xing-yang Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@zhucan Can you shorten this prefix to just "snapshotter"?
@msau42 Are there any naming convention? If not, I'll just go with "snapshotter". Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The attacher and resizer all use "external-" prefix. Probably we should just be the same to be consistent.

In terms of length, the name is used as the ObjectMeta.Name, which is 253 characters

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure.

)

func main() {
Expand Down Expand Up @@ -214,7 +215,8 @@ func main() {
if !*leaderElection {
run(context.TODO())
} else {
le := leaderelection.NewLeaderElection(kubeClient, leaderElectionLockName, run)
lockName := fmt.Sprintf("%s-%s", prefix, strings.Replace(*snapshotterName, "/", "-", -1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we don't need this prefix at all. The lockName used in the provisioner does not have a prefix:
https://github.com/kubernetes-csi/external-provisioner/blob/master/cmd/csi-provisioner/csi-provisioner.go#L202

Copy link
Collaborator

Choose a reason for hiding this comment

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

We do need the prefix otherwise the name will conflict with other sidecar's locks. provisioner unfortunately has more backwards compatibility concerns so it's not so easy to fix: kubernetes-csi/external-provisioner#295

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sound good.

le := leaderelection.NewLeaderElection(kubeClient, lockName, run)
if *leaderElectionNamespace != "" {
le.WithNamespace(*leaderElectionNamespace)
}
Expand Down