Skip to content

Conversation

@hardys
Copy link

@hardys hardys commented Sep 16, 2020

Currently if we switch between ipv4 and ipv6 testing we get two
different SDN implementations, it may be better to make this
consistent, or e.g we lose all OVN coverage while CI is temporarily
switched to ipv4.

Currently if we switch between ipv4 and ipv6 testing we get two
different SDN implementations, it may be better to make this
consistent, or e.g we lose all OVN coverage while CI is temporarily
switched to ipv4.
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign hardys
You can assign the PR to them by writing /assign @hardys in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details 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

@hardys
Copy link
Author

hardys commented Sep 16, 2020

@stbenjam and @russellb - interested in your thoughts on this - @trozet noted that we've stopped testing OVN completely in CI since the temporary switch to ipv4 - it may be better to make the default consistent, and folks can select OpenShiftSDN explicitly if they need to test it?

@stbenjam
Copy link
Member

stbenjam commented Sep 16, 2020

I disagree, because then there's no coverage on OpenShiftSDN with e2e-metal-ipi. The idea behind recurring e2e-metal-ipi-ipv4 jobs is to test IPv4 + OpenShiftSDN. We may want to create more variants then, and maybe run them in more places.

IMHO we should've never been put in this position, breaking changes into OVN just to land code before artificial FF deadlines should've been rejected. I'd like to see a retrospective on this and ensure we don't do it again.

@stbenjam
Copy link
Member

stbenjam commented Sep 16, 2020

Should we rework our CI jobs naming? I believe @trozet asked for this before so it's clear what's under test.

In that scenario we'd end up with this:

  • e2e-metal-ipi (IPv4 + OpenShiftSDN)
  • e2e-metal-ipi-ovn (IPv4 + OVN)
  • e2e-metal-ipi-ovn-ipv6 (IPv6 + OVN)

That'd be done in the release repo, and possibly have all 3 jobs blocking on OCP releases. As for what gets tested on PR's, e2e-metal-ipi-ovn-ipv6 is probably the one that should get run.

Also: I'm not sure if that means we need to cover dualstack, virtualmedia, compact, and FIPS versions of all/many of those jobs as well, at least on some recurring basis.

@russellb
Copy link
Member

Should we rework our CI jobs naming? I believe @trozet asked for this before so it's clear what's under test.

Yes. :-)

In that scenario we'd end up with this:

In order of priority:

* e2e-metal-ipi-ovn-ipv6 (IPv6 + OVN)

Most important because no other platform can test IPv6 right now.

* e2e-metal-ipi-ovn (IPv4 + OVN)

Next most important based on customer usage. This combination can be tested in public clouds, but networking on bare metal is unique enough for some network focused jobs on metal, too.

* e2e-metal-ipi (IPv4 + OpenShiftSDN)

Same as above - networking focused jobs on metal are valuable, even if the same config is run elsewhere.

@stbenjam
Copy link
Member

stbenjam commented Sep 16, 2020

Ok if we'll do it that way then we can handle these changes in the release repo. It'll be a bit of work given how many repos run the "e2e-metal-ipi" job. We'll also need to update k8s testgrids and sippy.

@trozet
Copy link

trozet commented Sep 16, 2020

Yeah the problem is we are running an ambiguous. "metal-ipi" job in openshift/ovn-kubernetes that tests openshift sdn. Let's just have multiple jobs with proper sdn naming included in the job name as Russell suggested

@stbenjam
Copy link
Member

Yeah the problem is we are running an ambiguous. "metal-ipi" job in openshift/ovn-kubernetes that tests openshift sdn. Let's just have multiple jobs with proper sdn naming included in the job name as Russell suggested

FWIW, I was a bit surprised people weren't aware the job was IPv6 since the folks working on OVN+IPv6 were really closely involved in making it work, and requested the job to be enabled on the OVN repo to begin with.

Most platforms "e2e-XX-ipi" jobs represent the defaults for that platform which is why it had no special identifiers as part of the job name. We typically only name variants.

If it somehow helps people consider the importance of the jobs by naming them different, I'm happy to do it. IMHO it's only of marginal utility. Maybe on the OVN repo it helps, but for the rest of the repos I don't think people really care about the name, no one looks at optional jobs or cares if they break them, and the whole notion that "informers" and "optional" jobs have any value at all is dubious to me.

Anyway, I'll put together the PR's for the above naming changes. It might take me a few days. If we really need coverage on IPv4+OVN I'm happy to take this PR temporarily.

@hardys
Copy link
Author

hardys commented Sep 17, 2020

Ok it sounds like we can solve this by clearer naming so I'll go ahead and close this - @trozet as you mention it's temporarily confusing in the case of the ovn repo, but that's a conseqence of breaking ipv6 which hopefully we can resolve soon and switch back to the original OVN+ipv6 config (with improved naming)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants