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.

Sign up for GitHub

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

Jump to bottom

Bug 1826996: Adds DNS Forwarding e2e Test #168

Merged
merged 1 commit into from Apr 30, 2020
Merged

Bug 1826996: Adds DNS Forwarding e2e Test #168

merged 1 commit into from Apr 30, 2020

Conversation

danehans
Copy link
Contributor

Adds an end-to-end test for the DNS Forwarding feature.

In support of NE-257

/assign @ironcladlou @knobunc
/cc @Miciah @frobware

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2020
ironcladlou
ironcladlou suggested changes Apr 20, 2020
View reviewed changes
test/e2e/operator_test.go Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated
}

// Create the client Pod.
testClient := clientPod("test-client", "default", "openshift/origin-cli:latest", []string{"sleep", "3600"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to discover the client image that's part of the payload under test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... @ironcladlou is there a resource I can query to get the client image tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ironcladlou I'm now discovering the openshift-cli image through the clusteroperator/dns resource here.

test/e2e/operator_test.go Show resolved Hide resolved
@danehans danehans force-pushed the fwding_e2e branch 3 times, most recently from 3802325 to fcfd1e3 Compare April 21, 2020 18:01
@danehans
Copy link
Contributor Author

level=error msg="Error: error updating S3 Bucket (terraform-20200421180927410600000001) tags: error listing resource tags (terraform-20200421180927410600000001): NoSuchBucket: The specified bucket does not exist"

/test e2e-aws-operator

@danehans
Copy link
Contributor Author

/retitle Bug 1826996: Adds DNS Forwarding e2e Test

@openshift-ci-robot openshift-ci-robot changed the title Adds DNS Forwarding e2e Test Bug 1826996: Adds DNS Forwarding e2e Test Apr 23, 2020
@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Apr 23, 2020
@openshift-ci-robot
Copy link
Contributor

@danehans: This pull request references Bugzilla bug 1826996, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.5.0) matches configured target release for branch (4.5.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826996: Adds DNS Forwarding e2e Test

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.

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 23, 2020
Miciah
Miciah reviewed Apr 23, 2020
View reviewed changes
Copy link
Contributor

@Miciah Miciah left a comment

Choose a reason for hiding this comment

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

I have a lot of suggestions, but they are all pretty minor to trivial. I would be already with lgtm'ing this as-is.

test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated
{
Name: "dns",
ContainerPort: int32(5353),
Protocol: corev1.Protocol("UDP"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use corev1.ProtocolUDP here (and corev1.ProtocolTCP below)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using corev1.ProtocolUDP and corev1.ProtocolTCP here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, why not change corev1.Protocol("UDP") to corev1.ProtocolUDP? It isn't too important, but you might as well use the named value instead of casting a string.

test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
@ironcladlou
Copy link
Contributor

lgtm, @Miciah want to tag if you're okay given your existing feedback?

Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/operator_test.go Outdated Show resolved Hide resolved
Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/operator_test.go Outdated Show resolved Hide resolved
Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/utils.go Outdated Show resolved Hide resolved
Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/utils.go Outdated Show resolved Hide resolved
Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/operator_test.go Outdated Show resolved Hide resolved
Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/operator_test.go Outdated Show resolved Hide resolved
Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/operator_test.go Outdated Show resolved Hide resolved
Miciah
Miciah reviewed Apr 27, 2020
View reviewed changes
test/e2e/operator_test.go Outdated Show resolved Hide resolved
@danehans danehans force-pushed the fwding_e2e branch 2 times, most recently from 57e06d1 to d039c22 Compare April 28, 2020 00:51
@danehans
Copy link
Contributor Author

e2e-aws-upgrade:

Unexpected error:
    <*errors.errorString | 0xc0014cbe70>: {
        s: "Cluster did not complete upgrade: timed out waiting for the condition: Cluster operator openshift-apiserver is reporting a failure: APIServerDeploymentDegraded: 1 of 3 requested instances are unavailable for apiserver.openshift-apiserver",
    }

/test e2e-aws-upgrade

Miciah
Miciah reviewed Apr 29, 2020
View reviewed changes
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/operator_test.go Show resolved Hide resolved
test/e2e/operator_test.go Outdated Show resolved Hide resolved
test/e2e/utils.go Outdated Show resolved Hide resolved
@Miciah
Copy link
Contributor

Miciah commented Apr 29, 2020

/lgtm
/hold
No major issues. Any remaining issues can be ignored or deferred to a clean-up at your discretion. Feel free the /hold cancel if you want to go ahead and merge now.

@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 Apr 29, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2020
@danehans
Copy link
Contributor Author

prow/images failed due to:

error: could not run steps: step cluster-dns-operator failed: could not wait for build: the build cluster-dns-operator failed after 1m37s with reason BuildPodDeleted: The pod for this build was deleted before the build completed.

/test images

@danehans
Copy link
Contributor Author

prow/images failed due to:

error: unable to signal to artifacts container to terminate in pod release-latest, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")

/test images

@Miciah
Copy link
Contributor

Miciah commented Apr 30, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danehans, Miciah

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

@Miciah
Copy link
Contributor

Miciah commented Apr 30, 2020

/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 Apr 30, 2020
@openshift-merge-robot openshift-merge-robot merged commit 8e09841 into openshift:master Apr 30, 2020
@openshift-ci-robot
Copy link
Contributor

@danehans: All pull requests linked via external trackers have merged: openshift/cluster-dns-operator#168. Bugzilla bug 1826996 has been moved to the MODIFIED state.

In response to this:

Bug 1826996: Adds DNS Forwarding e2e Test

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Miciah Miciah Miciah left review comments

@ironcladlou ironcladlou ironcladlou requested changes

@frobware frobware Awaiting requested review from frobware

Assignees

@ironcladlou ironcladlou

@Miciah Miciah

@knobunc knobunc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@danehans @openshift-ci-robot @ironcladlou @Miciah @knobunc @openshift-merge-robot