Adding node ip hint for all who want to use it #185

tsorya · 2022-07-06T16:35:42Z

No description provided.

openshift-ci · 2022-07-06T16:36:11Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

tsorya · 2022-07-06T16:36:48Z

@cybertron this is what you meant in openshift/machine-config-operator#3222 ?

dougsland

it seems a good patch but it's in Draft mode. Are you planing to update it?

tsorya · 2022-09-08T15:50:52Z

@dougsland waited for enhancement to be pushed

cmd/runtimecfg/node-ip.go

cybertron

Just a couple of things from my test in a dual stack env. Otherwise lgtm.

cmd/runtimecfg/node-ip.go

cybertron · 2022-09-13T17:02:58Z

cmd/runtimecfg/node-ip.go

+				return err
+			}
+			ipv6Created = true
+		} else if !ipv4Created {


This makes me a little nervous. If we ever got two ipv6 chosenAddresses, it could potentially result in us writing an ipv6 address to the ipv4 file. To avoid checking IsIPv6 twice (which, honestly, I think is premature optimization. This is a service that runs once per boot and is not performance critical code), I think we need that to be an outer check and then within the cases for it we do the ipv*Created checks. Something like:

if isipv6 { if !ipv6Created { create file } } else { if !ipv4Created { create file } }

Note that I don't think it's currently possible to get two ipv6 addresses in chosenAddresses, but if I'm wrong or that changes in the future I'd prefer to have this code handle it correctly.

This is actually a bug from my side . We should check if not ipv6 , this is why it was there from the start. Sorry

tsorya · 2022-09-14T11:39:23Z

/retest

cybertron

One question inline, but the logic looks good to me and this version worked correctly in my local dual stack deployment.

cybertron · 2022-09-14T14:30:17Z

cmd/runtimecfg/node-ip.go

@@ -142,7 +166,7 @@ func checkAddressUsable(chosen []net.IP) (err error) {
 	// If using IPv6, verify that the choosen address isn't tentative
 	// i.e. we can actually bind to it
 	if len(chosen) > 0 && net.IPv6len == len(chosen[0]) {
-		_, err = net.Listen("tcp", "["+chosen[0].String()+"]:")
+		_, err = net.Listen("tcp", "["+chosen[0].String()+"]:10010")


Did you mean to change this?

Added it as test and forgot to remove

Okay, I think we should remove it so we aren't adding another required port. Since ci has passed on this version, do you want to do that as a followup?

cmd/runtimecfg/node-ip.go

openshift-ci · 2022-09-14T21:54:29Z

@tsorya: all tests passed!

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.

cybertron · 2022-09-15T15:08:55Z

/lgtm
/approve

Checked locally in a dual stack env and this correctly populated all of the files. We should be good to go here.

openshift-ci · 2022-09-15T15:09:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cybertron, tsorya

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:

~~OWNERS~~ [cybertron]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2022

tsorya mentioned this pull request Jul 6, 2022

RFE-2962: configure ovs should use node-ip-hint set by nodeip-configuration service openshift/machine-config-operator#3233

Merged

openshift-ci bot requested review from cybertron and dougsland July 6, 2022 16:40

dougsland reviewed Aug 25, 2022

View reviewed changes

tsorya force-pushed the igal/ovs_config_node_ip_file branch from bb4381c to d330904 Compare September 6, 2022 10:12

tsorya marked this pull request as ready for review September 8, 2022 15:50

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 Sep 8, 2022

openshift-ci bot requested a review from creydr September 8, 2022 15:50

cybertron requested changes Sep 8, 2022

View reviewed changes

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

tsorya force-pushed the igal/ovs_config_node_ip_file branch 3 times, most recently from 15e5ebd to 21853a3 Compare September 9, 2022 10:40

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2022

tsorya force-pushed the igal/ovs_config_node_ip_file branch from 21853a3 to 311abcd Compare September 9, 2022 10:43

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2022

tsorya force-pushed the igal/ovs_config_node_ip_file branch from 311abcd to 7cee609 Compare September 9, 2022 10:50

cybertron requested changes Sep 9, 2022

View reviewed changes

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

tsorya force-pushed the igal/ovs_config_node_ip_file branch 2 times, most recently from cc9b257 to 05314b5 Compare September 10, 2022 16:10

rbbratta reviewed Sep 12, 2022

View reviewed changes

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

tsorya force-pushed the igal/ovs_config_node_ip_file branch from 05314b5 to 0facda2 Compare September 13, 2022 12:13

cybertron requested changes Sep 13, 2022

View reviewed changes

tsorya force-pushed the igal/ovs_config_node_ip_file branch from 0facda2 to 202759f Compare September 13, 2022 17:48

cybertron reviewed Sep 14, 2022

View reviewed changes

tsorya force-pushed the igal/ovs_config_node_ip_file branch from 202759f to ed5ecb5 Compare September 14, 2022 18:28

cybertron reviewed Sep 14, 2022

View reviewed changes

cmd/runtimecfg/node-ip.go Outdated Show resolved Hide resolved

RFE-2962: Adding node ip hint for all who want to use it

090fa86

tsorya force-pushed the igal/ovs_config_node_ip_file branch from ed5ecb5 to 090fa86 Compare September 14, 2022 19:18

openshift-ci bot assigned cybertron Sep 15, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2022

openshift-merge-robot merged commit 0504b5d into openshift:master Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding node ip hint for all who want to use it #185

Adding node ip hint for all who want to use it #185

tsorya commented Jul 6, 2022

openshift-ci bot commented Jul 6, 2022

tsorya commented Jul 6, 2022

dougsland left a comment

tsorya commented Sep 8, 2022

cybertron left a comment

cybertron Sep 13, 2022

tsorya Sep 13, 2022 •

edited

Loading

tsorya commented Sep 14, 2022

cybertron left a comment

cybertron Sep 14, 2022

tsorya Sep 14, 2022

cybertron Sep 14, 2022

openshift-ci bot commented Sep 14, 2022

cybertron commented Sep 15, 2022

openshift-ci bot commented Sep 15, 2022

Adding node ip hint for all who want to use it #185

Adding node ip hint for all who want to use it #185

Conversation

tsorya commented Jul 6, 2022

openshift-ci bot commented Jul 6, 2022

tsorya commented Jul 6, 2022

dougsland left a comment

Choose a reason for hiding this comment

tsorya commented Sep 8, 2022

cybertron left a comment

Choose a reason for hiding this comment

cybertron Sep 13, 2022

Choose a reason for hiding this comment

tsorya Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

tsorya commented Sep 14, 2022

cybertron left a comment

Choose a reason for hiding this comment

cybertron Sep 14, 2022

Choose a reason for hiding this comment

tsorya Sep 14, 2022

Choose a reason for hiding this comment

cybertron Sep 14, 2022

Choose a reason for hiding this comment

openshift-ci bot commented Sep 14, 2022

cybertron commented Sep 15, 2022

openshift-ci bot commented Sep 15, 2022

tsorya Sep 13, 2022 •

edited

Loading