Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
47 changes: 32 additions & 15 deletions cloudinit/sources/DataSourceOpenStack.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import logging
import time

from cloudinit import dmi, sources, url_helper, util
from cloudinit import dmi, net, sources, url_helper, util
from cloudinit.event import EventScope, EventType
from cloudinit.net.dhcp import NoDHCPLeaseError
from cloudinit.net.ephemeral import EphemeralDHCPv4
Expand Down Expand Up @@ -73,11 +73,9 @@ def __str__(self):
mstr = "%s [%s,ver=%s]" % (root, self.dsmode, self.version)
return mstr

def wait_for_metadata_service(self):
def wait_for_metadata_service(self, iface):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please add typing hints for function returns and params to aid our mypy CI validation.

DEF_MD_URLS = [
"http://[fe80::a9fe:a9fe%25{iface}]".format(
iface=self.distro.fallback_interface
),
"http://[fe80::a9fe:a9fe%25{iface}]".format(iface=iface),
"http://169.254.169.254",
]
urls = self.ds_cfg.get("metadata_urls", DEF_MD_URLS)
Expand Down Expand Up @@ -158,14 +156,22 @@ def _get_data(self):
"""

if self.perform_dhcp_setup: # Setup networking in init-local stage.
try:

with EphemeralDHCPv4(
self.distro, self.distro.fallback_interface
):
results = self._crawl_metadata()
except (NoDHCPLeaseError, sources.InvalidMetaDataException) as e:
util.logexc(LOG, str(e))
results = None
for iface in self.get_interface_list():
try:
with EphemeralDHCPv4(self.distro, iface):
results = self._crawl_metadata(iface)
break
except (
NoDHCPLeaseError,
sources.InvalidMetaDataException,
Comment on lines +166 to +167
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This approach aligns with what we have currently in GCE datasource with two things missing:

I think we want to distinguish NoDHCPLeaseError condition by logging "Unable to obtain a DHCP lease for "

) as e:
util.logexc(LOG, str(e))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Generally there is not a lot of value in logging the related traceback via util.logexc if we can valid it. Our common logging pattern gives is the module cloud-init is logging from, so the additional traceback info is nothing more than a lot of noise in our already busy logs. Since you are touching this area. Maybe we make an effort to LOG.debug("OpenStack metadata error: %s") vs "Unable to obtain DHCP lease on %s" etc.

if not results:
LOG.warning(
"All interfaces failed to get dhcp lease"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"All interfaces failed to get dhcp lease"
"All interfaces failed to get DHCP lease. "

"Metadata service will NOT be called"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"Metadata service will NOT be called"
" OpenStack metadata service will NOT be called"

)
return False
else:
try:
Expand Down Expand Up @@ -202,15 +208,17 @@ def _get_data(self):

return True

def _crawl_metadata(self):
def _crawl_metadata(self, iface=None):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since you are touching functions, please add type hints to function params and expected return values to aid in mypy verification.

"""Crawl metadata service when available.

