[go: up one dir, main page]
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

kvstore: add clusterName suffix to session controllers #23928

Merged
merged 1 commit into from
Mar 1, 2023
Merged

kvstore: add clusterName suffix to session controllers #23928

merged 1 commit into from
Mar 1, 2023

Conversation

oblazek
Copy link
Contributor
@oblazek oblazek commented Feb 22, 2023

Previously all session/locksession controllers used the same name as a controller name (even the ones for remote clusters) and it was not possible to distinguish between them.

cilium status --verbose would show this output for clustermesh with 4 remote clusters:

  kvstore-etcd-lock-session-renew      never          never        0       no error   
  kvstore-etcd-lock-session-renew      never          never        0       no error   
  kvstore-etcd-lock-session-renew      never          never        0       no error   
  kvstore-etcd-lock-session-renew      never          never        0       no error   
  kvstore-etcd-lock-session-renew      never          never        0       no error   
  kvstore-etcd-session-renew        never          never        0       no error   
  kvstore-etcd-session-renew        never          never        0       no error   
  kvstore-etcd-session-renew        never          never        0       no error   
  kvstore-etcd-session-renew        never          never        0       no error   
  kvstore-etcd-session-renew        never          never        0       no error

And noone has any idea which belongs to which connection.

This commit adds a suffix with a cluster name to each session controller name i.e.:

  kvstore-etcd-lock-session-renew                                       never          never        0       no error   
  kvstore-etcd-lock-session-renew-tt-k8st1-oa                           never          never        0       no error   
  kvstore-etcd-lock-session-renew-tt-k8st3-ko                           never          never        0       no error   
  kvstore-etcd-lock-session-renew-uacl-test-hadoop                      never          never        0       no error   
  kvstore-etcd-lock-session-renew-uacl-test-labrador-test               never          never        0       no error   
  kvstore-etcd-session-renew                                            never          never        0       no error   
  kvstore-etcd-session-renew-tt-k8st1-oa                                never          never        0       no error   
  kvstore-etcd-session-renew-tt-k8st3-ko                                never          never        0       no error   
  kvstore-etcd-session-renew-uacl-test-hadoop                           never          never        0       no error   
  kvstore-etcd-session-renew-uacl-test-labrador-test                    never          never        0       no error

Had a small discussion with Andre about this https://cilium.slack.com/archives/C2B917YHE/p1676646945994179

@oblazek oblazek requested review from a team as code owners February 22, 2023 07:56
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 22, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 22, 2023
@tklauser tklauser added release-note/misc This PR makes changes that have no direct user impact. area/clustermesh Relates to multi-cluster routing functionality in Cilium. labels Feb 22, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 22, 2023
tklauser
tklauser requested changes Feb 22, 2023
View reviewed changes
pkg/kvstore/etcd.go Outdated
@@ -788,7 +791,7 @@ func connectEtcdClient(ctx context.Context, config *client.Config, cfgPath strin

go ec.statusChecker()

ec.controllers.UpdateController("kvstore-etcd-session-renew",
ec.controllers.UpdateController(makeSessionName(etcdSessionRenewNamePrefix, opts.ClusterName),
Copy link
Member

Choose a reason for hiding this comment

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

opts can be nil here, that's probably why the integration tests are failing: https://github.com/cilium/cilium/actions/runs/4240559572/jobs/7371405312

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ofcourse, thanks

pkg/kvstore/etcd.go Outdated
@@ -799,7 +802,7 @@ func connectEtcdClient(ctx context.Context, config *client.Config, cfgPath strin
},
)

ec.controllers.UpdateController("kvstore-etcd-lock-session-renew",
ec.controllers.UpdateController(makeSessionName(etcdLockSessionRenewNamePrefix, opts.ClusterName),
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@oblazek
kvstore: add clusterName suffix to session controllers
78ede9b 
Previously all session/locksession controllers used the same
name as a controller name (even the ones for remote clusters)
and it was not possible to distinguish between them.

This commit adds a suffix with a cluster name to each session
controller name.

Signed-off-by: Ondrej Blazek <ondrej.blazek@firma.seznam.cz>
@tklauser
Copy link
Member

/test

@sayboras
Copy link
Member

/test-1.16-4.19

@oblazek
Copy link
Contributor Author
oblazek commented Feb 27, 2023

any idea what could be wrong with 1.16-4.19 ? doesn't look related

@sayboras
Copy link
Member

/test-1.16-4.19

@sayboras sayboras requested a review from a team February 28, 2023 10:37
@sayboras
Copy link
Member
sayboras commented Mar 1, 2023

Reviews are in, all CI jobs are passed. Marking this ready to merge.

@sayboras sayboras added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 1, 2023
@sayboras sayboras merged commit 0084307 into cilium:master Mar 1, 2023
@sayboras sayboras mentioned this pull request Mar 1, 2023
Merged
13 tasks
@maintainer-s-little-helper maintainer-s-little-helper bot removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Mar 1, 2023
@YutaroHayakawa YutaroHayakawa added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 labels Mar 3, 2023
@nebril nebril mentioned this pull request Mar 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tklauser tklauser tklauser approved these changes

Assignees
No one assigned
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/community-contribution This was a contribution made by a community member. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
cilium v1.13.1
Status: Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@oblazek @tklauser @sayboras @YutaroHayakawa