MGMT-3603 Changes for single node IPv6 #441

ori-amizur · 2021-01-18T09:46:06Z

Send machine CIDR to assisted-service in user-managed-networking single with IPv6
Set all dns names to point to the node IP address
/cc @empovit
/cc @tsorya

tsorya · 2021-01-18T10:10:35Z

discovery-infra/start_discovery.py

    cluster_info = client.cluster_get(cluster_id)
-    cluster_info.vip_dhcp_allocation = True
-    cluster_info.machine_network_cidr = machine_net.cidr_v4
+    if set_vip_dhcp_allocation:


no need for if ,just
cluster_info.vip_dhcp_allocation = set_vip_dhcp_allocation

Actually since python is typeless, prefer to set it to True explicitly

tsorya · 2021-01-18T10:13:00Z

discovery-infra/start_discovery.py

    client.update_cluster(cluster_id, cluster_info)

+def _get_ip_for_single_node_v6(client, cluster_id):


can we use this method ?
https://github.com/openshift/assisted-test-infra/blob/master/discovery-infra/test_infra/helper_classes/cluster.py#L660

i want to have only one place that get host ips

This one gets IP that belongs to the machine CIDR

but most of the logic is the same, so lets use the existing one and just verify machine-cidr ?

And also I want to verify that is is IPv6. Anyway, I will change

This approach will prefer IPv4 over IPv6. Not sure I want that

tsorya · 2021-01-18T10:15:41Z

discovery-infra/start_discovery.py

@@ -288,7 +302,7 @@ def _cluster_create_params():
    ipv4 = args.ipv4 and args.ipv4.lower() in MachineNetwork.YES_VALUES
    ipv6 = args.ipv6 and args.ipv6.lower() in MachineNetwork.YES_VALUES
    ntp_source = _get_host_ip_from_cidr(args.vm_network_cidr6 if ipv6 and not ipv4 else args.vm_network_cidr)
-    user_managed_networking = is_none_platform_mode()
+    user_managed_networking = is_user_managed_networking()


one of the ideas not to change this flag was to verify that there is no need to set it true in cluster create call

It seemed to me that the definition of is_user_managed_networking here was incorrect. It is not just for none platform but for single node as well

tsorya · 2021-01-18T10:16:56Z

discovery-infra/start_discovery.py

@@ -394,7 +408,7 @@ def nodes_flow(client, cluster_name, cluster):
        cluster_info = client.cluster_get(cluster.id)
        macs = utils.get_libvirt_nodes_macs(nodes_details["libvirt_network_name"])

-        if not (cluster_info.api_vip and cluster_info.ingress_vip) and args.master_count != 1:
+        if not (cluster_info.api_vip and cluster_info.ingress_vip):


not sure i understand this condition, maybe you know?

Whoever did it didn't want to set vips or machine-cidr for none platform

tsorya · 2021-01-18T10:44:22Z

terraform_files/baremetal/main.tf

