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

NO-ISSUE: Fix auto VIPs allocation on local Nutanix flow #2423

Closed
wants to merge 1 commit into from
Closed

NO-ISSUE: Fix auto VIPs allocation on local Nutanix flow #2423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packer_files/vsphere_centos_template/variables.pkr.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ variable "iso_url" {

variable "iso_checksum" {
type = string
default = "9602c69c52d93f51295c0199af395ca0edbe35e36506e32b8e749ce6c8f5b60a"
default = "9602c69c52d93f51295c0199af395ca0edbe35e3650632b8e749ce6c8f5b60a"
description = "The Centos8 ISO checksum"
}

Expand Down
18 changes: 16 additions & 2 deletions src/assisted_test_infra/test_infra/controllers/node_controllers/nutanix_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,17 @@ def restart_node(self, node_name: str) -> None:
vm.power_on()

def get_ingress_and_api_vips(self):
if not self._entity_config.vip_dhcp_allocation:
"""
Need to distinguish between 3 cases:
1) vip_dhcp_allocation is set to False: Need to provide API and Ingress VIPs - raise an exception if
one or more are empty.
2) vip_dhcp_allocation is set to True: No need to provide API and Ingress VIP, return None.
3) vip_dhcp_allocation is not being set at all and its value is equal to None: In this case, search free IPs
and set them as VIPs. The behavior is the same as vip_dhcp_allocation = False but getting the IPs first.
Note that (3) is supposed to happen only locally due to the face that vip_dhcp_allocation is set in CI to some
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note that (3) is supposed to happen only locally due to the face that vip_dhcp_allocation is set in CI to some
Note that (3) is supposed to happen only locally due to the fact that vip_dhcp_allocation is set in CI to some

value.
"""
if self._entity_config.vip_dhcp_allocation is False:
if self._entity_config.api_vips is None or len(self._entity_config.api_vips) == 0:
raise ValueError("API VIP is not set")
if self._entity_config.ingress_vips is None or len(self._entity_config.ingress_vips) == 0:
Expand All @@ -66,6 +76,10 @@ def get_ingress_and_api_vips(self):
"ingress_vips": self._entity_config.ingress_vips,
}

elif self._entity_config.vip_dhcp_allocation is True:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
elif self._entity_config.vip_dhcp_allocation is True:
else:

should be enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, we need to differ True, from False from None.
else in this case will include the None case

Copy link
Contributor

@adriengentil adriengentil May 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can we add some doc at the beginning of the function to describe the 3 cases? I think it would help to understand what we try to achieve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is https://github.com/openshift/assisted-test-infra/pull/2423/files#diff-6016d1ff333f74b2b2703cf120015d2daea4cd905295107122b40f81a7b7e435R72

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can elaborate more if needed

Copy link
Contributor

@adriengentil adriengentil May 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, it's just because I find this structure a bit misleading to read:

def f(my_bool):
  if my_bool==true:
    return
  if my_bool==false
    return
  
  ... more code here because my_bool is none...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adriengentil I added a function documentation, LMK if that is OK

Copy link
Contributor

Choose a reason for hiding this comment

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

perfect, thanks! Just a typo.

return None

# If VIP DHCP Allocation is not set at all - search for free IPs and select addresses for VIPs
nutanix_subnet = next(
s for s in NutanixSubnet.list_entities(self._provider_client) if s.name == self._config.nutanix_subnet
)
Expand All @@ -91,7 +105,7 @@ def get_ingress_and_api_vips(self):
raise ConnectionError("Failed to locate free API and Ingress VIPs")

log.info(f"Found 2 optional VIPs: {free_ips}")
return {"api_vips": [free_ips.pop()], "ingress_vips": [free_ips.pop()]}
return {"api_vips": [{"ip": free_ips.pop()}], "ingress_vips": [{"ip": free_ips.pop()}]}

def set_boot_order(self, node_name, cd_first=False, cdrom_iso_path=None) -> None:
vm = self._get_provider_vm(tf_vm_name=node_name)
Expand Down