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

extended tests: use leaner repo for contextdir test #20090

Conversation

wozniakjan
Copy link
Contributor

speed up from 166s to 17s

old using s2i ruby based docker image
STEP: starting a build
Jun 12 11:54:45.365: INFO: Running 'oc start-build --config=/tmp/configfile443130924 --namespace=e2e-test-contextdir-ntbrt dockercontext'
build "dockercontext-1" started
STEP: waiting for build to finish
[AfterEach]
  /tmp/openshift/build-rpms/rpm/BUILD/origin-3.10.0/_output/local/go/src/github.com/openshift/origin/test/extended/builds/contextdir.go:38
[AfterEach] [Feature:Builds][Slow] builds with a context directory
  /tmp/openshift/build-rpms/rpm/BUILD/origin-3.10.0/_output/local/go/src/github.com/openshift/origin/test/extended/util/cli.go:72
STEP: Deleting namespaces
Jun 12 11:57:21.689: INFO: namespace : e2e-test-contextdir-ntbrt api call to delete is complet
new using busybox
STEP: starting a build
Jun 15 13:41:24.455: INFO: Running 'oc start-build --config=/tmp/configfile409710299 --namespace=e2e-test-contextdir-nfbw4 dockercontext --from-repo /tmp/openshift/core/contextdir796719742/contextdir'
Uploading "/tmp/openshift/core/contextdir796719742/contextdir" at commit "HEAD" as binary input for the build ... 
WARNING: the provided file may not be an archive (tar, tar.gz, or zip), use --from-file to prevent extraction
build "dockercontext-1" started
STEP: waiting for build to finish
[AfterEach] 
  /go/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/test/extended/builds/contextdir.go:38
[AfterEach] [Feature:Builds][Slow] builds with a context directory
  /go/src/github.com/openshift/origin/_output/local/go/src/github.com/openshift/origin/test/extended/util/cli.go:72
STEP: Deleting namespaces
Jun 15 13:41:34.470: INFO: namespace : e2e-test-contextdir-nfbw4 api call to delete is complete 

ptal @openshift/sig-developer-experience , I would like to note that it issues a warning with --from-repo, is that expected warning? Using --from-file feels misleading given we would like to pass a repo

WARNING: the provided file may not be an archive (tar, tar.gz, or zip), use --from-file to prevent extraction

@wozniakjan wozniakjan requested a review from bparees June 25, 2018 13:04
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 25, 2018
Copy link
Contributor

@adambkaplan adambkaplan left a comment

Choose a reason for hiding this comment

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

/lgtm, though I wonder if the warning is due to the input being "binary"

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2018
@bparees
Copy link
Contributor

bparees commented Jun 25, 2018

nice!

/lgtm

@bparees
Copy link
Contributor

bparees commented Jun 25, 2018

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 25, 2018
@bparees
Copy link
Contributor

bparees commented Jun 25, 2018

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2018
@bparees
Copy link
Contributor

bparees commented Jun 25, 2018

ptal @openshift/sig-developer-experience , I would like to note that it issues a warning with --from-repo, is that expected warning?

that is not an expected warning and I don't see it when doing a binary build myself w/ --from-repo so we should definitely understand why you're getting it.

here's my output:

$ oc start-build nodejs-ex --from-repo .
Uploading "." at commit "HEAD" as binary input for the build ...
build "nodejs-ex-3" started
$ 

and my buildconfig:

apiVersion: build.openshift.io/v1
kind: BuildConfig
metadata:
  annotations:
    openshift.io/generated-by: OpenShiftNewBuild
  creationTimestamp: 2018-06-25T19:09:07Z
  labels:
    build: nodejs-ex
  name: nodejs-ex
  namespace: p1
  resourceVersion: "1488"
  selfLink: /apis/build.openshift.io/v1/namespaces/p1/buildconfigs/nodejs-ex
  uid: 3c0b3cad-78ab-11e8-8c6a-507b9d27b5d9
spec:
  failedBuildsHistoryLimit: 5
  nodeSelector: null
  output:
    to:
      kind: ImageStreamTag
      name: nodejs-ex:latest
  postCommit: {}
  resources: {}
  runPolicy: Serial
  source:
    binary: {}
    type: Binary
  strategy:
    sourceStrategy:
      from:
        kind: ImageStreamTag
        name: nodejs:8
        namespace: openshift
    type: Source
  successfulBuildsHistoryLimit: 5
  triggers:
  - github:
      secret: XRkOzKa8Mu6qWGur2tYE
    type: GitHub
  - generic:
      secret: 4N_Sb8beSuxEeI7BH_t8
    type: Generic
  - type: ConfigChange
  - imageChange:
      lastTriggeredImageID: 172.30.129.85:5000/openshift/nodejs@sha256:3f262ed6ed7ec7497576e1117ddfafacaa23fecf34693e05a0abcb652b93e653
    type: ImageChange
status:
  lastVersion: 3

@wozniakjan
Copy link
Contributor Author

wozniakjan commented Jun 26, 2018

@adambkaplan @bparees, the warning appears only if very small git repository is used

here it creates a tar archive from the freshly checked out repo in path /tmp/oc_cloning_[random]

pr, pw := io.Pipe()
go func() {
w := gzip.NewWriter(pw)
if err := tar.New(s2ifs.NewFileSystem()).CreateTarStream(path, false, w); err != nil {
pw.CloseWithError(err)
} else {
w.Close()
pw.CloseWithError(io.EOF)
}
if usedTempDir {
os.RemoveAll(tempDirectory)
}
}()
r = pr

which is later verified for being any type of known and understood archive here

func isArchive(r *bufio.Reader) bool {
data, err := r.Peek(280)
if err != nil {
return false
}
for _, b := range [][]byte{
{0x50, 0x4B, 0x03, 0x04}, // zip
{0x1F, 0x9D}, // tar.z
{0x1F, 0xA0}, // tar.z
{0x42, 0x5A, 0x68}, // bz2
{0x1F, 0x8B, 0x08}, // gzip
} {
if bytes.HasPrefix(data, b) {
return true
}
}
switch {
// Unified TAR files have this magic number
case len(data) > 257+5 && bytes.Equal(data[257:257+5], []byte{0x75, 0x73, 0x74, 0x61, 0x72}):
return true
default:
return false
}
}

The tar archive made from the small git repo appears to contain only 213 bytes so the peek returns an error - https://golang.org/pkg/bufio/#Reader.Peek. The small tar archive has a prefix {0x1F, 0x8B, 0x08}, // gzip and the same prefix is if I take https://github.com/sclorg/s2i-nodejs-container

Despite the warning, it works well and build gets successfully created but the isArchive() could probably work for small repos too

@bparees
Copy link
Contributor

bparees commented Jun 26, 2018

Despite the warning, it works well and build gets successfully created but the isArchive() could probably work for small repos too

or we should not do the check when --as-repo is passed since we know it's an archive then.

either way, doesn't need to be fixed here, but if you want to submit a separate PR to clean up startbuild.go, that'd be great. Thanks for looking into it.

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 26, 2018
@bparees
Copy link
Contributor

bparees commented Jun 29, 2018

/refresh

@wozniakjan wozniakjan closed this Jun 29, 2018
@wozniakjan wozniakjan reopened this Jun 29, 2018
@bparees
Copy link
Contributor

bparees commented Jun 29, 2018

/retest

@wozniakjan
Copy link
Contributor Author

/retest

#20136

@wozniakjan
Copy link
Contributor Author

/retest

flake SYNC REPOSITORIES and FORWARD PARAMETERS TO THE REMOTE HOST

@wozniakjan
Copy link
Contributor Author

/retest

3 similar comments
@wozniakjan
Copy link
Contributor Author

/retest

@wozniakjan
Copy link
Contributor Author

/retest

@wozniakjan
Copy link
Contributor Author

/retest

@wozniakjan wozniakjan force-pushed the test/extended/speedup_contextdir branch from 41d4db5 to f12e0fc Compare July 2, 2018 15:49
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2018
@bparees
Copy link
Contributor

bparees commented Jul 2, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, wozniakjan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@wozniakjan
Copy link
Contributor Author

/retest

failing unrelated tests

1 similar comment
@wozniakjan
Copy link
Contributor Author

/retest

failing unrelated tests

@wozniakjan
Copy link
Contributor Author

/retest

failing (different) unrelated tests

@wozniakjan
Copy link
Contributor Author

/refresh

@bparees
Copy link
Contributor

bparees commented Jul 6, 2018

/retest

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 6, 2018

@wozniakjan: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/cmd f12e0fc link /test cmd

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit f9bb6bf into openshift:master Jul 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/tests lgtm Indicates that a PR is ready to be merged. sig/developer-experience size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants