extended: fixed registry tests #15807
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-merge-robot
added
the
size/M
|
/assign @stevekuznetsov This is a second PR to address #15763. There will be one more to resolve: … which is a bit more difficult. |
|
/cc @dmage |
dmage
reviewed
Aug 17, 2017
test/extended/images/helper.go
Outdated
| if getErr == nil { | ||
| break | ||
| } | ||
| fmt.Fprintf(g.GinkgoWriter, "failed to %s %s: %v (%#+v)\n", req.Method, req.URL, err, err) |
There was a problem hiding this comment.
err, err? Does it make sense to print the same error in two different ways?
There was a problem hiding this comment.
If the error is a wrapper for some other errors, %#+v will not render the embedded string.
|
/lgtm |
test/extended/images/helper.go
Outdated
| resp *http.Response | ||
| getErr error | ||
| ) | ||
| for _, schema := range []string{"https", "http"} { |
There was a problem hiding this comment.
Instead of having logic here from the ordering of these in the list, could make it more explicit:
func MirrorBlobInRegistry(oc *exutil.CLI, dgst digest.Digest, repository string, timeout time.Duration) error {
// ...
c := http.Client{
Transport: knet.SetTransportDefaults(&http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
}),
}
func getBlob(schema string) (*http.Request, *http.Response, error) {
req, err := http.NewRequest("GET", fmt.Sprintf("%s://%s/v2/%s/blobs/%s", schema, registryURL, repository, dgst.String()), nil)
if err != nil {
return req, nil, err
}
req.Header.Set("range", "bytes=0-1")
req.Header.Set("Authorization", "Bearer "+token)
resp, err := c.Do(req)
return req, resp, err
}
var (
req *http.Request
resp *http.Response
getErr error
)
if req, resp, getErr = getBlob("https"); getErr != nil {
// fallback to http (why is this appropriate?)
if req, resp, getErr = getBlob("http"); getErr != nil {
fmt.Fprintf(g.GinkgoWriter, "failed to %s %s: %v (%#+v)\n", req.Method, req.URL, err, err)
return getErr
}
}
// ...
}There was a problem hiding this comment.
Good suggestion. Rewritten with a slight modification.
miminar
force-pushed
the
registry-extended-test-fixes
branch
from
3dafdd0
to
8fdb9ad
Compare
|
@stevekuznetsov please take a look again. |
|
@stevekuznetsov has |
|
/test extended_image_registry |
1 similar comment
|
/test extended_image_registry |
|
@miminar robot is misbehaving. Will be taken care of. |
|
/test extended_image_registry |
|
Working now! |
|
@stevekuznetsov 👍 🍺 |
|
Flake #16143 once again: @stevekuznetsov I thought you've disabled the check. Does it need to be disabled on its own for the extended test? |
|
Ugh -- we have an issue with an old version of the config being around in the base AMI right now, and the extended job does not explicitly refresh the repo -- new AMIs are being built but it may take a couple hours for them to be ready |
|
The extended registry tests got broken again (6 out of 14 fail). Not sure, what's the cause. I'll try to fix them in this PR.
|
|
/retest |
bparees
reviewed
Oct 11, 2017
| if err != nil { | ||
| return fmt.Errorf("failed to get registry pod: %v", err) | ||
| } | ||
| path, err := oc.Run("logs").Args("dc/docker-registry").OutputToFile("pod-" + pod.Name + ".log") |
There was a problem hiding this comment.
@jim-minter if you want to change the Pod log dumping logic to put it in a file, this looks like a promising way to do it instead of spamming the stdout.
There was a problem hiding this comment.
@bparees yes, but currently it ends up in /tmp, not as an artifact.
There was a problem hiding this comment.
well that's not very useful... and it makes me think @miminar should update this to dump to stdout then because this won't be accessible when the job is done, right?
(or use one of the existing DumpPodLogs methods)
There was a problem hiding this comment.
+1, @miminar I don't think this output will be retrievable when run in the CI system?
|
this lgtm but should probably be split into two prs, one that's changing the extended test infrastructure and one for the imageregistry test changes. I also want @stevekuznetsov and/or @smarterclayton to sign off on the extended test infrastructure changes. |
|
/unassign @dcbw |
miminar
force-pushed
the
registry-extended-test-fixes
branch
from
4b4ea2c
to
e8350eb
Compare
openshift-ci-robot
added
size/M
done. The reworked FOCUS commit has been extracted to #16806 /test extended_image_registry |
bparees
reviewed
Oct 11, 2017
| @@ -40,7 +40,8 @@ var _ = g.Describe("[Feature:ImageQuota] Image limit range", func() { | |||
| deleteTestImagesAndStreams(oc) | |||
| } | |||
|
|
|||
| g.It(fmt.Sprintf("should deny a push of built image exceeding %s limit", imageapi.LimitTypeImage), func() { | |||
| g.It(fmt.Sprintf("[Skipped] should deny a push of built image exceeding %s limit", imageapi.LimitTypeImage), func() { | |||
| g.Skip("FIXME: fill image metadata for schema1 in the registry") | |||
There was a problem hiding this comment.
do we have an issue for this? if not, please open one to track fixing this.
bparees
reviewed
Oct 11, 2017
| @@ -21,6 +22,7 @@ var _ = g.Describe("[imageapis][registry] image signature workflow", func() { | |||
| ) | |||
|
|
|||
| g.It("can push a signed image to openshift registry and verify it", func() { | |||
| g.Skip("FIXME: fix oadm verify-image-signature to work with secured registry") | |||
There was a problem hiding this comment.
same here, please open an issue to track fixing this if we don't have one.
|
@miminar please squash the commits and lgtm. (also please open issues to track enabling the currently skipped tests, if we don't have issues for that already) |
|
(and take the hold label off the PR assuming you're ready for it to be merged) |
|
/unassign @stevekuznetsov |
The extended test suite now secures the registry. This patch allows for secure connection to the registry. Mark few registry tests as serial. Prevent them from being run parallel with some other registry tests. Write registry log to file on re-deployment. The registry log is essential for externded test debugging. Without writing it to a file, this information will be lost. Skip image signature workflow test until we figure out, how to make `oadm verify-image-signature` work with secured integrated Docker registry. Issue openshift#16344. Temporarily skip limitrange_admission test. The image size counting is still broken for schema 1 - the layer sizes need to be filled on registry side. Will be fixed by openshift#16776. Signed-off-by: Michal Minář <miminar@redhat.com>
miminar
force-pushed
the
registry-extended-test-fixes
branch
from
e8350eb
to
9632e0e
Compare
miminar
removed
the
do-not-merge/hold
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, dmage, kargakis, miminar The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue. |
openshift-merge-robot
added a commit
that referenced
this pull request
Oct 15, 2017
Automatic merge from submit-queue. extended: log registry pod to artifacts directory To preserve it for post-CI-job-debugging. Follow-up for #15807 Addresses comment #15807 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The extended test suite now secures the registry. This patch allows for
secure connection to the registry.
Resolves #15763