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.

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

fix: plugin panic while watching progress #1796

Merged

Conversation

jessesuen
Copy link
Member

Resolves #1747

Fixes a panic introduced in v1.1.0 plugin which closed a channel prematurely while the sender (view controller) was still sending on it. This change prevents this by deferring the channel close, and also niling out the view controller's callback list when the view controller stops, so that it will not attempt to perform any callbacks when finished.

As part of this fix, I made the plugin less chatty and only showed status changes. Previously, it would print out the phase and message on every rollout change, even if it was the same as before. Example output:

$ kubectl-argo-rollouts status my-rollout -w
Progressing - more replicas need to be updated
Paused - CanaryPauseStep
Progressing - more replicas need to be updated
Progressing - updated replicas are still becoming available
Healthy

Signed-off-by: Jesse Suen [email protected]

@jessesuen jessesuen force-pushed the fix-plugin-panic-rollout-status branch from 0f7d652 to 6964726 Compare January 20, 2022 23:32
@sonarqubecloud
Copy link

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
4.4% 4.4% Duplication

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #1796 (6964726) into master (f5c229b) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1796      +/-   ##
==========================================
- Coverage   82.11%   82.06%   -0.05%     
==========================================
  Files         116      116              
  Lines       16317    16340      +23     
==========================================
+ Hits        13398    13410      +12     
- Misses       2236     2243       +7     
- Partials      683      687       +4     
Impacted Files Coverage Δ
pkg/kubectl-argo-rollouts/cmd/status/status.go 94.18% <100.00%> (+0.43%) ⬆️
...ctl-argo-rollouts/viewcontroller/viewcontroller.go 70.58% <100.00%> (-1.44%) ⬇️
rollout/trafficrouting/istio/controller.go 50.81% <0.00%> (-1.63%) ⬇️
pkg/kubectl-argo-rollouts/info/rollout_info.go 87.50% <0.00%> (-1.27%) ⬇️
rollout/trafficrouting/istio/istio.go 81.19% <0.00%> (+0.03%) ⬆️

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 f5c229b...6964726. Read the comment docs.

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

LGTM

@jessesuen jessesuen merged commit 6fabe9d into argoproj:master Jan 21, 2022
@jessesuen jessesuen deleted the fix-plugin-panic-rollout-status branch January 21, 2022 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubectl-argo-rollouts panic while watching progress
2 participants