switch name- and ip-checking to opt-in #1589

saschalucas · 2021-04-12T15:32:03Z

As often discussed (at Ganeticons etc.) many people like to just create
instances without the need to resolv their name via DNS or /etc/hosts.
So a common boilerplate in the add command is to use --no-name-check
--no-ip-check. This commit inverts the logic to an opt-in model.

During changing the code, I disconverd an undocumented value of the
instance's NIC: ip=auto, which resolves the instance name and sets the
IP to the result. Until now, I was not aware of this.

This fixes #1384

rbott · 2021-04-20T20:32:36Z

Thanks for the PR @saschalucas

I have a few thoughts on this:
Changing the CLI might break scripts. We should probably - at least for a deprecation period - also keep the --no-[something] parameters and just change and document the default behavior. Maybe we can have the CLI tools issue a stderr warning if the deprecated parameters are used?

Another aspect: we should include this explicitly in the QA suite. From a quick glance I think it currently validates e.g. the name check but just because it does not disable it explicitly. I would opt (but possibly in a separate PR?) to test both variants in the QA suite.

What do you think?

saschalucas · 2021-04-23T22:10:11Z

Changing the CLI might break scripts. We should probably - at least for a deprecation period - also keep the --no-[something] parameters and just change and document the default behavior. Maybe we can have the CLI tools issue a stderr warning if the deprecated parameters are used?

It will also break scripts that rely on the old default name-/ip-check. For this case, there is no backwards compatibility. However, I've readded the old options with a deprecation warning. PTAL.

Another aspect: we should include this explicitly in the QA suite. From a quick glance I think it currently validates e.g. the name check but just because it does not disable it explicitly. I would opt (but possibly in a separate PR?) to test both variants in the QA suite.

Indeed. I'm still unfamiliar with the QA suite, but it looks like that a fix needs to be addressed in this PR:

ganeti/qa/qa_instance.py

Lines 452 to 456 in 592c0e9

    
           # first do a rename to a different actual name, expecting it to fail 
        
           qa_utils.AddToEtcHosts(["meeeeh-not-exists", rename_target]) 
        
           try: 
        
             AssertCommand(["gnt-instance", "rename", rename_source, rename_target], 
        
                           fail=True)

I'll need some time to get in touch with QA.

rbott · 2021-09-23T09:52:43Z

I just ran the QA suite against this PR and it breaks because the QA suite actually tests the name/ip-check behavior (somewhat implicitly).

The test TestInstanceRenameAndBack manipulates the /etc/hosts and expects the rename to fail:

Command: ssh disco-stu.staging.ganeti.org '{ cat /etc/hosts && echo -e '\''::1 meeeeh-not-exists kvm-test-instance01.staging.ganeti.org\n127.0.0.1 meeeeh-not-exists kvm-test-instance01.staging.ganeti.org'\''; } > /tmp/gnt.oRHyVN && mv /tmp/gnt.oRHyVN /etc/hosts'

Command: ssh disco-stu.staging.ganeti.org 'gnt-instance rename kvm-test-instance01.staging.ganeti.org kvm-test-instance01.staging.ganeti.org'

Instance 'kvm-test-instance01.staging.ganeti.org' renamed to 'kvm-test-instance01.staging.ganeti.org'
---- FAILED [TestInstanceRenameAndBack] gnt-instance rename: Command 'gnt-instance rename kvm-test-instance01.staging.ganeti.org kvm-test-instance01.staging.ganeti.org' on node disco-stu.staging.ganeti.org was expected to fail but didn't

With the checks disabled the command does not fail any more and this is unexpected to the QA suite. We could alter the tests to

accept the new default behavior and not expect the command to fail
add an extra step where we add the --name-check/--ip-check parameters and expect that step to fail

This way we would have both variants covered.

apoikos

@saschalucas thanks for this, we had been discussing it for quite a while.

If I recall correctly, the default behavior of checking IP and DNS had to do with how Ganeti was deployed initially at Google as part of a pipeline where an earlier step would do IPAM/DNS provisioning. It is true that most setups are simpler than that, and virtually every one I've seen uses --no-name-check and --no-ip-check for instance creation.

apoikos · 2024-05-13T06:33:00Z

lib/cli_opts.py

+                           action="store_true",
+                           help="Check that the instance's IP is alive (ping)")
+
+def WarnDeprecatedOption(option, opt_str, value, parser):


This should probably live in lib/cli.py.

I have tried to move WarnDeprecatedOption to lib/cli.py, but ended up with an circular import error, because lib/cli.py also imports cli_opts:

# make PYTHONPATH=. ./autotools/run-in-tempdir \ /tmp/022a060c4e2e293f642f2a46006698f849e40834/saschalucas_colon_name_ip_check-tmp/./autotools/build-bash-completion --compact > doc/examples/bash_completion Traceback (most recent call last): File "/tmp/022a060c4e2e293f642f2a46006698f849e40834/saschalucas_colon_name_ip_check-tmp/./autotools/build-bash-completion", line 50, in <module> from ganeti import cli File "/tmp/gntbuild.eKSZ4Lo2/ganeti/cli.py", line 58, in <module> import ganeti.cli_opts File "/tmp/gntbuild.eKSZ4Lo2/ganeti/cli_opts.py", line 831, in <module> action="callback", callback=cli.WarnDeprecatedOption, ^^^^^^^^^^^^^^^^^^^^^^^^ AttributeError: partially initialized module 'ganeti.cli' has no attribute 'WarnDeprecatedOption' (most likely due to a circular import) make: *** [Makefile:4502: doc/examples/bash_completion] Error 1 make: *** Deleting file 'doc/examples/bash_completion'

