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

Fix: adding new ips with inventory builder (#7577) #7583

Merged
merged 4 commits into from
Sep 10, 2021
Merged

Fix: adding new ips with inventory builder (#7577) #7583

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
78185ef
Fix: adding new ips with inventory builder (#7577)
VladMasarik May 2, 2021
bf2286a
moved conflig loading logic
VladMasarik May 5, 2021
e7860f5
added check for removing nodes from config
VladMasarik May 5, 2021
72b8e82
Fix tox errors
VladMasarik May 5, 2021
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
67 changes: 48 additions & 19 deletions contrib/inventory_builder/inventory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@
'calico_rr']
PROTECTED_NAMES = ROLES
AVAILABLE_COMMANDS = ['help', 'print_cfg', 'print_ips', 'print_hostnames',
'load']
'load', 'add']
_boolean_states = {'1': True, 'yes': True, 'true': True, 'on': True,
'0': False, 'no': False, 'false': False, 'off': False}
yaml = YAML()
Expand Down Expand Up @@ -82,22 +82,30 @@ class KubesprayInventory(object):
def __init__(self, changed_hosts=None, config_file=None):
self.config_file = config_file
self.yaml_config = {}
if self.config_file:
loadPreviousConfig = False
if self.config_file: # Load previous YAML file
try:
self.hosts_file = open(config_file, 'r')
self.yaml_config = yaml.load_all(self.hosts_file)
except OSError:
pass

self.yaml_config = yaml.load(self.hosts_file)
except OSError as e:
# I am assuming we are catching "cannot open file" exceptions
print(e)
sys.exit(1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this mandatory for adding add option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will the add work without it? Yes, in that case it is not mandatory.

However, not having it, means that a user can specify incorrect config path, and reading the file might fail. In that case, the exception is just ignored, and yaml_config is not read. Meaning, a user expected the script to use a config file, but it silently failed, and user doesn't even know something went wrong.

# See whether there are any commands to process
if changed_hosts and changed_hosts[0] in AVAILABLE_COMMANDS:
self.parse_command(changed_hosts[0], changed_hosts[1:])
sys.exit(0)
if changed_hosts[0] == "add":
loadPreviousConfig = True
changed_hosts = changed_hosts[1:]
else:
self.parse_command(changed_hosts[0], changed_hosts[1:])
sys.exit(0)

self.ensure_required_groups(ROLES)

if changed_hosts:
changed_hosts = self.range2ips(changed_hosts)
self.hosts = self.build_hostnames(changed_hosts)
self.hosts = self.build_hostnames(changed_hosts,
loadPreviousConfig)
self.purge_invalid_hosts(self.hosts.keys(), PROTECTED_NAMES)
self.set_all(self.hosts)
self.set_k8s_cluster()
Expand Down Expand Up @@ -158,24 +166,36 @@ def get_host_id(self, host):
except IndexError:
raise ValueError("Host name must end in an integer")

def build_hostnames(self, changed_hosts):
# Keeps already specified hosts,
# and adds or removes the hosts provided as an argument
def build_hostnames(self, changed_hosts, loadPreviousConfig=False):
existing_hosts = OrderedDict()
highest_host_id = 0
try:
for host in self.yaml_config['all']['hosts']:
existing_hosts[host] = self.yaml_config['all']['hosts'][host]
host_id = self.get_host_id(host)
if host_id > highest_host_id:
highest_host_id = host_id
except Exception:
pass
# Load already existing hosts from the YAML
if loadPreviousConfig:
try:
for host in self.yaml_config['all']['hosts']:
# Read configuration of an existing host
existing_hosts[host] = self.yaml_config['all']['hosts'][host]
Copy link
Contributor

Choose a reason for hiding this comment

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

pep8 detects invalid coding style like"

pep8 run-test: commands[0] | bash -c 'find /builds/kargo-ci/kubernetes-sigs-kubespray/contrib/inventory_builder/* -type f -name '"'"'*.py'"'"' -print0 | xargs -0 flake8'
/builds/kargo-ci/kubernetes-sigs-kubespray/contrib/inventory_builder/inventory.py:179:80: E501 line too long (81 > 79 characters)
                    existing_hosts[host] = self.yaml_config['all']['hosts'][host]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I wanted to know how to resolve it properly, as I couldn't find any good solution, ie. something that would look better than ignoring the warnings. I fixed it though.

# If the existing host seems
# to have been created automatically, detect its ID
if host.startswith(HOST_PREFIX):
host_id = self.get_host_id(host)
if host_id > highest_host_id:
highest_host_id = host_id
except Exception as e:
# I am assuming we are catching automatically
# created hosts without IDs
print(e)
sys.exit(1)

# FIXME(mattymo): Fix condition where delete then add reuses highest id
next_host_id = highest_host_id + 1
next_host = ""

all_hosts = existing_hosts.copy()
for host in changed_hosts:
# Delete the host from config the hostname/IP has a "-" prefix
if host[0] == "-":
realhost = host[1:]
if self.exists_hostname(all_hosts, realhost):
Expand All @@ -184,6 +204,8 @@ def build_hostnames(self, changed_hosts):
elif self.exists_ip(all_hosts, realhost):
self.debug("Marked {0} for deletion.".format(realhost))
self.delete_host_by_ip(all_hosts, realhost)
# Host/Argument starts with a digit,
# then we assume its an IP address
elif host[0].isdigit():
if ',' in host:
ip, access_ip = host.split(',')
Expand All @@ -203,11 +225,15 @@ def build_hostnames(self, changed_hosts):
next_host = subprocess.check_output(cmd, shell=True)
next_host = next_host.strip().decode('ascii')
else:
# Generates a hostname because we have only an IP address
next_host = "{0}{1}".format(HOST_PREFIX, next_host_id)
next_host_id += 1
# Uses automatically generated node name
# in case we dont provide it.
all_hosts[next_host] = {'ansible_host': access_ip,
'ip': ip,
'access_ip': access_ip}
# Host/Argument starts with a letter, then we assume its a hostname
elif host[0].isalpha():
if ',' in host:
try:
Expand All @@ -226,6 +252,7 @@ def build_hostnames(self, changed_hosts):
'access_ip': access_ip}
return all_hosts

# Expand IP ranges into individual addresses
def range2ips(self, hosts):
reworked_hosts = []

Expand Down Expand Up @@ -394,9 +421,11 @@ def show_help(self):
print_cfg - Write inventory file to stdout
print_ips - Write a space-delimited list of IPs from "all" group
print_hostnames - Write a space-delimited list of Hostnames from "all" group
add - Adds specified hosts into an already existing inventory

Advanced usage:
Add another host after initial creation: inventory.py 10.10.1.5
Create new or overwrite old inventory file: inventory.py 10.10.1.5
Add another host after initial creation: inventory.py add 10.10.1.6
Add range of hosts: inventory.py 10.10.1.3-10.10.1.5
Add hosts with different ip and access ip: inventory.py 10.0.0.1,192.168.10.1 10.0.0.2,192.168.10.2 10.0.0.3,192.168.10.3
Add hosts with a specific hostname, ip, and optional access ip: first,10.0.0.1,192.168.10.1 second,10.0.0.2 last,10.0.0.3
Expand Down
Loading