Support lease expiry option

[cybertron]

This was added to metal3-dev-env in [0]. Expose the setting for
dev-scripts too.

With the current version of libvirt available, we are not able to run
virsh net-dhcp-leases when infinite leases are enabled. I've added
logic to skip that step when LEASE_EXPIRY=0. It just means we
can't retrieve clouds.yaml from the bootstrap.

0: metal3-io/metal3-dev-env#599

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign cybertron after the PR has been reviewed.
You can assign the PR to them by writing /assign @cybertron in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[hardys]

Sorry this has been around for so long @cybertron - I'm just testing a metal3-dev-env patch, and noticed that we need this because the lease_expiry option is mandatory now in metal3-dev-env.

Could you update https://github.com/openshift-metal3/dev-scripts/blob/master/01_install_requirements.sh#L22 to pin to a recent/latest version which contains the lease-expiry change please?

I also needed the following galaxy role:

--- a/01_install_requirements.sh
+++ b/01_install_requirements.sh
@@ -43,7 +43,7 @@ sudo alternatives --set python /usr/bin/python3
 
 pushd ${METAL3_DEV_ENV_PATH}
 ansible-galaxy install -r vm-setup/requirements.yml
-ansible-galaxy collection install ansible.netcommon ansible.posix
+ansible-galaxy collection install ansible.netcommon ansible.posix community.general

[hardys]

Also it'd probably be good to add the new variable to config_example.sh - and do you think we should perhaps prefix the name e.g DHCP_LEASE_EXPIRY ?

[cybertron]

Sorry this has been around for so long @cybertron - I'm just testing a metal3-dev-env patch, and noticed that we need this because the lease_expiry option is mandatory now in metal3-dev-env.

Oops, I'm not sure I meant it to work that way. :-/

Could you update https://github.com/openshift-metal3/dev-scripts/blob/master/01_install_requirements.sh#L22 to pin to a recent/latest version which contains the lease-expiry change please?

Sure.

I also needed the following galaxy role:

--- a/01_install_requirements.sh
+++ b/01_install_requirements.sh
@@ -43,7 +43,7 @@ sudo alternatives --set python /usr/bin/python3
 
 pushd ${METAL3_DEV_ENV_PATH}
 ansible-galaxy install -r vm-setup/requirements.yml
-ansible-galaxy collection install ansible.netcommon ansible.posix
+ansible-galaxy collection install ansible.netcommon ansible.posix community.general

Interesting. I think that's unrelated to this patch, but I can include it in the metal3-dev-env bump commit.

Also it'd probably be good to add the new variable to config_example.sh - and do you think we should perhaps prefix the name e.g DHCP_LEASE_EXPIRY ?

Ah, good point. I'm okay with prepending DHCP to the name too.

[hardys]

Sorry this has been around for so long @cybertron - I'm just testing a metal3-dev-env patch, and noticed that we need this because the lease_expiry option is mandatory now in metal3-dev-env.

Oops, I'm not sure I meant it to work that way. :-/

It's because we unconditionally access the value here https://github.com/metal3-io/metal3-dev-env/blob/master/vm-setup/roles/libvirt/templates/network.xml.j2#L45

I'm fine if we just land this PR so dev-scripts matches, but an alternative would be to make that templating conditional on the lease_expiry existing in the network definition (or default the value in the template I guess)

Could you update https://github.com/openshift-metal3/dev-scripts/blob/master/01_install_requirements.sh#L22 to pin to a recent/latest version which contains the lease-expiry change please?

Sure.

I also needed the following galaxy role:

--- a/01_install_requirements.sh
+++ b/01_install_requirements.sh
@@ -43,7 +43,7 @@ sudo alternatives --set python /usr/bin/python3
 
 pushd ${METAL3_DEV_ENV_PATH}
 ansible-galaxy install -r vm-setup/requirements.yml
-ansible-galaxy collection install ansible.netcommon ansible.posix
+ansible-galaxy collection install ansible.netcommon ansible.posix community.general

Interesting. I think that's unrelated to this patch, but I can include it in the metal3-dev-env bump commit.

Yeah, it's just related to the metal3-dev-env bump due to metal3-io/metal3-dev-env#711 - I'm not sure if we also want to remove the -vvv in dev-scripts, but at a minimum we need to install the community.general role AFAICS or the ansible run fails.

[hardys]

FYI I rebased this PR and updated the metal3-dev-env pin as discussed, and locally testing seems fine https://github.com/hardys/dev-scripts/commits/pr_1191

[cybertron]

Okay, once I remembered that you need a newer libvirt for this to have any effect I was able to verify that infinite leases still work as expected after the changes we discussed above. I think this should be good to go once I push the new version.

[cybertron]

/retest

[hardys]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys

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 [hardys]

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

[hardys]

/test e2e-metal-ipi-ovn-ipv6

[hardys]

/lgtm