Any hints?

Probably in this case it's better to leave it here…

iustin

Wow, talk about a blast from the past. I've left a couple of minor wording changes, but looks good from my side.

man/gnt-instance.rst

test/py/cmdlib/instance_unittest.py

iustin · 2024-05-13T08:34:28Z

If I recall correctly, the default behavior of checking IP and DNS had to do with how Ganeti was deployed initially at Google as part of a pipeline where an earlier step would do IPAM/DNS provisioning. It is true that most setups are simpler than that, and virtually every one I've seen uses --no-name-check and --no-ip-check for instance creation.

The details were slightly different, but I agree that in normal setups, these being the default are not needed.

rbott · 2024-05-15T20:46:49Z

Nice to hear from you @iustin :-)

As often discussed (at Ganeticons etc.) many people like to just create instances without the need to resolv their name via DNS or /etc/hosts. So a common boilerplate in the add command is to use `--no-name-check` `--no-ip-check`. This commit inverts the logic to an opt-in model. During changing the code, I disconverd an undocumented value of the instance's NIC: `ip=auto`, which resolves the instance name and sets the IP to the result. Until now, I was not aware of this.

Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

The idea is, that before the name-check was implicitly enabled. Now it must be explicitly enabled via extra CLI option. Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

saschalucas · 2024-05-15T22:47:19Z

I just ran the QA suite against this PR and it breaks because the QA suite actually tests the name/ip-check behavior (somewhat implicitly).

I have blindly tried to address this issue by keeping as much as possible as before (see commit a230119). @rbott can you try to do a QA run?

Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

rbott · 2024-05-16T18:59:43Z

I tried running it against xen-pvm (which is the only option that currently works without any problems unrelated to this PR) and it went well 😄

I also checked against kvm, but that fails on bookworm due to hotplug issues. I will do one more run against buster, but I think we are safe to merge.

I think it would be better to either make a single commit out of this PR or to sqash-merge it into a single commit, what do you think @saschalucas?

saschalucas · 2024-05-17T13:53:00Z

I think it would be better to either make a single commit out of this PR or to sqash-merge it into a single commit, what do you think @saschalucas?

Thanks @rbott. Of cause I'll squash the commits... if I do not forget it again :-).

saschalucas requested review from apoikos, iustin and rbott April 20, 2021 18:40

saschalucas force-pushed the name_ip_check branch from f56bc96 to cdb4f4c Compare August 20, 2022 09:34

apoikos approved these changes May 13, 2024

View reviewed changes

iustin reviewed May 13, 2024

View reviewed changes

man/gnt-instance.rst Outdated Show resolved Hide resolved

man/gnt-instance.rst Outdated Show resolved Hide resolved

test/py/cmdlib/instance_unittest.py Outdated Show resolved Hide resolved

saschalucas force-pushed the name_ip_check branch from cdb4f4c to 408f58d Compare May 15, 2024 22:05

saschalucas added 4 commits May 16, 2024 00:14

fix unit tests

a1049fb

Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

keep --no-* and emit a deprecation warning

d2c02ba

Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

address minor wording changes

30e5984

Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

saschalucas force-pushed the name_ip_check branch from 408f58d to 30e5984 Compare May 15, 2024 22:16

a blind guess to fix QA and DNS checking

a230119

The idea is, that before the name-check was implicitly enabled. Now it must be explicitly enabled via extra CLI option. Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

fix lines longer than 80 characters

d42904a

Signed-off-by: Sascha Lucas <sascha_lucas@web.de>

rbott approved these changes May 16, 2024

View reviewed changes

document to use name-check, if ip-check is used

d8c3268

saschalucas merged commit 31e5194 into ganeti:master May 17, 2024
6 checks passed

saschalucas deleted the name_ip_check branch May 17, 2024 14:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch name- and ip-checking to opt-in #1589

switch name- and ip-checking to opt-in #1589

saschalucas commented Apr 12, 2021 •

edited

rbott commented Apr 20, 2021

saschalucas commented Apr 23, 2021

rbott commented Sep 23, 2021 •

edited

apoikos left a comment

apoikos May 13, 2024

saschalucas May 15, 2024

iustin May 15, 2024

iustin left a comment

iustin commented May 13, 2024

rbott commented May 15, 2024

saschalucas commented May 15, 2024

rbott commented May 16, 2024

saschalucas commented May 17, 2024

switch name- and ip-checking to opt-in #1589

switch name- and ip-checking to opt-in #1589

Conversation

saschalucas commented Apr 12, 2021 • edited

rbott commented Apr 20, 2021

saschalucas commented Apr 23, 2021

rbott commented Sep 23, 2021 • edited

apoikos left a comment

Choose a reason for hiding this comment

apoikos May 13, 2024

Choose a reason for hiding this comment

saschalucas May 15, 2024

Choose a reason for hiding this comment

iustin May 15, 2024

Choose a reason for hiding this comment

iustin left a comment

Choose a reason for hiding this comment

iustin commented May 13, 2024

rbott commented May 15, 2024

saschalucas commented May 15, 2024

rbott commented May 16, 2024

saschalucas commented May 17, 2024

saschalucas commented Apr 12, 2021 •

edited

rbott commented Sep 23, 2021 •

edited