Allow setting the lease expiry for DHCP by cybertron · Pull Request #599 · metal3-io/metal3-dev-env

cybertron · 2021-02-11T22:22:33Z

Newer versions of libvirt (6.3.0 and up) allow the lease expiry to
be set for hosts. We have some functionality that depends on using
infinite leases, so we would like to be able to set this through
metal3-dev-env.

This was added to metal3-dev-env in [0]. Expose the setting for dev-scripts too. 0: metal3-io/metal3-dev-env#599

hardys · 2021-02-25T13:24:44Z

/test-integration
/test-centos-integration

hardys · 2021-02-25T13:25:30Z

I triggered test-integration which may confirm - but do we know what libvirt version is expected in the supported Ubuntu release?

hardys · 2021-02-25T13:27:46Z

vm-setup/roles/libvirt/templates/network.xml.j2

        {% set hostname = hostname_format % num %}
-      <host mac='{{ node_mac_map.get(ironic_name).get(item.name)}}' name='{{hostname}}' ip='{{item.dhcp_range_v4[0]|ipmath(ns.index|int)}}'/>
+      <host mac='{{ node_mac_map.get(ironic_name).get(item.name)}}' name='{{hostname}}' ip='{{item.dhcp_range_v4[0]|ipmath(ns.index|int)}}'>
+        <lease expiry='{{ item.lease_expiry }}'/>


Should this be conditional on item.lease_expiry being defined?

I added a default for lease_expiry so it should always be set.

fmuyassarov · 2021-02-25T13:54:33Z

I triggered test-integration which may confirm - but do we know what libvirt version is expected in the supported Ubuntu release?

Description above says we need libvirt 6.3.0+. I just checked from the CI, and Ubuntu 20.04 based CI shows that we have 6.0.0 version,

ubuntu@ci-test-vm-20210225133921-xphj:~$ sudo virsh --version
6.0.0

which is installed here. I'm wondering if we can install 7.0.0 version on Ubuntu by specifying the version explicitly(UPDATED) during installation.

cybertron

I've been using this locally for a while now and have noticed a couple of odd things with the advanced-virtualization version of libvirt. Neither prevent use of it, but I'm wondering if we don't want to force installation of the newer version and just document what version of libvirt is needed for this particular feature? That would save us from having to deal with the distro-specific methods of getting the correct version.

cybertron · 2021-02-25T15:35:23Z

vm-setup/roles/libvirt/templates/network.xml.j2

        {% set hostname = hostname_format % num %}
-      <host mac='{{ node_mac_map.get(ironic_name).get(item.name)}}' name='{{hostname}}' ip='{{item.dhcp_range_v4[0]|ipmath(ns.index|int)}}'/>
+      <host mac='{{ node_mac_map.get(ironic_name).get(item.name)}}' name='{{hostname}}' ip='{{item.dhcp_range_v4[0]|ipmath(ns.index|int)}}'>
+        <lease expiry='{{ item.lease_expiry }}'/>


I added a default for lease_expiry so it should always be set.

maelk · 2021-02-27T16:25:14Z

Could you please elaborate on the use case you are trying to cover ? as is now, there would not be any choice from the user point of view (except if the user is using the roles directly, not via the high level scripts). Would it make sense to introduce a configuration variable and pass it to ansible ?

cybertron · 2021-03-02T17:08:50Z

I'm looking to support setting infinite leases from dev-scripts. I have a patch up that makes use of this configuration: openshift-metal3/dev-scripts#1191

I'm not sure how broadly applicable this is. It's for an OpenShift feature that does some special handling of infinite leases, which I think is a little unusual. If you think others might want to use it I can wire something in though.

maelk · 2021-03-15T14:34:37Z

If it is openshift specific, then maybe it should not be added here

cybertron · 2021-03-16T17:41:47Z

OpenShift is based on metal3 though. Without this we can't use metal3-dev-env to do development and testing.

hardys · 2021-03-25T11:20:58Z

@cybertron IIRC this is actually testing a feature of NetworkManager, so it's not specific to OpenShift (although the requirement originated there)?

@maelk the dev-scripts repo has some dependencies on metal3-dev-env, specifically centos_install_requirements.sh and the ansible roles - we could fork those pieces but the idea is to reduce duplicated effort when we need basically the exact same test setup for upstream metal3 testing, and metal3-enabled OpenShift.

fmuyassarov · 2021-03-25T11:24:13Z

@cybertron IIRC this is actually testing a feature of NetworkManager, so it's not specific to OpenShift (although the requirement originated there)?

@maelk the dev-scripts repo has some dependencies on metal3-dev-env, specifically centos_install_requirements.sh and the ansible roles - we could fork those pieces but the idea is to reduce duplicated effort when we need basically the exact same test setup for upstream metal3 testing, and metal3-enabled OpenShift.

