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

Add canary finalizers #495

Merged
merged 5 commits into from
Mar 23, 2020
Merged

Add canary finalizers #495

merged 5 commits into from
Mar 23, 2020

Conversation

ta924
Copy link
Contributor

@ta924 ta924 commented Mar 10, 2020

@stefanprodan This is a work in progress PR looking for acceptance on the approach and feedback. This PR provides the opt-in capability for users to revert flagger mutations on deletion of a canary. If users opt-in finalizers will be utilized to revert the mutated resources before the canary and owned resources are handed over for finalizing.

Changes:
Add evaluations for finalizers controller/controller
Add finalizers source controller/finalizer
Add interface method on deployment and daemonset controllers
Add interface method on routers
Add e2e tests

Work to be done:
Cover mesh and ingress outside of Istio

Fix: #388
Fix: #488

@ta924 ta924 requested a review from stefanprodan as a code owner March 10, 2020 15:30
@stefanprodan
Copy link
Member

@ta924 please run make fmt to unblock the CI.

@ta924
Copy link
Contributor Author

ta924 commented Mar 10, 2020

Yeah you beat me to it 👍

@ta924
Copy link
Contributor Author

ta924 commented Mar 10, 2020

FYI, I have test cases written but I did them prior to the logic and the unit tests had a pretty significant change. I'm going to work on fixing those up and will have them committed as well

Dockerfile Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@stefanprodan
Copy link
Member

@ta924 please do a master rebase and don't merge master from now on, just rebase please.

@stefanprodan
Copy link
Member

@ta924 the go mod revert is wrong as it brings an older version, please rebase your branch with master.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Please squash all commits after reverting Dockerfile and main.go to cleanup the log.

Dockerfile Outdated Show resolved Hide resolved
cmd/flagger/main.go Outdated Show resolved Hide resolved
pkg/controller/finalizer.go Outdated Show resolved Hide resolved
pkg/router/istio.go Outdated Show resolved Hide resolved
@stefanprodan stefanprodan changed the title Add finalize Add canary finalizers Mar 12, 2020
@stefanprodan
Copy link
Member

Hey @ta924 unrelated to this PR, can you please add your organisation to #470 ?

Thanks

@ta924
Copy link
Contributor Author

ta924 commented Mar 13, 2020

We are still evaluating Flagger as a specialized deployment option, so I won't be able to update the README at this time. If we adopt this as a production solution I will circle back and do some updating.

pkg/router/istio.go Outdated Show resolved Hide resolved
@ta924
Copy link
Contributor Author

ta924 commented Mar 14, 2020

I did some initial testing with Helm 3 and I didn’t see any issues with overwriting of the spec between deployments. Helm assumes the VS matches the secret used for release and doesn’t mutate. Kustomize, helm 2, and helm 3 function as expected.

@stefanprodan
Copy link
Member

@ta924 that’s great news, thanks for testing. I think we can move forward with adding the finalizer to the Istio e2e tests.

@ta924
Copy link
Contributor Author

ta924 commented Mar 14, 2020

I will clean up a few things based on comments, then I want to look over the logging messages just for readability. Them move it into the e2e testing, probably be early next week

@codecov-io
Copy link

codecov-io commented Mar 17, 2020

Codecov Report

Merging #495 into master will decrease coverage by 0.43%.
The diff coverage is 38.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
- Coverage   50.73%   50.30%   -0.44%     
==========================================
  Files          62       63       +1     
  Lines        4679     4934     +255     
==========================================
+ Hits         2374     2482     +108     
- Misses       1910     2042     +132     
- Partials      395      410      +15     
Impacted Files Coverage Δ
pkg/canary/service_controller.go 0.00% <0.00%> (ø)
pkg/controller/controller.go 1.33% <0.00%> (-0.26%) ⬇️
pkg/router/appmesh.go 85.14% <0.00%> (-0.57%) ⬇️
pkg/router/contour.go 88.19% <0.00%> (-0.62%) ⬇️
pkg/router/gloo.go 70.00% <0.00%> (-1.30%) ⬇️
pkg/router/ingress.go 53.54% <0.00%> (-0.86%) ⬇️
pkg/router/kubernetes_noop.go 0.00% <0.00%> (ø)
pkg/router/nop.go 0.00% <0.00%> (ø)
pkg/router/smi.go 47.69% <0.00%> (-0.75%) ⬇️
pkg/controller/finalizer.go 32.45% <32.45%> (ø)
... and 7 more

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 52d9951...7cf836e. Read the comment docs.

