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

Support IPv6 on e2e StatefulSet tests #78871

Merged
merged 1 commit into from
Jun 24, 2019

Conversation

aojea
Copy link
Member

@aojea aojea commented Jun 10, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation

/kind failing-test

/kind feature
/kind flake

What this PR does / why we need it:

It adds IPv6 support to the [sig-apps] StatefulSet [k8s.io] Basic StatefulSet functionality [StatefulSetBasic] tests

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Nginx official docker images only listen on IPv4 by default, however, Apache httpd listen in both IPv4 and IPv6 and offers the same functionality for the e2e tests.

xref #70248

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2019
@k8s-ci-robot
Copy link
Contributor

Hi @aojea. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@aojea aojea changed the title E2e ipv6 stateful Support IPv6 on e2e StatefulSet tests Jun 10, 2019
@k8s-ci-robot k8s-ci-robot added area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 10, 2019
@aojea
Copy link
Member Author

aojea commented Jun 10, 2019

/assign @BenTheElder

@aojea
Copy link
Member Author

aojea commented Jun 10, 2019

/assign @neolit123
/assign @listx
/assign @nikhiljindal

@BenTheElder
Copy link
Member

what's the delta in image size? are we using httpd anywhere else in the tests? (pretty sure we use nginx frequently).

@BenTheElder
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 10, 2019
Copy link
Member

@neolit123 neolit123 left a comment

Choose a reason for hiding this comment

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

// NewNginxImage is the fully qualified URI to the NginxNew image
NewNginxImage = imageutils.GetE2EImage(imageutils.NginxNew)
// NewWebserverImage is the fully qualified URI to the Httpd22 image
NewWebserverImage = imageutils.GetE2EImage(imageutils.Httpd22)
Copy link
Member

Choose a reason for hiding this comment

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

the name assigned to Httpd22 seems to be NewWebserverImage
but 2.2-alpine is probably older than 2.4-alpine?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I have to read the test code again, don;t know if is new as in different or new as in newer

@aojea
Copy link
Member Author

aojea commented Jun 11, 2019

/hold

@BenTheElder good catch, the delta is big , almost 9x in my host, however dockerhub only repotrs 40MB for the httpd image 🤔

httpd                                            2-alpine                       48dffa369547        5 days ago          125MB
nginx                                            1.14-alpine                    cafef9fe265b        3 months ago        16MB

let me check if there is another way to use nginx with ipv6 without creating a new image

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2019
@aojea
Copy link
Member Author

aojea commented Jun 11, 2019

Grepping for nginx in the test folder I observe that there is a lot of tests using the nginx version. Those tests will fail with IPv6.
I think that the best path is to make nginx dual stack but I´m blocked and the only option I see is to recover this PR #75255 , any ideas?

EDIT

angost seems a good way to move forward :)
#78390

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 11, 2019
Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/approve

test/utils change

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019
@fejta
Copy link
Contributor

fejta commented Jun 21, 2019

/approve cancel

Can you actually get someone from sig-apps to review and approve this first? Once you do I can approve the test/utils (and test/e2e/framework) changes.

/assign janetkuo nikhiljindal kargakis mfojtik

@k8s-ci-robot k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2019
@aojea
Copy link
Member Author

aojea commented Jun 21, 2019

/hold
@fejta thanks for approving, will check with the other groups and try to align with the effort done in #78390

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2019
@aojea aojea force-pushed the e2e_ipv6_stateful branch 2 times, most recently from a0c1904 to 5601adf Compare June 22, 2019 17:36
@aojea
Copy link
Member Author

aojea commented Jun 22, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2019
Use httpd docker images instead of nginx because they listen
by default both in IPv4 and IPv6
@aojea
Copy link
Member Author

aojea commented Jun 23, 2019

@janetkuo could you review?
Seems that using apache docker images is the easier way to make the tests work for IPv4 and IPv6.

Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

@fejta the changes in tests/e2e/apps are just renames, (s/nginx/webserver/ || s/nginx/httpd/), the functional changes are in the framework code and are just:

  • use httpd instead of nginx
  • update the static html path mved to match httpd instead of nginx

This looks good to me, thanks!
/lgtm
/approve
/hold

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 23, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, BenTheElder

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2019
@fejta
Copy link
Contributor

fejta commented Jun 24, 2019

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3e0e2a2 into kubernetes:master Jun 24, 2019
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/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants