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

VladMasarik · 2021-05-02T17:44:51Z

What type of PR is this?
/kind bug

What this PR does / why we need it:

The documentation and help suggested that the dynamic inventory script was able to add new hosts if specified. However, this did not work, and instead the original config file was overwritten by the newly specified hosts. This PR fixes it by changing load_all() function call to just load().
After fixing this issue there were problems with naming scheme of the nodes. This was previously unreachable code. The original script looked at the hostname of a node, and tried to look for a numeric ID. This however failed when the hostnames were manually created such as "master" / "main" / "worker". An exception was thrown, but ignored/swallowed. I was not able to find an important usage for the IDs, therefore I have adjusted it so that the IDs are checked only when a hostname of a node starts with the HOST_PREFIX string(ie. we can assume that the hostname was automatically created). Additionally, I have at least added a print out of the exceptions, and the script exits with 1 when they are raised.
I have adjusted the documentation in the script's help command, and also added a few comments to help with understanding. It is up for a discussion whether they are helpful, or whether we should remove them. As for the other documentation, I searched for the word "inventory", but couldn't find any text that would need a change. I managed to find only one instance in Getting started.
I have added a new add command for the script. The previously expected behavior was that if you had an inventory config with 4 nodes, the:

$ inventory.py 10.0.0.3

would add a new node with the 10.0.0.3 IP address, thus you would have 5 nodes, or 4 if it was a duplicate IP address. However, this would actually overwrite the original inventory configs, so you would end up with just 1 node. I am not sure if people use this script, but I assume that not changing the original behavior (whether intended or not) would be better, as this issue was introduced a year ago #5373 . Although, they seem to be doing the exact opposite of what this PR does. They never show the structure of their config file, and are using .ini files, which I am assuming are different from YAML, so what they were doing, beats me. So in order to keep the old functionality, but also allow the already existing one that did not work, this PR adds an add command. Example, you have 4 nodes in your inventory and execute:

$ inventory.py add gpuvm1,10.0.0.3 gpuvm2,10.0.0.5

This adds 5th and 6th node called "gpuvm1" and "gpuvm2" into the existing inventory YAML file.

I have added new tests, and also fixed the old ones, as they had a few problems, eg. they succeed although they were not supposed to. I surely did not cover everything, but I felt this PR is an overkill already, so this is up for a discussion as well. How do you suggest I should deal with the linting errors? I couldn't think of any nice solution.

Which issue(s) this PR fixes:

Fixes #7577

Special notes for your reviewer:
Lots of text, sorry for that, but I wanted to mention things I have considered so that you can point out something if I have missed a thing.

Does this PR introduce a user-facing change?:

The dynamic inventory builder will by default overwrite the inventory config. This was previously unintended behavior. In order to add new hosts into the already existing inventory config use the "add" command e.g. "$ inventory.py add 10.0.1.8".

k8s-ci-robot · 2021-05-02T17:44:58Z

Hi @VladMasarik. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

floryut · 2021-05-03T12:45:30Z

Please check the fmt issue in the CI job

EppO · 2021-05-04T02:25:30Z

/ok-to-test

oomichi · 2021-05-04T22:08:38Z

contrib/inventory_builder/inventory.py

+            try:
+                for host in self.yaml_config['all']['hosts']:
+                    # Read configuration of an existing host
+                    existing_hosts[host] = self.yaml_config['all']['hosts'][host]


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]

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.

oomichi · 2021-05-04T22:09:48Z

contrib/inventory_builder/inventory.py

+            except OSError as e:
+                # I am assuming we are catching "cannot open file" exceptions
+                print(e)
+                sys.exit(1)


Is this mandatory for adding add option?

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.

to after checking whether the config should be loaded, and added check for whether the config should be loaded

if the user wants to remove a node, we need to load the config

VladMasarik · 2021-05-05T12:06:06Z

While using the script, I noticed and fixed a few mistakes I made.

python inventory.py -10.0.1.1 did not work as intended, and rather deleted the config. Basically the same mistake that python inventory.py 10.0.1.1 had, and why I created this PR.
The config file was loaded regardless of the loadPreviousConfig, and some functions actually used it even though they were not meant to.

VladMasarik · 2021-06-07T14:02:45Z

ping pong @floryut @oomichi

VladMasarik · 2021-08-18T11:19:51Z

ping @floryut @oomichi

floryut · 2021-08-19T06:54:36Z

ping @floryut @oomichi

