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.

Sign up for GitHub

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

Bug 1798146: [baremetal] Ipv6 non virtual ip fix #1436

Merged
merged 2 commits into from
Feb 6, 2020
Merged

Bug 1798146: [baremetal] Ipv6 non virtual ip fix #1436

merged 2 commits into from
Feb 6, 2020

Conversation

celebdor
Copy link
Contributor

@celebdor celebdor commented Feb 3, 2020

Closes: #1420

- What I did
Make non_virtual_ip IPv6 compatible by checking the IPv6 route advertisements to complement the check for whether an address in in the VIPs subnet.

- How to verify it
Deploy OpenShift Baremetal on IPv6 with addresses provided by a DHCPv6 with Route Advertisement enabled and check that /etc/resolv.conf gets the right non VIP IPv6 address.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 3, 2020
@celebdor celebdor requested a review from bcrochet February 3, 2020 22:53
@celebdor
Copy link
Contributor Author

celebdor commented Feb 3, 2020

/cc @hardys @russellb @cybertron @yboaron

@hardys
Copy link
Contributor

hardys commented Feb 4, 2020

@russellb we discussed this yesterday in the context of a fix for kublet choosing the wrong IP - also note #1430 which I think this means rewriting in golang, perhaps we can create some common library which is consumed in both cases?

@hardys
Copy link
Contributor

hardys commented Feb 4, 2020 •
edited
Loading

This seemed to work on first boot of the worker, but then on a subsequent lease renewal I'm seeing:

$ /usr/local/bin/non_virtual_ip fd2e:6f44:5dd8:c956::5 fd2e:6f44:5dd8:c956:0:0:0:2 fd2e:6f44:5dd8:c956::4
SHDEBUG line= ::1 dev lo proto kernel metric 256 pref medium
SHDEBUG line= fd01:0:0:4::/64 dev k8s-worker-0.os proto kernel metric 256 pref medium
SHDEBUG line= fd01::/48 via fd01:0:0:4::1 dev k8s-worker-0.os metric 1024 pref medium
Traceback (most recent call last):
  File "/usr/local/bin/non_virtual_ip", line 193, in <module>
    main()
  File "/usr/local/bin/non_virtual_ip", line 179, in main
    iface_cidrs = list(interface_cidrs((non_host_scope, non_deprecated)))
  File "/usr/local/bin/non_virtual_ip", line 144, in interface_cidrs
    for route in (V6Route.from_line(rline) for rline in route_out.splitlines()):
  File "/usr/local/bin/non_virtual_ip", line 144, in <genexpr>
    for route in (V6Route.from_line(rline) for rline in route_out.splitlines()):
  File "/usr/local/bin/non_virtual_ip", line 62, in from_line
    return cls(**attrs)
TypeError: __init__() missing 1 required positional argument: 'proto'

Edit - added some debug, we can see the proto arg doesn't exist in the line for the last case

hardys
hardys reviewed Feb 4, 2020
View reviewed changes
templates/worker/00-worker/baremetal/files/baremetal-non-virtual-ip.yaml
if dest == 'default':
dest = '::/0'
attrs = dict(itertools.zip_longest(*[iter(items[1:])]*2, fillvalue=None))
attrs['destination'] = dest
Copy link
Contributor

Choose a reason for hiding this comment

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

proto isn't always set, so I think we need attrs.setdefault('proto', '') or similar here, to avoid a possible traceback with a line like fd01::/48 via fd01:0:0:4::1 dev k8s-worker-0.os metric 1024 pref medium

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @hardys. Will update

@celebdor
baremetal IPv6: Use route adv to find non-vip
cdd53fc 
In IPv6 DHCPv6 environments, the prefix for the IPv6 address that we
must use to bind services may be 128. In case that happens, the current
detection code will fail to consider the address as being in the right
subnet.

Thankfully, thanks to the Route Advertisements, we get the prefix
information we need from the routing table. This patch adds the extra
code to generate pseudo CIDRs that allow us to check if an address is in
the right subnet.

Signed-off-by: Antoni Segura Puimedon <[email protected]>
@celebdor
baremetal IPv6: filtering improvement
59c17b6 
This patch simplifies filtering by:
* Calling iface_cidrs only once,
* Filtering out IPv6 addresses marked with the *deprecated* flag
* Allowing multiple filters

Signed-off-by: Antoni Segura Puimedon <[email protected]>
@celebdor celebdor force-pushed the ipv6-non-virtual-ip branch from 6857645 to 59c17b6 Compare February 4, 2020 13:41
@hardys hardys mentioned this pull request Feb 4, 2020
Merged
@celebdor
Copy link
Contributor Author

celebdor commented Feb 4, 2020

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@celebdor: 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:

/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.

