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

feat: Implement watch for Istio resources #354

Merged
merged 3 commits into from
Jan 21, 2020
Merged

Conversation

dthomson25
Copy link
Member

@dthomson25 dthomson25 commented Jan 14, 2020

To ensure the controller maintains the desired state of the virtual service, this PR adds a watch on virtual services and enqueues rollout that reference those virtual services.

@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #354 into master will decrease coverage by 0.22%.
The diff coverage is 61.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
- Coverage   83.97%   83.75%   -0.23%     
==========================================
  Files          70       70              
  Lines        6528     6593      +65     
==========================================
+ Hits         5482     5522      +40     
- Misses        750      774      +24     
- Partials      296      297       +1
Impacted Files Coverage Δ
rollout/trafficrouting/istio/istio.go 94.07% <100%> (+0.47%) ⬆️
rollout/bluegreen.go 82.52% <100%> (+0.08%) ⬆️
utils/controller/controller.go 80.15% <52.17%> (-15.15%) ⬇️
rollout/controller.go 71.14% <62.5%> (-0.5%) ⬇️

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 002944a...c17c159. Read the comment docs.

@dthomson25 dthomson25 changed the title feat: Implement informer for Istio feat: Implement watch for Istio resources Jan 14, 2020
rollout/trafficrouting/istio/istio.go Show resolved Hide resolved
utils/controller/controller.go Outdated Show resolved Hide resolved
utils/controller/controller.go Show resolved Hide resolved
rollout/controller.go Outdated Show resolved Hide resolved
utils/controller/controller.go Outdated Show resolved Hide resolved
@dthomson25 dthomson25 force-pushed the istio-informer branch 2 times, most recently from 3d72488 to f6c7d97 Compare January 17, 2020 19:10
utils/controller/controller.go Outdated Show resolved Hide resolved
utils/controller/controller.go Outdated Show resolved Hide resolved
for watchEvent := range watchI.ResultChan() {
processNextWatchObj(watchEvent, queue, indexer, index)
}
return nil
Copy link
Member

Choose a reason for hiding this comment

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

There should be a watchI.Stop() call which will clean up resources held by the watch.

}
err := WatchResource(client, namespace, gvk, queue, indexer, index)
if err == nil {
continue
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but If there is no error, I would suggest that we "reset" the exponential backoff by re-initializing the backoff variable.

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.

Minor comments to ensure we call the .Stop() and to reset exponential backoff if we get a happy path.

acc, err := meta.Accessor(obj)
if err != nil {
log.Errorf("Error processing object from watch: %v", err)
return
Copy link
Member

@jessesuen jessesuen Jan 18, 2020

Choose a reason for hiding this comment

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

Is there a reason you don't surface these errors to the caller? I'm afraid that we might get into an error hotloop which we will not back off from with the exponential backoff

Actually, nevermind, we might get here because of normal stream closes, which we would not want to backoff from, and is covered by the closing of the ResultChan()

@dthomson25 dthomson25 merged commit 96f3ca4 into master Jan 21, 2020
@dthomson25 dthomson25 deleted the istio-informer branch January 21, 2020 15:13
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.

3 participants