Add canary finalizers #495
Conversation
|
@ta924 please run |
|
Yeah you beat me to it 👍 |
|
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 |
|
@ta924 please do a master rebase and don't merge master from now on, just rebase please. |
|
@ta924 the go mod revert is wrong as it brings an older version, please rebase your branch with master. |
|
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. |
|
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. |
|
@ta924 that’s great news, thanks for testing. I think we can move forward with adding the finalizer to the Istio e2e tests. |
|
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 Report
@@ 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
Continue to review full report at Codecov.
|
|
@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. |
|
Sorry about the rebase and squash, it was getting late last night and it slipped my mind. |
|
I will dive into the docs today or sometime this evening, thanks for the previous comments. |
|
@stefanprodan do you have any recommendations on where you would like the docs to land for this feature? |
|
@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 |
|
@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. |
|
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. |
|
The commits are mixed up, can you please do a rebase with master and squash the reverted commit. |
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
|
@stefanprodan rebase and squash done, any way to rerun the nginx-test without a commit? I don't have permissions |
|
Can you check the changes? you need to rebase with upstream master |
fmt update e2e test typo finalizer return if not found fix typo
|
Should be all squared away now, the revert lumped together with the squash dropped my rebase from upstream. Thanks for catching that. |
There was a problem hiding this comment.
i noticed that some errors are not wrapped but we can do refactoring later 👍 looks good to me though i just skimmed over @stefanprodan
@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