@returns: Dictionary with all metadata discovered for this datasource.
@raise: InvalidMetaDataException on unreadable or broken
metadata.
"""
if not iface:
iface = self.distro.fallback_interface
try:
if not self.wait_for_metadata_service():
if not self.wait_for_metadata_service(iface):
raise sources.InvalidMetaDataException(
"No active metadata service found"
)
Expand Down Expand Up @@ -239,6 +247,15 @@ def _crawl_metadata(self):
raise sources.InvalidMetaDataException(msg) from e
return result

def get_interface_list(self):
ifaces = []
for iface in net.find_candidate_nics():
if "dummy" in iface:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if we want to promote this logic into net.find_candidate_nics directly. I see Vultr also attempts to ignore this iface name and I think it's generally irrelevant as a candidate.

continue
ifaces.append(iface)

return ifaces

def ds_detect(self):
"""Return True when a potential OpenStack platform is detected."""
accept_oracle = "Oracle" in self.sys_cfg.get("datasource_list")
Expand Down
107 changes: 102 additions & 5 deletions tests/unittests/sources/test_openstack.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import responses

from cloudinit import settings, util
from cloudinit.net.dhcp import NoDHCPLeaseError
from cloudinit.sources import UNSET, BrokenMetadata
from cloudinit.sources import DataSourceOpenStack as ds
from cloudinit.sources import convert_vendordata
Expand Down Expand Up @@ -76,6 +77,9 @@

MOCK_PATH = "cloudinit.sources.DataSourceOpenStack."

UNFILTERED_INTERFACES = ["lo", "dummy", "ens3", "ens4"]
FILTERED_INTERFACES = ["lo", "ens3", "ens4"]


@pytest.fixture(autouse=True)
def mock_is_resolvable():
Expand Down Expand Up @@ -329,10 +333,11 @@ def test_datasource(self, m_dhcp, ds_os):
m_dhcp.assert_not_called()

@responses.activate
@mock.patch("cloudinit.sources.DataSourceOpenStack.DataSourceOpenStack.get_interface_list")
@mock.patch("cloudinit.net.ephemeral.EphemeralIPv4Network")
@mock.patch("cloudinit.net.ephemeral.maybe_perform_dhcp_discovery")
@pytest.mark.usefixtures("disable_netdev_info")
def test_local_datasource(self, m_dhcp, m_net, paths, tmp_path):
def test_local_datasource(self, m_dhcp, m_net, m_get_ifaces, paths, tmp_path):
"""OpenStackLocal calls EphemeralDHCPNetwork and gets instance data."""
_register_uris(
self.VERSION,
Expand All @@ -346,14 +351,14 @@ def test_local_datasource(self, m_dhcp, m_net, paths, tmp_path):
ds_os_local = ds.DataSourceOpenStackLocal(
settings.CFG_BUILTIN, distro, paths
)
distro.fallback_interface = "eth9" # Monkey patch for dhcp
m_dhcp.return_value = {
"interface": "eth9",
"interface": FILTERED_INTERFACES[1],
"fixed-address": "192.168.2.9",
"routers": "192.168.2.1",
"subnet-mask": "255.255.255.0",
"broadcast-address": "192.168.2.255",
}
m_get_ifaces.return_value = [FILTERED_INTERFACES[1]]

assert ds_os_local.version is None
with mock.patch.object(
Expand All @@ -370,7 +375,7 @@ def test_local_datasource(self, m_dhcp, m_net, paths, tmp_path):
assert USER_DATA == ds_os_local.userdata_raw
assert 2 == len(ds_os_local.files)
assert ds_os_local.vendordata_raw is None
m_dhcp.assert_called_with(distro, "eth9", None)
m_dhcp.assert_called_with(distro, FILTERED_INTERFACES[1], None)

@responses.activate
def test_bad_datasource_meta(self, caplog, ds_os):
Expand Down Expand Up @@ -484,7 +489,7 @@ def test_wb__crawl_metadata_does_not_persist(self, ds_os):
OS_FILES,
responses_mock=responses,
)
crawled_data = ds_os._crawl_metadata()
crawled_data = ds_os._crawl_metadata(FILTERED_INTERFACES[1])
assert UNSET == ds_os.ec2_metadata
assert ds_os.userdata_raw is None
assert 0 == len(ds_os.files)
Expand Down Expand Up @@ -516,6 +521,98 @@ def test_wb__crawl_metadata_does_not_persist(self, ds_os):
assert VENDOR_DATA2 == crawled_data["vendordata2"]
assert 2 == crawled_data["version"]

# EphemeralDHCPv4 override
def override_enter(self):
return

# EphemeralDHCPv4 override
def override_exit(self, exc_type, exc_value, exc_tb):
return

@mock.patch('cloudinit.util.logexc')
@mock.patch(
"cloudinit.net.ephemeral.EphemeralDHCPv4.__init__",
side_effect=(NoDHCPLeaseError("Mock failed to get dhcp lease.")),
)
@mock.patch(
"cloudinit.net.ephemeral.EphemeralDHCPv4.__enter__", override_enter
)
@mock.patch(
"cloudinit.net.ephemeral.EphemeralDHCPv4.__exit__", override_exit
)
@mock.patch("cloudinit.sources.DataSourceOpenStack.DataSourceOpenStack.get_interface_list")
def test_local_datasource_no_dhcp_iface(
self,
m_interface_list,
m_eph_init,
m_logexc,
paths,
tmp_path
):
m_interface_list.return_value = FILTERED_INTERFACES[1:2]
distro = mock.MagicMock()
distro.get_tmp_exec_path = str(tmp_path)
ds_os_local = ds.DataSourceOpenStackLocal(
settings.CFG_BUILTIN, distro, paths
)

assert ds_os_local._get_data() == False
assert m_logexc.call_args[0][1] == "Mock failed to get dhcp lease."

@responses.activate
@mock.patch('cloudinit.util.logexc')
@mock.patch(
"cloudinit.net.ephemeral.EphemeralDHCPv4.__init__",
side_effect=[NoDHCPLeaseError("Mock failed to get dhcp lease."), None],
)
@mock.patch(
"cloudinit.net.ephemeral.EphemeralDHCPv4.__enter__", override_enter
)
@mock.patch(
"cloudinit.net.ephemeral.EphemeralDHCPv4.__exit__", override_exit
)
@mock.patch("cloudinit.sources.DataSourceOpenStack.DataSourceOpenStack._crawl_metadata")
@mock.patch("cloudinit.sources.DataSourceOpenStack.DataSourceOpenStack.get_interface_list")
def test_local_datasource_no_dhcp_and_dhcp_iface(
self,
m_interface_list,
m_crawl_meta,
m_eph_init,
m_logexc,
paths,
tmp_path
):
# setup
_register_uris(
self.VERSION,
EC2_FILES,
EC2_META,
OS_FILES,
responses_mock=responses,
)
m_crawl_meta.return_value = _read_metadata_service()
m_interface_list.return_value = FILTERED_INTERFACES[1:3]
distro = mock.MagicMock()
distro.get_tmp_exec_path = str(tmp_path)
ds_os_local = ds.DataSourceOpenStackLocal(
settings.CFG_BUILTIN, distro, paths
)

# eventually, we should succeed in getting data
assert ds_os_local._get_data() == True
# crawl meta should only be called once with 3rd iface
assert m_crawl_meta.call_count == 1
assert m_crawl_meta.call_args[0][0] == FILTERED_INTERFACES[2]
# first call to m_eph_init should fail, but we don't raise
# so we can only check via logexc
assert m_logexc.call_args[0][1] == "Mock failed to get dhcp lease."

@mock.patch("cloudinit.net.find_candidate_nics")
def test_get_interface_list(self, m_findnics, ds_os):
m_findnics.return_value = UNFILTERED_INTERFACES

assert ds_os.get_interface_list() == FILTERED_INTERFACES


class TestVendorDataLoading:
def cvj(self, data):
Expand Down