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 flake TestRun/Dockerfile_test_copy_symlink #1030

Merged
merged 2 commits into from
Feb 6, 2020

Conversation

tejal29
Copy link
Member

@tejal29 tejal29 commented Feb 5, 2020

Fixes #1019

In this PR

  1. Replicate docker build behavior by copying the target file to destination link.

  2. Create file with same uid and gid as target.

  3. Added different scenarios for ln in integration test.

  4. Manually verified by building use kaniko and docker.

  5. Verified by running the test 10 times

 go test ./integration/...  -run TestRun/test_Dockerfile_test_copy_symlink -count 10 -v --bucket tejal-test --repo gcr.io/tejal-test

...
=== CONT  TestRun/test_Dockerfile_test_copy_symlink
--- PASS: TestRun (0.00s)
    --- PASS: TestRun/test_Dockerfile_test_copy_symlink (0.91s)
        integration_test.go:188: diff = [
              {
                "Image1": "gcr.io/tejal-test/docker-dockerfile_test_copy_symlink",
                "Image2": "gcr.io/tejal-test/kaniko-dockerfile_test_copy_symlink",
                "DiffType": "File",
                "Diff": {
                  "Adds": null,
                  "Dels": null,
                  "Mods": null
                }
              },
              {
                "Image1": "gcr.io/tejal-test/docker-dockerfile_test_copy_symlink",
                "Image2": "gcr.io/tejal-test/kaniko-dockerfile_test_copy_symlink",
                "DiffType": "Metadata",
                "Diff": {
                  "Adds": [],
                  "Dels": []
                }
              }
            ]
PASS
ok  	github.com/GoogleContainerTools/kaniko/integration	140.239s


```

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Feb 5, 2020
@tejal29 tejal29 changed the title fix flake in copy symlink fix flake TestRun/Dockerfile_test_copy_symlink Feb 5, 2020
@tejal29 tejal29 requested a review from cvgw February 5, 2020 22:56
@@ -616,7 +616,7 @@ func CopySymlink(src, dest, buildcontext string) (bool, error) {
if err := createParentDirectory(dest); err != nil {
return false, err
}
return false, os.Symlink(link, dest)
return CopyFile(link, dest, buildcontext, uid, gid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change from link to copy? I'm struggling to connect this change to a flaky test.

Copy link
Member Author

@tejal29 tejal29 Feb 8, 2020

Choose a reason for hiding this comment

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

This test was flaky because of 2 reasons

  1. The link value was /tmp/target.
    We were linking kaniko/0/tmp/link to /tmp/target - should have been /kaniko/0/tmp/link
  2. The target file /tmp/target was getting wiped out before call to os.Symlink(link, dest) . - this caused flakiness.
    (I still don't know why this is caused. However, imitating docker by actually creating the destlink as a regular file with contents of target file fixed the flakiness)

In the description, i mentioned i fixed the CopySymlink to replicate docker behavior. Docker copies the target file to destination link and hence i replaced os.Symlink to CopyFile.

if err != nil {
return err
}
return os.Symlink(link, destFile)
linkPath := filepath.Join(destDir, link)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain this code a bit? I'm having trouble wraping my head around it.

It looks like linkPath is the resolved path of src joined with destDir. I'm confused how we can be sure this path exists.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was error 1 mentioned in here

This ensures, we are copying/creating the required file in kaniko sratch workspace.

@tejal29 tejal29 deleted the fix_flake branch April 26, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flake] integration test flake Dockerfile_test_copy_symlink
3 participants