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

grains/core: ignore HOST_NOT_FOUND errno in fqdns() #51706

Merged
merged 1 commit into from
Dec 9, 2019
Merged

grains/core: ignore HOST_NOT_FOUND errno in fqdns() #51706

merged 1 commit into from
Dec 9, 2019

Conversation

aplanas
Copy link
Contributor

@aplanas aplanas commented Feb 19, 2019 •
edited
Loading

What does this PR do?

We ignore also HOST_NOT_FOUND errno (actually h_errno) when trying to recover the fqdn.

Fixes #52788

@aplanas aplanas force-pushed the fix_fqdns branch 4 times, most recently from fee6f42 to 3c05bdc Compare March 4, 2019 10:45
@aplanas
Copy link
Contributor Author

aplanas commented Mar 5, 2019

Is ready for review. @isbm what do you think?

aplanas added a commit to aplanas/salt-1 that referenced this pull request Mar 6, 2019
@aplanas
Backport from develop features required by yomi
f57b7d8 
* Add root and no_recommends parameters in the public interface for Zypper and RPM (lowpkg)
  saltstack/salt#50125

* Add root parameter to useradd, shadow and groupadd
  saltstack/salt#50175

* cmd: Add root parameter for wait and run states
  saltstack/salt#50302

* systemd: add optional root parameter
  saltstack/salt#50380

* Add new chroot module
  https://github.com/openSUSE/salt/pull/50418

* Add new module freezer
  saltstack/salt#50452

* btrfs: add all subvolume commands
  saltstack/salt#50541

* btrfs: add new btrfs state
  saltstack/salt#50635

* zypper: demote log from error to warning
  saltstack/salt#50671

* blkid: add search by token
  saltstack/salt#50706

* mount: add fstab_{present,absent} states
  saltstack/salt#50725

* btrfs: add option to not set subvolumes as default
  saltstack/salt#50801

* Add disk_set and disk_toggle functions, and update valid partition flags
  saltstack/salt#50834

* disk: support setting FAT size for format_
  saltstack/salt#51074

* parted: fix set_ valid flags comment.
  saltstack/salt#51704

* grains/core: ignore HOST_NOT_FOUND errno in fqdns()
  saltstack/salt#51706

* cmdmod: add 'binds' parameter in run_chroot
  saltstack/salt#51871

* mount: fix extra -t parameter
  saltstack/salt#51905

* lvm: be quiet when a pv, lv or vg is not expected
  saltstack/salt#51929

* linux_lvm: clean error in pvcreate and pvremove
  saltstack/salt#51954

* blockdev: hide blkid errors when are expected
  saltstack/salt#51956

* partially unify public functions signature for pkg and lowpkg
  saltstack/salt#51973

* extmods: add utils directories in sys.path
  saltstack/salt#52001
@aplanas aplanas mentioned this pull request Mar 6, 2019
Closed
@aplanas aplanas force-pushed the fix_fqdns branch 3 times, most recently from e475011 to b9c88e7 Compare March 14, 2019 12:36
@aplanas aplanas force-pushed the fix_fqdns branch 2 times, most recently from 60c42f4 to 19669eb Compare May 2, 2019 09:08
brejoc
brejoc approved these changes May 14, 2019
View reviewed changes
Copy link
Contributor

@brejoc brejoc left a comment

Choose a reason for hiding this comment

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

lgtm

@aplanas aplanas force-pushed the fix_fqdns branch 2 times, most recently from ca362cb to 84f1d6d Compare May 27, 2019 08:12
@waynew
Copy link
Contributor

waynew commented May 31, 2019

@aplanas would it be reasonable to use the "warn once" method that we have elsewhere in the code, or does it make more sense to always ignore it?

@aplanas
Copy link
Contributor Author

aplanas commented Jun 1, 2019

Oh I like the idea. Makes sense for me. I will check how this was done before.

@aplanas
Copy link
Contributor Author

aplanas commented Jun 3, 2019

@waynew maybe I will need a bit of help here. The only place that I found where there is some kind of repetition filter for warnings is in __init__.py, but this is related with exceptions, and not with logs.