wow sorry, don't know how this PR was missed and forgotten, fine by me on the fix; I indeed saw that this wasn't adding anything and just reloading all the file, but hadn't time to take a look.

Anyway, thanks man.

/approve
/ok-to-test
/cc @oomichi @EppO

k8s-ci-robot · 2021-08-19T06:54:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, VladMasarik

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~~ [floryut]

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

oomichi

Sorry for my late response.

/lgtm

floryut · 2021-09-15T08:01:34Z

@VladMasarik Added this to the release note for 2.17, again sorry this took time to merge, great work man.

VladMasarik · 2021-10-04T12:03:39Z

@oomichi @floryut AFAIK maintaining OSS projects isnt easy, and it takes a few months to merge the requests. I am very happy to help, and have no problems so as long as I get clear deny / approve / not priority. So happy to help, and contribute more later!

floryut · 2021-10-04T14:32:26Z

@oomichi @floryut AFAIK maintaining OSS projects isnt easy, and it takes a few months to merge the requests. I am very happy to help, and have no problems so as long as I get clear deny / approve / not priority. So happy to help, and contribute more later!

Indeed, not whining at all but the hard part is managing time between work and family 😄
Thanks again for your PR, that's what allow us to keep the project alive

…bernetes-sigs#7583) * Fix: adding new ips with inventory builder (kubernetes-sigs#7577) * moved conflig loading logic to after checking whether the config should be loaded, and added check for whether the config should be loaded * added check for removing nodes from config if the user wants to remove a node, we need to load the config * Fix tox errors

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 2, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 2, 2021

k8s-ci-robot requested review from EppO and holmsten May 2, 2021 17:44

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 2, 2021

VladMasarik force-pushed the fix-config-resets branch from af3f9c8 to 88db844 Compare May 2, 2021 17:57

Fix: adding new ips with inventory builder (kubernetes-sigs#7577)

78185ef

VladMasarik force-pushed the fix-config-resets branch from 88db844 to 78185ef Compare May 2, 2021 18:06

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 4, 2021

oomichi reviewed May 4, 2021

View reviewed changes

VladMasarik added 3 commits May 5, 2021 13:28

moved conflig loading logic

bf2286a

to after checking whether the config should be loaded, and added check for whether the config should be loaded

added check for removing nodes from config

e7860f5

if the user wants to remove a node, we need to load the config

Fix tox errors

72b8e82

k8s-ci-robot requested a review from oomichi August 19, 2021 06:54

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2021

oomichi reviewed Sep 10, 2021

View reviewed changes

k8s-ci-robot assigned oomichi Sep 10, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2021

k8s-ci-robot merged commit a5a88e4 into kubernetes-sigs:master Sep 10, 2021

floryut mentioned this pull request Sep 15, 2021

Release Proposal v2.17 #7951

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

VladMasarik commented May 2, 2021 •

edited

Loading

k8s-ci-robot commented May 2, 2021

floryut commented May 3, 2021

EppO commented May 4, 2021

oomichi May 4, 2021

VladMasarik May 5, 2021

oomichi May 4, 2021

VladMasarik May 5, 2021

VladMasarik commented May 5, 2021

VladMasarik commented Jun 7, 2021

VladMasarik commented Aug 18, 2021

floryut commented Aug 19, 2021

k8s-ci-robot commented Aug 19, 2021

oomichi left a comment

floryut commented Sep 15, 2021

VladMasarik commented Oct 4, 2021

floryut commented Oct 4, 2021 •

edited

Loading

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

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

Conversation

VladMasarik commented May 2, 2021 • edited Loading

k8s-ci-robot commented May 2, 2021

floryut commented May 3, 2021

EppO commented May 4, 2021

oomichi May 4, 2021

Choose a reason for hiding this comment

VladMasarik May 5, 2021

Choose a reason for hiding this comment

oomichi May 4, 2021

Choose a reason for hiding this comment

VladMasarik May 5, 2021

Choose a reason for hiding this comment

VladMasarik commented May 5, 2021

VladMasarik commented Jun 7, 2021

VladMasarik commented Aug 18, 2021

floryut commented Aug 19, 2021

k8s-ci-robot commented Aug 19, 2021

oomichi left a comment

Choose a reason for hiding this comment

floryut commented Sep 15, 2021

VladMasarik commented Oct 4, 2021

floryut commented Oct 4, 2021 • edited Loading

VladMasarik commented May 2, 2021 •

edited

Loading

floryut commented Oct 4, 2021 •

edited

Loading