Delete and redeploy object upon error 'field is immutable' #940

priyawadhwa · 2018-08-29T21:01:54Z

As discussed in #891, when running skaffold dev certain immutable Kubernetes
objects (like Jobs) can't be redeployed. A 'field is immutable' error is
returned when this happens.

To fix this issue, we can check the error from kubectl apply for 'field
is immutable'. If we find it, we can delete the object and try to deploy
it again.

Adds an integration test for skaffold dev (#441)

As discussed in GoogleContainerTools#891, when running skaffold dev certain immutable Kubernetes objects (like Jobs) can't be redeployed. A 'field is immutable' error is returned when this happens. To fix this issue, we can check the error from kubectl apply for 'field is immutable'. If we find it, we can delete the object and try to deploy it again.

codecov-io · 2018-08-29T21:25:25Z

Codecov Report

Merging #940 into master will decrease coverage by 0.13%.
The diff coverage is 12.5%.

@@            Coverage Diff             @@
##           master     #940      +/-   ##
==========================================
- Coverage   42.57%   42.44%   -0.14%     
==========================================
  Files          71       71              
  Lines        3239     3254      +15     
==========================================
+ Hits         1379     1381       +2     
- Misses       1727     1740      +13     
  Partials      133      133

Impacted Files	Coverage Δ
pkg/skaffold/deploy/kubectl/cli.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/deploy/kubectl/version.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/kubernetes/wait.go	`27.02% <0%> (-2.39%)`	⬇️
pkg/skaffold/deploy/kubectl.go	`52.38% <100%> (+1.16%)`	⬆️

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 95df153...eb06f39. Read the comment docs.

bhack · 2018-08-30T16:21:57Z

/cc @lenlen

r2d4 · 2018-08-30T17:06:20Z

pkg/skaffold/deploy/kubectl/cli.go

+		}
+		// If the output contains the string 'field is immutable', we want to delete the object and recreate it
+		// See Issue #891 for more information
+		if err := c.Detete(ctx, out, manifests); err != nil {


this will need a rebase on balint's PR

balopat

can we please write an integration test for this?

I added an integration test to make sure a Job is deleted and redeployed upon changes when running via skaffold dev. The test sets up by creating a file foo. It runs skaffold dev and make sure the Job is created. It then changes foo so that skaffold redeploys, and makes sure the UID of the new Job is different from the UID of the old job.

priyawadhwa · 2018-08-31T00:09:44Z

@balopat for sure, I added one!

dgageot · 2018-08-31T05:28:46Z

@priyawadhwa should we switch to applying object one by one so that if only one can't be recreated, we don't force delete the others?

…objects

r2d4 · 2018-08-31T18:09:31Z

pkg/skaffold/deploy/kubectl/cli.go

+	for _, mfst := range manifests {
+		buf := bytes.NewBuffer([]byte{})
+		writer := bufio.NewWriter(buf)
+		ml := ManifestList{mfst}


no need to convert this back to a manifest list, mfst is just a []byte at this point

So I had to convert it because the Delete function takes in a ManifestList

func (c *CLI) Delete(ctx context.Context, out io.Writer, manifests ManifestList) error

dgageot · 2018-08-31T18:12:11Z

Is applying one by one slower? How much?

priyawadhwa · 2018-08-31T18:20:40Z

@dgageot I'm not sure what the best way to figure this out would be, is there benchmarking for skaffold set up?

I would guess that it's probably faster to go one by one than it would be to (potentially) delete and recreate all objects.

r2d4 · 2018-08-31T18:24:53Z

We can compare the integration test timings to the previous runs to get a rough idea

priyawadhwa · 2018-08-31T21:58:42Z

Timings with this PR:

--- PASS: TestRun (180.37s)
    --- PASS: TestRun/getting-started_example (26.92s)
    --- PASS: TestRun/annotated_getting-started_example (8.94s)
    --- PASS: TestRun/getting-started_envTagger (14.37s)
    --- PASS: TestRun/gcb_builder_example (31.13s)
    --- PASS: TestRun/deploy_kustomize (4.46s)
    --- PASS: TestRun/bazel_example (47.35s)
    --- PASS: TestRun/kaniko_example (35.48s)
    --- PASS: TestRun/helm_example (11.73s)
--- PASS: TestDev (13.37s)
    --- PASS: TestDev/delete_and_redeploy_job (13.37s)
--- PASS: TestFix (14.83s)
PASS

A previous run:

--- PASS: TestRun (156.64s)
    --- PASS: TestRun/getting-started_example (18.55s)
    --- PASS: TestRun/annotated_getting-started_example (7.16s)
    --- PASS: TestRun/getting-started_envTagger (7.25s)
    --- PASS: TestRun/gcb_builder_example (29.19s)
    --- PASS: TestRun/deploy_kustomize (3.27s)
    --- PASS: TestRun/bazel_example (48.78s)
    --- PASS: TestRun/kaniko_example (33.81s)
    --- PASS: TestRun/helm_example (8.62s)
--- PASS: TestFix (16.07s)
PASS

So it does take a few seconds more for all of the tests except for the bazel test. Since this change only applies to developing with certain objects (Jobs, CronJobs) maybe it would be better to delete all objects and recreate instead?

priyawadhwa · 2018-09-05T23:27:02Z

Update: @r2d4 suggested we use the --force flag with kubectl apply which will delete and recreate an object if patching fails:

  --force=false: Delete and re-create the specified resource, when PATCH encounters conflict and has retried for 5 times.

Support for using this flag with the immutable error was added in this PR, which hasn't been released officially. As discussed offline, we'll wait for kubectl v1.12.0 to come out so that we can use this flag.

balopat · 2018-09-11T22:29:56Z

One more comment: we should be careful about version support for kubectl and make it clear in the docs what works. Also we could print a warning for lower than supported kubectl versions.

The --force flag will delete and redeploy a deployment if 'kubectl apply' doesn't work because a field is immutable. Updated the skaffold deploy Dockerfile to reflect this change, added a note in the docs that kubectl > 1.12.0 is recommended, and added a check in the kubectl deployer for the version.

priyawadhwa · 2018-10-01T21:10:06Z

kubectl v1.12.0 is out, this should be RFAL :)

nkubala

A couple nits, but otherwise this seems fine

nkubala · 2018-10-01T21:55:05Z

docs/concepts.adoc

@@ -28,6 +28,8 @@ tools for deployment, for example `kubectl` or `helm`.
 Each deployment type has parameters that allow you to
 define how you want your app to be installed and updated.

+_Note: kubectl version 1.12.0 or greater is recommended for use with skaffold._


might be worth documenting the known issues for earlier kubectl versions for users

for sure, would this be the right spot for that?

ah, sorry missed your reply on this. FWIW this is a fine place to do it :)

nkubala · 2018-10-01T21:56:37Z

pkg/skaffold/deploy/kubectl/version.go

+func (c *CLI) CheckVersion() error {
+	m, err := strconv.Atoi(c.Version().Minor)
+	if err != nil {
+		return fmt.Errorf("couldn't get kubectl minor version: %v", err)


nit: errors.Wrapf(err, "retrieving kubectl minor version")

nkubala · 2018-10-01T21:57:07Z

pkg/skaffold/deploy/kubectl/version.go

+		return fmt.Errorf("couldn't get kubectl minor version: %v", err)
+	}
+	if m < 12 {
+		return fmt.Errorf("kubectl version 1.12.0 or greater is recommended for use with skaffold")


nit: errors.New("...")

dgageot · 2018-10-02T15:23:34Z

@priyawadhwa I wonder if we could use that feature only when kubectl is 1.12+

priyawadhwa · 2018-10-02T16:58:27Z

@dgageot so the flag exists in lower versions it just doesn't work as expected, so if a user has a lower version then the same bug from the issue will occur.

albertkang · 2018-10-10T15:56:19Z

Any ideas when this will get merged?

I think your feedback was taken into account

haf-afa · 2019-10-29T12:09:56Z

Shouldn't it be the same for skaffold run? Because for me that errors.

haf · 2020-02-02T13:44:50Z

Ping, @priyawadhwa @dgageot , see the comment from last year: this is still broken

priyawadhwa · 2020-02-04T00:29:17Z

hey @haf -- what version of kubectl are you using? This feature only works with kubectl >1.12; if you're already on that, would you mind opening an issue so this bug can be tracked?

balopat · 2020-03-24T21:31:50Z

hi @haf - you can override the behavior for skaffold run with --force - does that work for you?

haf · 2020-03-24T21:40:25Z

I’m using latest of everything.

I never do force on almost anything. Only interested in happy path for the team.

priyawadhwa requested review from balopat, dgageot and r2d4 as code owners August 29, 2018 21:01

priyawadhwa force-pushed the job branch from 9229993 to 939e6cf Compare August 29, 2018 21:16

fixed merge conflict

8747bf9

r2d4 reviewed Aug 30, 2018

View reviewed changes

balopat previously requested changes Aug 30, 2018

View reviewed changes

priyawadhwa force-pushed the job branch 2 times, most recently from 9744858 to 2e5ac25 Compare August 30, 2018 22:54

priyawadhwa force-pushed the job branch from 2e5ac25 to ba13055 Compare August 30, 2018 22:56

Rebased

354de8e

priyawadhwa force-pushed the job branch from 86c89f4 to 4bda4a1 Compare August 30, 2018 23:51

Remove cleanup from skaffold dev test

01a154d

priyawadhwa force-pushed the job branch from 4bda4a1 to 01a154d Compare August 30, 2018 23:59

Apply objects one by one so that one redeploy won't force delete all …

1cb427b

…objects

r2d4 reviewed Aug 31, 2018

View reviewed changes

balopat mentioned this pull request Sep 4, 2018

Create a benchmark script for deployment speed #952

Closed

dgageot added the wip label Sep 29, 2018

merged master, fixed merge conflict in cli.go

b80c9f3

priyawadhwa requested a review from nkubala as a code owner October 1, 2018 20:38

priyawadhwa force-pushed the job branch from efbc142 to 1953659 Compare October 1, 2018 20:42

priyawadhwa force-pushed the job branch from 1953659 to 1c4780f Compare October 1, 2018 20:42

priyawadhwa removed the wip label Oct 1, 2018

nkubala reviewed Oct 1, 2018

View reviewed changes

r2d4 mentioned this pull request Oct 3, 2018

Skipping Deploy due to error: apply: kubectl apply #1077

Closed

priyawadhwa mentioned this pull request Oct 3, 2018

Upgrade kubectl version in docker image. #1081

Closed

Address code review comments

eb06f39

priyawadhwa force-pushed the job branch from 5456f1c to eb06f39 Compare October 3, 2018 20:45

balopat mentioned this pull request Oct 4, 2018

use kubectl replace --force for one-off pods #1099

Closed

dgageot approved these changes Oct 10, 2018

View reviewed changes

dgageot merged commit ce01bd7 into GoogleContainerTools:master Oct 10, 2018

balopat mentioned this pull request Nov 13, 2018

Deploy a Job #891

Closed

rlotufo mentioned this pull request Feb 28, 2019

Cannot get job redeployed using helm #1706

Closed

priyawadhwa deleted the job branch February 4, 2020 00:27

nkubala added the triage/discuss Items for discussion label May 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete and redeploy object upon error 'field is immutable' #940

Delete and redeploy object upon error 'field is immutable' #940

priyawadhwa commented Aug 29, 2018 •

edited

Loading

codecov-io commented Aug 29, 2018 •

edited

Loading

bhack commented Aug 30, 2018

r2d4 Aug 30, 2018

balopat left a comment

priyawadhwa commented Aug 31, 2018

dgageot commented Aug 31, 2018

r2d4 Aug 31, 2018

priyawadhwa Aug 31, 2018

dgageot commented Aug 31, 2018

priyawadhwa commented Aug 31, 2018

r2d4 commented Aug 31, 2018

priyawadhwa commented Aug 31, 2018

priyawadhwa commented Sep 5, 2018

balopat commented Sep 11, 2018

priyawadhwa commented Oct 1, 2018

nkubala left a comment

nkubala Oct 1, 2018

priyawadhwa Oct 2, 2018

nkubala Oct 10, 2018

nkubala Oct 1, 2018

nkubala Oct 1, 2018

dgageot commented Oct 2, 2018

priyawadhwa commented Oct 2, 2018

albertkang commented Oct 10, 2018

haf-afa commented Oct 29, 2019

haf commented Feb 2, 2020

priyawadhwa commented Feb 4, 2020

balopat commented Mar 24, 2020

haf commented Mar 24, 2020

Delete and redeploy object upon error 'field is immutable' #940

Delete and redeploy object upon error 'field is immutable' #940

Conversation

priyawadhwa commented Aug 29, 2018 • edited Loading

codecov-io commented Aug 29, 2018 • edited Loading

Codecov Report

bhack commented Aug 30, 2018

Choose a reason for hiding this comment

balopat left a comment

Choose a reason for hiding this comment

priyawadhwa commented Aug 31, 2018

dgageot commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgageot commented Aug 31, 2018

priyawadhwa commented Aug 31, 2018

r2d4 commented Aug 31, 2018

priyawadhwa commented Aug 31, 2018

priyawadhwa commented Sep 5, 2018

balopat commented Sep 11, 2018

priyawadhwa commented Oct 1, 2018

nkubala left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgageot commented Oct 2, 2018

priyawadhwa commented Oct 2, 2018

albertkang commented Oct 10, 2018

haf-afa commented Oct 29, 2019

haf commented Feb 2, 2020

priyawadhwa commented Feb 4, 2020

balopat commented Mar 24, 2020

haf commented Mar 24, 2020

priyawadhwa commented Aug 29, 2018 •

edited

Loading

codecov-io commented Aug 29, 2018 •

edited

Loading