aplanas added a commit to aplanas/salt-1 that referenced this pull request Jun 5, 2019
@aplanas
Backport from develop features required by yomi
563440b 
* Add root and no_recommends parameters in the public interface for Zypper and RPM (lowpkg)
  saltstack/salt#50125

* Add root parameter to useradd, shadow and groupadd
  saltstack/salt#50175

* cmd: Add root parameter for wait and run states
  saltstack/salt#50302

* systemd: add optional root parameter
  saltstack/salt#50380

* Add new chroot module
  https://github.com/openSUSE/salt/pull/50418

* Add new module freezer
  saltstack/salt#50452

* btrfs: add all subvolume commands
  saltstack/salt#50541

* btrfs: add new btrfs state
  saltstack/salt#50635

* zypper: demote log from error to warning
  saltstack/salt#50671

* blkid: add search by token
  saltstack/salt#50706

* mount: add fstab_{present,absent} states
  saltstack/salt#50725

* btrfs: add option to not set subvolumes as default
  saltstack/salt#50801

* Add disk_set and disk_toggle functions, and update valid partition flags
  saltstack/salt#50834

* disk: support setting FAT size for format_
  saltstack/salt#51074

* parted: fix set_ valid flags comment.
  saltstack/salt#51704

* grains/core: ignore HOST_NOT_FOUND errno in fqdns()
  saltstack/salt#51706

* cmdmod: add 'binds' parameter in run_chroot
  saltstack/salt#51871

* mount: fix extra -t parameter
  saltstack/salt#51905

* lvm: be quiet when a pv, lv or vg is not expected
  saltstack/salt#51929

* linux_lvm: clean error in pvcreate and pvremove
  saltstack/salt#51954

* blockdev: hide blkid errors when are expected
  saltstack/salt#51956

* partially unify public functions signature for pkg and lowpkg
  saltstack/salt#51973

* extmods: add utils directories in sys.path
  saltstack/salt#52001
@aplanas
Copy link
Contributor Author

aplanas commented Jun 7, 2019

this is potentially a bug, can we tag it for Neon?

@aplanas aplanas mentioned this pull request Jun 11, 2019
Closed
@waynew
Copy link
Contributor

waynew commented Jun 14, 2019

@aplanas I'll bring it up with @saltstack/team-core, but my feelings are that it should get in with the Neon release.

@Ch3LL Ch3LL requested a review from waynew June 21, 2019 19:06
@aplanas aplanas requested a review from a team as a code owner August 23, 2019 07:56
@aplanas
Copy link
Contributor Author

aplanas commented Aug 26, 2019

The clint complain is about this:

salt/grains/core.py:51: [W1505(deprecated-method), linux_distribution] Using deprecated method _deprecated_linux_distribution()

As I do not touch this piece of code at all, can we review the patch? This is taking too long IMHO.

@waynew
Copy link
Contributor

waynew commented Aug 28, 2019

Hey @aplanas sorry for the delay. https://github.com/saltstack/salt/blob/216e3a7f9991ad3f1650841a744e7eb2c60a8a1f/salt/grains/core.py#L44 Looks like we should just tell lint to ignore that line, apparently we're already aware of it - especially if we want to put this change into neon as well. If we're just going into the Sodium release we should be able to just remove the entire other block of code.

Hm. Actually now that I look closer - maybe we should just rely on distro in general 🤔 Though I'm not actually seeing it in requirements anywhere 😕

@aplanas
Copy link
Contributor Author

aplanas commented Aug 29, 2019 •
edited
Loading

Looks like we should just tell lint to ignore that line, apparently we're already aware of it - especially if we want to put this change into neon as well. If we're just going into the Sodium release we should be able to just remove the entire other block of code.

I like your idea. If Sodium is Python3 there is no reason to maintain the old code that makes it compatible with Python2. Let me add an extra patch for this.

Edit: I just saw a disable=W1505 in the new master, I will rebase then! But I still like the idea.

@aplanas aplanas changed the base branch from develop to master October 11, 2019 15:56
@aplanas
Copy link
Contributor Author

aplanas commented Oct 11, 2019

Rebased into the new master (from develop), any chance now?