@hardys
Copy link
Contributor

hardys commented Feb 4, 2020

I tested this (on ipv6 only so far) and can confirm the latest revision works - this lgtm pending feedback from someone that it all still works OK with ipv4

@runcom
Copy link
Member

runcom commented Feb 4, 2020

can you link this PR to https://bugzilla.redhat.com/show_bug.cgi?id=1797647 as well?

@hardys
Copy link
Contributor

hardys commented Feb 4, 2020

/retitle Bug 1798146: [baremetal] Ipv6 non virtual ip fix

@openshift-ci-robot openshift-ci-robot changed the title Ipv6 non virtual ip Bug 1798146: [baremetal] Ipv6 non virtual ip fix Feb 4, 2020
@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 4, 2020
@openshift-ci-robot
Copy link
Contributor

@celebdor: This pull request references Bugzilla bug 1798146, which is invalid:

  • expected the bug to target the "4.4.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 1798146: [baremetal] Ipv6 non virtual ip fix

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.

@hardys
Copy link
Contributor

hardys commented Feb 4, 2020

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Feb 4, 2020
@openshift-ci-robot
Copy link
Contributor

@hardys: This pull request references Bugzilla bug 1798146, which is valid.

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.

@openshift-ci-robot openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Feb 4, 2020
@hardys
Copy link
Contributor

hardys commented Feb 4, 2020

can you link this PR to https://bugzilla.redhat.com/show_bug.cgi?id=1797647 as well?

We now have the following bz's

4.4: https://bugzilla.redhat.com/show_bug.cgi?id=1798146
4.3.z: https://bugzilla.redhat.com/show_bug.cgi?id=1797647

They were initially cloned the wrong way round but we reversed the depends on which hopefully the bot will accept.

@celebdor
Copy link
Contributor Author

celebdor commented Feb 4, 2020

@yboaron tested the previous iteration in IPv4.

@hardys
Copy link
Contributor

hardys commented Feb 5, 2020

I also tested this in ipv4 and can confirm it works so lgtm

@hardys
Copy link
Contributor

hardys commented Feb 5, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2020
@yboaron
Copy link
Contributor

yboaron commented Feb 5, 2020

/lgtm

@runcom
Copy link
Member

runcom commented Feb 5, 2020

/approve
/retest

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: celebdor, hardys, runcom, yboaron

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2020
@openshift-bot
Copy link
Contributor

/retest

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

7 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-merge-robot openshift-merge-robot merged commit e6c0c65 into openshift:master Feb 6, 2020
@openshift-ci-robot
Copy link
Contributor

@celebdor: All pull requests linked via external trackers have merged. Bugzilla bug 1798146 has been moved to the MODIFIED state.

In response to this:

Bug 1798146: [baremetal] Ipv6 non virtual ip fix

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.

@hardys
Copy link
Contributor

hardys commented Feb 6, 2020

/cherrypick release-4.3

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Feb 6, 2020
Closed
@openshift-cherrypick-robot
Copy link

@hardys: new pull request created: #1445

In response to this:

/cherrypick release-4.3

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

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

Test name Commit Details Rerun command
ci/prow/e2e-aws-scaleup-rhel7 59c17b6 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@celebdor celebdor deleted the ipv6-non-virtual-ip branch February 6, 2020 14:12
@russellb russellb mentioned this pull request Feb 12, 2020
Merged
mandre added a commit to mandre/machine-config-operator that referenced this pull request Feb 18, 2020
@mandre
OpenStack IPv6: Use route adv to find non-vip
b55c850 
In IPv6 DHCPv6 environments, the prefix for the IPv6 address that we
must use to bind services may be 128. In case that happens, the current
detection code will fail to consider the address as being in the right
subnet.

Thankfully, thanks to the Route Advertisements, we get the prefix
information we need from the routing table. This patch adds the extra
code to generate pseudo CIDRs that allow us to check if an address is
in the right subnet.

This ports the BM fix from
openshift#1436 to
OpenStack platform.
@mandre mandre mentioned this pull request Feb 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardys hardys hardys left review comments

@bcrochet bcrochet Awaiting requested review from bcrochet

@cgwalters cgwalters Awaiting requested review from cgwalters

@kikisdeliveryservice kikisdeliveryservice Awaiting requested review from kikisdeliveryservice

@cybertron cybertron Awaiting requested review from cybertron

@russellb russellb Awaiting requested review from russellb

@yboaron yboaron Awaiting requested review from yboaron

Assignees

@hardys hardys

@yboaron yboaron

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

baremetal: ipv6 non_virtual_ip for workers is empty
8 participants
@celebdor @hardys @openshift-ci-robot @runcom @yboaron @openshift-bot @openshift-cherrypick-robot @openshift-merge-robot