Note that centos_install_requirements.sh has been replaced by Ansible script in #574.

Newer versions of libvirt (6.3.0 and up) allow the lease expiry to be set for hosts. We have some functionality that depends on using infinite leases, so we would like to be able to set this through metal3-dev-env.

cybertron · 2021-03-29T19:51:28Z

I've removed the requirements part entirely. I'm not sure we should be forcing people to pull in a newer version of libvirt if they don't need it. Older releases of libvirt will just drop the lease configuration, and anyone who wants to use custom lease expirations can make sure they have the necessary libvirt themselves. That way it's a conscious choice, not something that might come as a surprise.

maelk · 2021-03-30T06:28:17Z

/test-integration

maelk · 2021-03-30T06:28:33Z

/lgtm

cybertron · 2021-03-31T19:40:22Z

Hmm, the centos job seems to have gotten stuck.

/test-centos-integration

furkatgofurov7 · 2021-06-11T10:55:15Z

Hi @cybertron seems there was a glitch in CI by that time, re-triggering all tests once again since we have had changes recently to run integration tests.

/test-centos-integration
/test-integration
/test-v1a4-centos-integration
/test-v1a4-integration

furkatgofurov7

/lgtm

cybertron · 2021-06-11T20:04:44Z

Hi @cybertron seems there was a glitch in CI by that time, re-triggering all tests once again since we have had changes recently to run integration tests.

Thanks, I've been meaning to ask about this. Looks like it's still working!

hardys · 2021-06-24T10:04:40Z

/approve

metal3-io-bot · 2021-06-24T10:04:44Z

[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

Details

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

This was added to metal3-dev-env in [0]. Expose the setting for dev-scripts too. 0: metal3-io/metal3-dev-env#599

* Support lease expiry option This was added to metal3-dev-env in [0]. Expose the setting for dev-scripts too. 0: metal3-io/metal3-dev-env#599 * Bump to current metal3-dev-env

metal3-io-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 11, 2021

cybertron added a commit to cybertron/dev-scripts that referenced this pull request Feb 17, 2021

Support lease expiry option

dd4e82c

This was added to metal3-dev-env in [0]. Expose the setting for dev-scripts too. 0: metal3-io/metal3-dev-env#599

cybertron mentioned this pull request Feb 17, 2021

Support lease expiry option openshift-metal3/dev-scripts#1191

Merged

hardys reviewed Feb 25, 2021

View reviewed changes

cybertron commented Feb 25, 2021

View reviewed changes

Allow setting the lease expiry for DHCP

f7bd923

Newer versions of libvirt (6.3.0 and up) allow the lease expiry to be set for hosts. We have some functionality that depends on using infinite leases, so we would like to be able to set this through metal3-dev-env.

cybertron force-pushed the infinite-leases branch from 520534b to f7bd923 Compare March 29, 2021 19:39

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 30, 2021

furkatgofurov7 reviewed Jun 11, 2021

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 24, 2021

metal3-io-bot merged commit e5269d7 into metal3-io:master Jun 24, 2021

hardys pushed a commit to hardys/dev-scripts that referenced this pull request Aug 27, 2021

Support lease expiry option

17e62d2

This was added to metal3-dev-env in [0]. Expose the setting for dev-scripts too. 0: metal3-io/metal3-dev-env#599

cybertron added a commit to cybertron/dev-scripts that referenced this pull request Aug 30, 2021

Support lease expiry option

6535821

This was added to metal3-dev-env in [0]. Expose the setting for dev-scripts too. 0: metal3-io/metal3-dev-env#599

Conversation

cybertron commented Feb 11, 2021

Uh oh!

hardys commented Feb 25, 2021

Uh oh!

hardys commented Feb 25, 2021

Uh oh!

hardys Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

cybertron Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

fmuyassarov commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cybertron left a comment

Choose a reason for hiding this comment

Uh oh!

cybertron Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

maelk commented Feb 27, 2021

Uh oh!

cybertron commented Mar 2, 2021

Uh oh!

maelk commented Mar 15, 2021

Uh oh!

cybertron commented Mar 16, 2021

Uh oh!

hardys commented Mar 25, 2021

Uh oh!

fmuyassarov commented Mar 25, 2021

Uh oh!

cybertron commented Mar 29, 2021

Uh oh!

maelk commented Mar 30, 2021

Uh oh!

maelk commented Mar 30, 2021

Uh oh!

cybertron commented Mar 31, 2021

Uh oh!

furkatgofurov7 commented Jun 11, 2021

Uh oh!

furkatgofurov7 left a comment

Choose a reason for hiding this comment

Uh oh!

cybertron commented Jun 11, 2021

Uh oh!

hardys commented Jun 24, 2021

Uh oh!

metal3-io-bot commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

fmuyassarov commented Feb 25, 2021 •

edited

Loading