@stefanprodan
Copy link
Member

@ta924 I'll have to ask you again to rebase master so that other PRs don't show up in here and also please squash the e2e commits into a single one.

@ta924
Copy link
Contributor Author

ta924 commented Mar 17, 2020

Sorry about the rebase and squash, it was getting late last night and it slipped my mind.

@stefanprodan
Copy link
Member

@ta924 this looks really good, thank you!

As for the next steps:

  • add "Canary finalizer" section to docs
  • add a Cluster IP and Virtual Service definition to Istio e2e to test the restore from kubectl annotation

@ta924
Copy link
Contributor Author

ta924 commented Mar 18, 2020

I will dive into the docs today or sometime this evening, thanks for the previous comments.

@ta924
Copy link
Contributor Author

ta924 commented Mar 18, 2020

@stefanprodan do you have any recommendations on where you would like the docs to land for this feature?

@stefanprodan
Copy link
Member

@ta924 it could be a section called "Canary finalizers" right before "Canary analysis" in here https://github.com/weaveworks/flagger/blob/master/docs/gitbook/usage/how-it-works.md#canary-analysis

@stefanprodan
Copy link
Member

@ta924 about ingress controllers, we don't need to implement finalizers for any of them since Flagger doesn't mutates an ingress/gateway object, it creates a shadow one that is GCed by Kubernetes. Same for SMI, once the TrafficSplit is removed, Linkerd will route to the original deployment.

@ta924
Copy link
Contributor Author

ta924 commented Mar 20, 2020

Gotcha, I can back out the commit for SMI. I was looking at the resources mutated and tossed that in there for linkerd. That being said only istio currently needs the finalizing. Thanks for the details.

@stefanprodan
Copy link
Member

The commits are mixed up, can you please do a rebase with master and squash the reverted commit.

Tanner Altares added 4 commits March 20, 2020 15:13
rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

add unit tests for finalizing

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

run fmt to clean up formatting

review changes

add kubectl annotation

add kubectl annotation support

introduction of finalizer

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

add unit tests for finalizing

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

run fmt to clean up formatting

review changes

introduction of finalizer

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

add unit tests for finalizing

introduction of finalizer

rebase and squash

fix fmt issues

revert Dockerfile

revert go.mod and go.sum

introduction of finalizer

introduction of finalizer

remove test for finalizer add istio tests

fix fmt issues

revert go.mod and go.sum

revert Dockerfile and main.go

fmt deployment controller

run fmt to clean up formatting

review changes
add e2e tests istio

clean up comment from review

add e2e tests istio

clean up comment from review

clean up logging statement

add e2e tests istio

clean up comment from review

clean up logging statement

add log statement on e2e iteration

add e2e tests istio

clean up comment from review

clean up logging statement

add log statement on e2e iteration

extend timeout for finalizing

add e2e tests istio

clean up comment from review

clean up logging statement

add log statement on e2e iteration

extend timeout for finalizing

add phase to kustomize crd

add e2e tests istio

clean up comment from review

clean up logging statement

add log statement on e2e iteration

extend timeout for finalizing

add phase to kustomize crd

revert timeout on circleci

vs and svc checks for istio e2e tests

fix fmt errors and tests

add get statement in e2e test

add get statement in e2e test

add namespace to e2e

use only selector for service revert
@ta924
Copy link
Contributor Author

ta924 commented Mar 20, 2020

@stefanprodan rebase and squash done, any way to rerun the nginx-test without a commit? I don't have permissions

@stefanprodan
Copy link
Member

Can you check the changes? you need to rebase with upstream master

fmt

update e2e test typo

finalizer return if not found

fix typo
@ta924
Copy link
Contributor Author

ta924 commented Mar 21, 2020

Should be all squared away now, the revert lumped together with the squash dropped my rebase from upstream. Thanks for catching that.

Copy link
Collaborator

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

i noticed that some errors are not wrapped but we can do refactoring later 👍 looks good to me though i just skimmed over @stefanprodan

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

LGTM

Awesome contribution @ta924, thanks a lot 🏅

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.

Is it posible to automate resources rollback Switching between canary and k8s rollout with no downtime
4 participants