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

Bug 1956989: initialize framework.TestContext.IPFamily correctly #26140

Merged
merged 1 commit into from Jul 27, 2021

Conversation

fedepaol
Copy link
Member

@fedepaol fedepaol commented May 7, 2021

Some tests rely on the TestContext IPFamily field, that is being initialized in the upstream ginkgo suites but not in origin's one.
Here we add the initialization in InitTest so the test find the right value.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. vendor-update Touching vendor dir or related files labels May 7, 2021
@openshift-ci openshift-ci bot requested review from adambkaplan and csrwng May 7, 2021 14:09
@fedepaol
Copy link
Member Author

fedepaol commented May 7, 2021

/test e2e-metal-ipi-ovn-ipv6

1 similar comment
@fedepaol
Copy link
Member Author

fedepaol commented May 8, 2021

/test e2e-metal-ipi-ovn-ipv6

@fedepaol
Copy link
Member Author

fedepaol commented May 8, 2021

/retest

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2021
@fedepaol
Copy link
Member Author

fedepaol commented May 9, 2021

/test e2e-metal-ipi-ovn-ipv6

@fedepaol
Copy link
Member Author

fedepaol commented May 9, 2021

/retest

@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 10, 2021
@fedepaol fedepaol changed the title WIP: validate the fix for Bug 26140: initialize framework.TestContext.IPFamily correctly May 10, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 10, 2021

@fedepaol: An error was encountered searching for bug 26140 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message. code 101: Bug #26140 does not exist.

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 26140: initialize framework.TestContext.IPFamily correctly

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.

1 similar comment
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 10, 2021

@fedepaol: An error was encountered searching for bug 26140 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details.

Full error message. code 101: Bug #26140 does not exist.

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 26140: initialize framework.TestContext.IPFamily correctly

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.

if err != nil {
klog.Fatal("Error loading client: ", err)
}
TestContext.IPFamily = getDefaultClusterIPFamily(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to call getDefaultClusterIPFamily(); we already know whether the cluster is ipv4/ipv6/dual-stack based on reading the OCP network config. You can set context.IPFamily in initializeTestFramework in cmd/openshift-tests/provider.go based on config.HasIPv4/config.HasIPv6.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, this will avoid the need of hitting the apis since we are basing this on config.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, I am not totally sure the way I used hasIPv4 / v6 has the same semantic as checking the ipfamily of the primary clusterip (especially in case of a dual stack cluster).

@@ -94,6 +101,8 @@ func InitTest(dryRun bool) error {
TestContext.CreateTestingNS = createTestingNS

klog.V(2).Infof("Extended test version %s", version.Get().String())

e2e.Logf("Cluster IP family: %s", e2e.TestContext.IPFamily)
Copy link
Contributor

Choose a reason for hiding this comment

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

the upstream tests do this but it's basically spam. just kill that

Copy link
Member Author

Choose a reason for hiding this comment

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

Well it (or the lack of it) helped me triaging the bug :-)
Will remove

@@ -94,6 +94,7 @@ func InitTest(dryRun bool) error {
TestContext.CreateTestingNS = createTestingNS

klog.V(2).Infof("Extended test version %s", version.Get().String())

Copy link
Member

Choose a reason for hiding this comment

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

@fedepaol spurious change? might as well just remove this hunk.

context.IPFamily = "ipv4"
if config.HasIPv6 {
context.IPFamily = "ipv6"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

hm... I think I'd go with if config.HasIPv6 && !config.HasIPv4
also maybe add a comment pointing out that these strings come from kube e2e and are not equal to corev1.IPv4Protocol / corev1.IPv6Protocol

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, thanks

@danwinship
Copy link
Contributor

/retitle Bug 1956989: initialize framework.TestContext.IPFamily correctly

@openshift-ci openshift-ci bot changed the title Bug 26140: initialize framework.TestContext.IPFamily correctly Bug 1956989: initialize framework.TestContext.IPFamily correctly May 10, 2021
@openshift-ci openshift-ci bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label May 10, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 10, 2021

@fedepaol: This pull request references Bugzilla bug 1956989, which is invalid:

  • expected the bug to target the "4.8.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1956989: initialize framework.TestContext.IPFamily correctly

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 openshift-ci bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 10, 2021
@fedepaol
Copy link
Member Author

/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 12, 2021

@fedepaol: This pull request references Bugzilla bug 1956989, 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.8.0) matches configured target release for branch (4.8.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (anusaxen@redhat.com), skipping review request.

In response to this:

/bugzilla refresh

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.

@ardaguclu
Copy link
Member

/retest

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 26, 2021
Some tests rely on the TestContext IPFamily field, that is being
initialized in the upstream ginkgo suites but not in origin's one.
Here we add the initialization in initializeTestFramework based on
config.HasIPV6 / config.HasIPV4.

Signed-off-by: Federico Paolinelli <fpaoline@redhat.com>
@danwinship
Copy link
Contributor

/lgtm
/assign @knobunc

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 26, 2021
@knobunc
Copy link
Contributor

knobunc commented Jul 26, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, fedepaol, knobunc

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 26, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2021

@fedepaol: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-disruptive 244982a link /test e2e-gcp-disruptive
ci/prow/e2e-aws-jenkins 244982a link /test e2e-aws-jenkins

Full PR test history. Your PR dashboard.

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-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 52dd59d into openshift:master Jul 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2021

@fedepaol: All pull requests linked via external trackers have merged:

Bugzilla bug 1956989 has been moved to the MODIFIED state.

In response to this:

Bug 1956989: initialize framework.TestContext.IPFamily correctly

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.

@fedepaol
Copy link
Member Author

/cherry-pick release-4.8

@openshift-cherrypick-robot

@fedepaol: #26140 failed to apply on top of branch "release-4.8":

Applying: initializeTestFramework: initialize TextContext ip family
Using index info to reconstruct a base tree...
M	test/extended/util/annotate/generated/zz_generated.annotations.go
M	test/extended/util/annotate/rules.go
Falling back to patching base and 3-way merge...
Auto-merging test/extended/util/annotate/rules.go
CONFLICT (content): Merge conflict in test/extended/util/annotate/rules.go
Auto-merging test/extended/util/annotate/generated/zz_generated.annotations.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 initializeTestFramework: initialize TextContext ip family
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.8

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
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. 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. vendor-update Touching vendor dir or related files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants