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

fix: leaderelection uses the lock in the same ns as the controller #1717

Merged
merged 2 commits into from
Dec 21, 2021
Merged

fix: leaderelection uses the lock in the same ns as the controller #1717

merged 2 commits into from
Dec 21, 2021

Conversation

huikang
Copy link
Member

@huikang huikang commented Dec 15, 2021

  • by default, the lock is in argo-rollouts
  • remove leader namespace from cli arguments

Signed-off-by: Hui Kang [email protected]

close #1704

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
  • The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
  • I've signed my commits with DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My builds are green. Try syncing with master if they are not.
  • My organization is added to USERS.md.

@huikang huikang force-pushed the 1704-lease-ns-controller branch from f10045c to f654a39 Compare December 15, 2021 20:26
@huikang huikang requested a review from jessesuen December 15, 2021 20:26
@huikang huikang force-pushed the 1704-lease-ns-controller branch from f654a39 to a6d67dc Compare December 15, 2021 20:43
@codecov
Copy link

codecov bot commented Dec 15, 2021 •
edited
Loading

Codecov Report

Merging #1717 (6824aa3) into master (5b140c9) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1717   +/-   ##
=======================================
  Coverage   82.02%   82.02%           
=======================================
  Files         116      116           
  Lines       16096    16096           
=======================================
  Hits        13203    13203           
  Misses       2218     2218           
  Partials      675      675
Impacted Files Coverage Δ
controller/controller.go 89.45% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b140c9...6824aa3. Read the comment docs.

jessesuen
jessesuen requested changes Dec 18, 2021
View reviewed changes
Copy link
Member

@jessesuen jessesuen left a comment

Choose a reason for hiding this comment

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

It looks like every e2e test all time out. I suspect the controller is not actually starting properly after this change for some reason.

fix: leaderelection uses the lock in the same ns as the controller
50af85c 
- by default the lock is created in argo-rollouts
- remove leader namespace from cli arguments

Signed-off-by: Hui Kang <[email protected]>
@huikang huikang force-pushed the 1704-lease-ns-controller branch from a6d67dc to 50af85c Compare December 18, 2021 03:19
@sonarcloud
Copy link

sonarcloud bot commented Dec 18, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@huikang
Copy link
Member Author

huikang commented Dec 18, 2021

It looks like every e2e test all time out. I suspect the controller is not actually starting properly after this change for some reason.

Yeah, I think it is because the e2e cluster doesn't have the argo-rollouts` ns. Pushing a new commit to create the ns in e2e.

@huikang
Copy link
Member Author

huikang commented Dec 18, 2021

@jessesuen , I think the failed tests are due to flakiness.

@jessesuen jessesuen merged commit a3477cf into argoproj:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessesuen jessesuen jessesuen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Leader election lease namespace should be controller namespace
2 participants
@huikang @jessesuen