@@ -162,24 +162,24 @@ resource "libvirt_domain" "worker" {

 data "libvirt_network_dns_host_template" "api" {
  count    = 1
-  ip       = var.bootstrap_in_place ? var.libvirt_master_ips[count.index][0] : var.api_vip
+  ip       = var.bootstrap_in_place && !var.ipv6 ? var.libvirt_master_ips[count.index][0] : var.api_vip


can you explain the logic please ?

When using ipv6, we do not know the ips before and we cannot set before. Therefore, for DNS, we set all to point to api-vip, and when the node IP is known, we set the api-vip to point to the node-ip. Here we just set all the DNS names to point to the API vip

it was added to support adding dns for single node and single node have no api vip. so i don't understand the logic

empovit · 2021-01-18T11:14:39Z

IMO this PR may interfere with static IPs, e.g. in how it handles the API VIP. What do you think @yevgeny-shnaidman ?

tsorya · 2021-01-18T18:18:35Z

/lgtm
/approve

openshift-ci-robot · 2021-01-18T18:18:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ori-amizur, 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~~ [tsorya]

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

empovit · 2021-01-18T18:50:46Z

discovery-infra/start_discovery.py

-    cluster_info.machine_network_cidr = machine_net.cidr_v4
+    if set_vip_dhcp_allocation:
+        cluster_info.vip_dhcp_allocation = True
+    cluster_info.machine_network_cidr = machine_net.cidr_v6 if machine_net.has_ip_v6 and not machine_net.has_ip_v4 else machine_net.cidr_v4


Can be simplified as machine_net.cird_v4 if machine_net.has_ip_v4 else machine_net.cidr_v6

It is not the same

openshift-bot · 2021-01-18T19:55:47Z

/retest

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

openshift-bot · 2021-01-18T20:08:46Z

/retest

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

empovit · 2021-01-18T20:09:18Z

discovery-infra/start_discovery.py

-            if args.vip_dhcp_allocation:
+            if args.master_count == 1 and machine_net.has_ip_v6 and not machine_net.has_ip_v4:
+                set_cluster_machine_cidr(client, cluster.id, machine_net, set_vip_dhcp_allocation=False)
+                tf.change_variables({"libvirt_master_ips": [[helper_cluster.Cluster.get_ip_for_single_node_v6(client, cluster.id, args.vm_network_cidr6)]]})


This line will break static IPs as it tries to update TF variables. Currently this causes an attempt to set a MAC address, which fails.

I believe if not args.with_static_ips: can solve it.

openshift-bot · 2021-01-19T01:07:47Z

/retest

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

openshift-bot · 2021-01-19T01:46:52Z

/retest

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

openshift-bot · 2021-01-19T03:30:50Z

/retest

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

openshift-bot · 2021-01-19T05:14:51Z

/retest

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

openshift-bot · 2021-01-19T05:27:53Z

/retest

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

openshift-bot · 2021-01-19T05:41:58Z

/retest

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

openshift-bot · 2021-01-19T08:29:50Z

/retest

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

ori-amizur · 2021-01-19T10:46:18Z

/retest

openshift-ci-robot requested review from empovit and tsorya January 18, 2021 09:46

tsorya reviewed Jan 18, 2021

View reviewed changes

ori-amizur requested a review from tsorya January 18, 2021 11:53

ori-amizur force-pushed the ori/MGMT-3603 branch 8 times, most recently from 59adc18 to d9f185a Compare January 18, 2021 17:50

MGMT-3603 Changes for single node IPv6

826092f

ori-amizur force-pushed the ori/MGMT-3603 branch from d9f185a to 826092f Compare January 18, 2021 17:57

openshift-ci-robot assigned tsorya Jan 18, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 18, 2021

empovit reviewed Jan 18, 2021

View reviewed changes

openshift-merge-robot merged commit a04110b into openshift:master Jan 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-3603 Changes for single node IPv6 #441

MGMT-3603 Changes for single node IPv6 #441

ori-amizur commented Jan 18, 2021

tsorya Jan 18, 2021

ori-amizur Jan 18, 2021

ori-amizur Jan 18, 2021

tsorya Jan 18, 2021

tsorya Jan 18, 2021

ori-amizur Jan 18, 2021

tsorya Jan 18, 2021

ori-amizur Jan 18, 2021

ori-amizur Jan 18, 2021

tsorya Jan 18, 2021

ori-amizur Jan 18, 2021

tsorya Jan 18, 2021

ori-amizur Jan 18, 2021

tsorya Jan 18, 2021

ori-amizur Jan 18, 2021 •

edited

Loading

tsorya Jan 18, 2021

empovit commented Jan 18, 2021

tsorya commented Jan 18, 2021

openshift-ci-robot commented Jan 18, 2021

empovit Jan 18, 2021

ori-amizur Jan 19, 2021

openshift-bot commented Jan 18, 2021

openshift-bot commented Jan 18, 2021

empovit Jan 18, 2021

empovit Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

ori-amizur commented Jan 19, 2021

		client.update_cluster(cluster_id, cluster_info)

		def _get_ip_for_single_node_v6(client, cluster_id):

MGMT-3603 Changes for single node IPv6 #441

MGMT-3603 Changes for single node IPv6 #441

Conversation

ori-amizur commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ori-amizur Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

empovit commented Jan 18, 2021

tsorya commented Jan 18, 2021

openshift-ci-robot commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Jan 18, 2021

openshift-bot commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

openshift-bot commented Jan 19, 2021

ori-amizur commented Jan 19, 2021

ori-amizur Jan 18, 2021 •

edited

Loading