waynew
waynew suggested changes Oct 14, 2019
View reviewed changes
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Hey @aplanas this is looking great (and, apologies for the lengthy process on this one 😞 )

Would you be able to write a test for this? I'm hoping that it shouldn't be too much effort and we have something already testing around this area...

@aplanas
Copy link
Contributor Author

aplanas commented Oct 15, 2019

@waynew I added a test. Can you re-review?

waynew
waynew approved these changes Oct 17, 2019
View reviewed changes
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

🚀 Sweet action!

I know we currently have some issues around our Mac testing infrastructure that we're working on, but otherwise this should be ready to go. I did notice some Centos failures that I need to check out before we merge this.

@waynew
Copy link
Contributor

waynew commented Oct 17, 2019

Ah. It was a test from an unrelated thing (we do still need to fix that, so we'll probably need to do one more rebase 🤞 after we fix the mac/other issue elsewhere)

@aplanas
Copy link
Contributor Author

aplanas commented Nov 6, 2019 •
edited
Loading

I am loosing hope that this, or any of my others PR will be merged ever: there is no way to have a green CI result after almost one year of rebasing (or moving the code between branches) (Edit: sorry, I was a bit frustrated)

We have 49(!) checks in place, but they are still randomly failing, making very hard to differentiate between an useful failing report, from noise.

Can we have a more strict policy in place, where we trade off a super big number of checks with reliable ones? For example, if ci/py2/macosxmojave is broken, lets disable it with urgency before re-adding it once that is fixed.

@aplanas
Copy link
Contributor Author

aplanas commented Nov 6, 2019

ci/py3/debian10 fail seems unrelated:

11:51:30         ======================================================================
11:51:30         FAIL [29.945s]: test_pkg_latest_version (integration.modules.test_pkg.PkgModuleTest)
11:51:30         ----------------------------------------------------------------------
11:51:30         Traceback (most recent call last):
11:51:30           File "/tmp/kitchen/testing/tests/support/helpers.py", line 121, in wrap
11:51:30             return caller(cls)
11:51:30           File "/tmp/kitchen/testing/tests/support/helpers.py", line 1154, in wrapper
11:51:30             return caller(cls)
11:51:30           File "/tmp/kitchen/testing/tests/support/helpers.py", line 1077, in decorator
11:51:30             return func(*args, **kwargs)
11:51:30           File "/tmp/kitchen/testing/tests/integration/modules/test_pkg.py", line 362, in test_pkg_latest_version
11:51:30             self.assertIn(pkg_latest, cmd_pkg)
11:51:30         AssertionError: "ERROR: E: Repository 'https://repo.saltstack.com/apt/debian/9/amd64/latest stretch InRelease' changed its 'Suite' value from 'stable' to 'oldstable'" not found in '\nWARNING: apt does not have a stable CLI interface. Use with caution in scripts.\n\nListing...\nfiglet/stable,now 2.2.5-3 amd64 [residual-config]'

@aplanas aplanas force-pushed the fix_fqdns branch 2 times, most recently from 9d1797a to 0dd3798 Compare November 7, 2019 08:54
@aplanas
Copy link
Contributor Author

aplanas commented Nov 7, 2019

This is as green as I can get. The current fails, IIUC, are timeouts. I am not able to diagnose the reason of the VM timeout, tho.

@aplanas aplanas force-pushed the fix_fqdns branch 3 times, most recently from 624e834 to 6d25b32 Compare November 18, 2019 14:55
@aplanas
grains/core: ignore HOST_NOT_FOUND errno in fqdns()
d4d0901 
Also ignore NO_DATA errors, as this imply an empty field

Fixes saltstack#52788
@dwoz dwoz merged commit 5a20e90 into saltstack:master Dec 9, 2019
@aplanas aplanas deleted the fix_fqdns branch December 10, 2019 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brejoc brejoc brejoc approved these changes

@waynew waynew waynew approved these changes

Assignees
No one assigned
Labels
Reviewers-Assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

FQDNs grains generate errors in IPv6
5 participants
@aplanas @waynew @brejoc @dwoz @Ch3LL