OPNET-330: fix DualStackIPv6Primary #28292
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@rbbratta: This pull request references OPNET-329 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but no target version was set. DetailsIn response to this:
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. |
|
|
/cc @mkowalski |
rbbratta
force-pushed
the
fix-DualStackIPv6Primary
branch
from
6c3870e to
865ed5a
Compare
|
/retitle OPNET-330: fix DualStackIPv6Primary |
openshift-ci
bot
changed the title
|
@rbbratta: This pull request references OPNET-330 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but no target version was set. DetailsIn response to this:
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. |
| // IPFamily matches the kube e2e way | ||
| IPFamily string |
There was a problem hiding this comment.
If anything, I'd rather describe this variable like described upstream in https://github.com/kubernetes/kubernetes/blob/master/test/e2e/framework/test_context.go#L173, e.g.
IPFamily defines default IP stack of the cluster
| } | ||
| } | ||
| // IP order matters for DualStack. | ||
| for _, cidr := range state.NetworkSpec.ServiceNetwork { |
There was a problem hiding this comment.
Can we have if config.HasIPv6 && config.HasIPv4 before we run this loop? If this is something for dual-stack only, then let's run it only for dual-stack
Plus, as we are detecting primary stack based on the 1st network, would utilnet.IsIPv6CIDRString(state.NetworkSpec.ServiceNetwork[0]) suffice?
There was a problem hiding this comment.
Will shorten comment. This is for single and dual.
Plus, as we are detecting primary stack based on the 1st network, would utilnet.IsIPv6CIDRString(state.NetworkSpec.ServiceNetwork[0]) suffice?
I was trying to think of a way to combine the loops.
There was a problem hiding this comment.
This is for single and dual.
I miss something to understand how this added code is relevant for single-stack... For single-stack the previous loop (just above the code you add) should already correctly detect the IP stack, or no?
There was a problem hiding this comment.
The new loop replaces the old logic
// these constants are taken from kube e2e and used by tests
context.IPFamily = "ipv4"
if config.HasIPv6 && !config.HasIPv4 {
context.IPFamily = "ipv6"
}
There was a problem hiding this comment.
That's fair, thanks for explaining!
There was a problem hiding this comment.
avoided loop by assuming IPv4 default.
|
Old discusssion #26140 (review) |
rbbratta
force-pushed
the
fix-DualStackIPv6Primary
branch
2 times, most recently
from
2797fb0 to
5e652e8
Compare
|
/test e2e-vsphere-ovn-dualstack |
|
@rbbratta: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
DetailsIn response to this:
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. |
Since OPNET-198 we support IPv6 Primary cluster Correctly set IPFamily based on the first family found
rbbratta
force-pushed
the
fix-DualStackIPv6Primary
branch
from
5e652e8 to
8d8acd8
Compare
|
/retest-required |
|
/cc @cybertron |
|
/lgtm |
|
@rbbratta: This pull request references OPNET-330 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but no target version was set. DetailsIn response to this:
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. |
|
/cc @danwinship |
|
/test e2e-openstack-ovn The other jobs seem to be pretty much perma-failing, but these two seem like they should pass. |
|
@rbbratta: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
| if config.HasIPv6 && !config.HasIPv4 { | ||
| context.IPFamily = "ipv6" | ||
| } | ||
| // IPFamily constants are taken from kube e2e and used by tests |
There was a problem hiding this comment.
The comment here before was explaining "the reason we have the strings "ipv4" and "ipv6" hardcoded here is because that's what kube e2e uses, but it doesn't export them as constants we can import, so we have to hardcode them". So in the context of the rewrite, you should be explaining that in pkg/clioptions/clusterdiscovery/cluster.go. But maybe it would be better to add local constants for them (and explain there that they have to be the same as the upstream ones)? Or else use v1.Family for our config, and call strings.ToLower(string(config.PrimaryIPFamily)) here and comment that the upstream tests use the strings "ipv4" and "ipv6", which are also the lowercasified versions of the v1.Family constants.
| HasIPv4 bool | ||
| HasIPv6 bool | ||
| // IPFamily defines default IP stack of the cluster, replaces upstream getDefaultClusterIPFamily | ||
| IPFamily string |
There was a problem hiding this comment.
Seems like it should be PrimaryIPFamily?
Also, HasIPv4+HasIPv6+IPFamily is redundant (and allows impossible configurations like hasIPv6: false, ipFamily: "ipv6"). We don't want to get rid of the old fields, for compatibility, but it would probably be nicer to make this PrimaryIPFamily string (or PrimaryIPFamily v1.Family?) plus DualStack bool and then deprecate HasIPv4 and HasIPv6?
There was a problem hiding this comment.
IPFamily matches context.IPFamily Changing the name would confuse.
The only use case of context.IPFamily in e2e seems to beClusterIsIPv6 which is used frequently.
// ClusterIsIPv6 returns true if the cluster is IPv6
func (tc TestContextType) ClusterIsIPv6() bool {
return tc.IPFamily == "ipv6"
}So, assume IPv4 unless IPFamily is IPv6. Then to detect dualstack something else.
if c.IPFamily == "ipv6" {
skips = append(skips, "[Feature:Networking-IPv4]")
} else {
skips = append(skips, "[Feature:Networking-IPv6]")
}
if !c.HasIPv4 || !c.HasIPv6 {
// lack of "]" is intentional; this matches multiple tags
skips = append(skips, "[Feature:IPv6DualStack")
with consts.
There was a problem hiding this comment.
err, well
if !c.DualStack {
// lack of "]" is intentional; this matches multiple tags
skips = append(skips, "[Feature:IPv6DualStack")
// c.IPFamily can be empty, which == "ipv4"
if c.IPFamily != strings.ToLower(string(corev1.IPv6Protocol)) {
skips = append(skips, "[Feature:Networking-IPv6]")
} else {
skips = append(skips, "[Feature:Networking-IPv4]")
}
}
// else DualStack, run all the testsThere was a problem hiding this comment.
Also,
HasIPv4+HasIPv6+IPFamilyis redundant (and allows impossible configurations likehasIPv6: false, ipFamily: "ipv6").
I'm not sure what to do in the case of no discovery. The tc.Context has an IPFamily but we seem to want to skip ipv4 tests if there is no discovery, which implies that HasIPv4 can be false while IPFamily == "ipv4"
{
name: "complex override without discovery",
provider: `{"type":"aws","region":"us-east-2","zone":"us-east-2a","multimaster":false,"multizone":true}`,
discoveredPlatform: nil,
optionalCapabilities: configv1.KnownClusterVersionCapabilities,
expectedConfig: `{"type":"aws","ProjectID":"","Region":"us-east-2","Zone":"us-east-2a","NumNodes":0,"MultiMaster":false,"MultiZone":true,"Zones":null,"ConfigFile":"","Disconnected":false,"SingleReplicaTopology":false,"NetworkPlugin":"","HasIPv4":false,"HasIPv6":false,"HasSCTP":false,"IsProxied":false,"IsIBMROKS":false,"HasNoOptionalCapabilities":false}`,
runTests: sets.NewString("everyone", "not-gce", "not-sdn", "not-multitenant", "online", "requires-optional-cap"),
},
knobunc
added
the
approved
|
@rbbratta: This pull request references OPNET-330 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.15.0" version, but no target version was set. DetailsIn response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: cybertron, mkowalski, rbbratta The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
6 participants
Since OPNET-198 we support IPv6 Primary cluster
Correctly set IPFamily based on the first family found