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

Jump to bottom

Bugs 1884421,1884422: Backport router hostindex fixes #25578

Closed
wants to merge 3 commits into from
Closed

Bugs 1884421,1884422: Backport router hostindex fixes #25578

wants to merge 3 commits into from

Conversation

Miciah
Copy link
Contributor

@Miciah Miciah commented Oct 1, 2020

Backport openshift/router#57, openshift/router#59, and openshift/router#126 (modulo potentially incompatible changes related to unit tests).

hostindex: Passthrough displaces path-based TLS

Ensure that a passthrough route displaces any path-based TLS routes with the same host, because passthrough is incompatible with path-based routing.

  • pkg/router/controller/hostindex/activation.go (hasExistingMatch): Return true if both routes are TLS and the existing route is a passthrough route.
  • pkg/router/controller/hostindex/hostindex_test.go (Test_hostIndex): Verify that a passthrough route displaces path-based TLS routes with the same host, but does not displace non-TLS routes.

hostindex: Path-based TLS displaces passthrough

Just as a passthrough route displaces any path-based TLS routes with the same host, a path-based TLS route displaces any passthrough route with the same host.

  • pkg/router/controller/hostindex/activation.go (hasExistingMatch): Return true if both routes are TLS and either route is passthrough.
  • pkg/router/controller/hostindex/hostindex_test.go (Test_hostIndex): Verify that a path-based TLS route displaces passthrough routes with the same host.

Try to promote inactive routes following route deletion

Before this patch, inactive routes were not being promoted when the conflicting routes were deleted. Fix it so that when deletes happen, all inactive routes are given a chance to be promoted.

Miciah and others added 3 commits October 1, 2020 18:30
@Miciah
hostindex: Passthrough displaces path-based TLS
3940dee 
Ensure that a passthrough route displaces any path-based TLS routes with
the same host, because passthrough is incompatible with path-based routing.

This commit fixes bug 1691190.

https://bugzilla.redhat.com/show_bug.cgi?id=1691190

* pkg/router/controller/hostindex/activation.go (hasExistingMatch): Return
true if both routes are TLS and the existing route is a passthrough route.
* pkg/router/controller/hostindex/hostindex_test.go (Test_hostIndex):
Verify that a passthrough route displaces path-based TLS routes with the
same host, but does not displace non-TLS routes.
@Miciah
hostindex: Path-based TLS displaces passthrough
83dcc4a 
Just as a passthrough route displaces any path-based TLS routes with the
same host, a path-based TLS route displaces any passthrough route with the
same host.

Follow-up to commit 65e784fe491ef02c4db0346348e6ac6192d68e30.

* pkg/router/controller/hostindex/activation.go (hasExistingMatch): Return
true if both routes are TLS and *either* route is passthrough.
* pkg/router/controller/hostindex/hostindex_test.go (Test_hostIndex):
Verify that a path-based TLS route displaces passthrough routes with the
same host.
@ironcladlou @Miciah
Try to promote inactive routes following route deletion
f503c5c 
Before this patch, inactive routes were not being promoted when the conflicting
routes were deleted. Fix it so that when deletes happen, all inactive routes are
given a chance to be promoted.
@openshift-ci-robot
Copy link

@Miciah: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Bugs 1884421, 1884422: Backport router hostindex fixes

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2020
@Miciah Miciah changed the title Bugs 1884421, 1884422: Backport router hostindex fixes Bugs 1884421,1884422: Backport router hostindex fixes Oct 1, 2020
@openshift-ci-robot
Copy link

@Miciah: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

Bugs 1884421,1884422: Backport router hostindex fixes

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.

@Miciah
Copy link
Contributor Author

Miciah commented Oct 2, 2020

/test unit

@openshift-ci-robot
Copy link

@Miciah: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/unit f503c5c link /test unit

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.

@sgreene570
Copy link

@Miciah unit tests are known to be very flakey on 3.11 right now
https://coreos.slack.com/archives/CDCP2LA9L/p1601572585167600

@frobware
Copy link
Contributor

@Miciah unit tests are known to be very flakey on 3.11 right now
https://coreos.slack.com/archives/CDCP2LA9L/p1601572585167600

Known flakes and fixes:

3.11 related BZs to address the flakes are:

@Miciah
Copy link
Contributor Author

Miciah commented Nov 3, 2020

/test unit

@sgreene570
Copy link

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@Miciah
Copy link
Contributor Author

Miciah commented Nov 13, 2020 

+ sudo yum install -y openshift-ansible-3.11.319-9999.git.0.ff3ad7a.el7
Loaded plugins: amazon-id, search-disabled-repos
No package openshift-ansible-3.11.319-9999.git.0.ff3ad7a.el7 available.
Error: Nothing to do

/test extended_conformance_install

@openshift-bot
Copy link
Contributor

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

19 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot
Copy link
Contributor

@Miciah: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_install f503c5c link /test extended_conformance_install

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.

@frobware
Copy link
Contributor

frobware commented Dec 1, 2020

/hold

Looking into the failures.

@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 Dec 1, 2020
@Miciah
Copy link
Contributor Author

Miciah commented Jan 4, 2021

Ansible fails on restarting the node. The origin-node.service journal has the following:

Dec 01 11:45:24 ip-172-18-4-64.ec2.internal origin-node[15608]: W1201 11:45:24.601752   15608 cni.go:172] Unable to update cni config: No networks found in /etc/cni/net.d
Dec 01 11:45:24 ip-172-18-4-64.ec2.internal origin-node[15608]: I1201 11:45:24.601951   15608 kubelet.go:2098] Container runtime status: Runtime Conditions: RuntimeReady=true reason: message:, NetworkReady=false reason:NetworkPluginNotReady message:docker: network plugin is not ready: cni config uninitialized
Dec 01 11:45:24 ip-172-18-4-64.ec2.internal origin-node[15608]: E1201 11:45:24.601976   15608 kubelet.go:2101] Container runtime network not ready: NetworkReady=false reason:NetworkPluginNotReady message:docker: network plugin is not ready: cni config uninitialized

/test extended_conformance_install

@Miciah
Copy link
Contributor Author

Miciah commented Feb 11, 2021 •
edited

Tests are all green, but some of them last ran in November, so let's rerun tests and try to get this PR approved.
/retest

@Miciah
Copy link
Contributor Author

Miciah commented Feb 11, 2021

/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 Feb 11, 2021
@Miciah
Copy link
Contributor Author

Miciah commented Feb 11, 2021

/close
Superseded by #25890 and #25891.

@openshift-ci-robot
Copy link

@Miciah: Closed this PR.

In response to this:

/close
Superseded by #25890 and #25891.

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

@knobunc knobunc Awaiting requested review from knobunc

@pweil- pweil- Awaiting requested review from pweil-

Assignees

@sgreene570 sgreene570

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
7 participants
@Miciah @openshift-ci-robot @sgreene570 @frobware @openshift-bot @openshift-merge-robot @ironcladlou