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.

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

WIP: #54281 speedup pkg5 module #54284

Open
wants to merge 9 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@danowar2k
Copy link

danowar2k commented Mar 23, 2019

SUMMARY

This resolves #54281.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

pkg5

ADDITIONAL INFORMATION

Added a function: Given a list of packages, it returns a dictionary of packages that are installed, packages that are not installed and the subset of the installed packages that could be updated.
Parts of the dictionary are then fed to the ensure function. I removed unnecessary helper functions.

This greatly improves speed of package checks, as not every package is checked alone but all are checked at once.

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 23, 2019

Wait, this isn't right yet....

@danowar2k danowar2k closed this Mar 23, 2019

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 23, 2019

Now this should work

@danowar2k danowar2k reopened this Mar 23, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 23, 2019

@danowar2k this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

danowar2k added some commits Mar 23, 2019

Resolves #54281
- Speeds up checking if a number of packages are installed
Resolves #54281
- now a single method detects which packages are installed, not installed and (when you need it) not up to "latest"; This uses the pkg list stderr output when listing packages that are not installed or do not exist
- an additional pkg list -u command then only checks the installed packages whether they are updatable
- When "latest" is requested, updatable packages and not installed packages are combined
- This removes the need for the lambda filter method

@danowar2k danowar2k force-pushed the danowar2k:#54281-Speedup_pkg5 branch to 30154ee Mar 23, 2019

@ansibot

This comment has been minimized.

@danowar2k danowar2k changed the title #54281 speedup pkg5 #54281 speedup pkg5 module Mar 23, 2019

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 24, 2019

Wait with reviewing, this seems to still have problems with FMRIs containing wildcards ("*"). I'll have to look into the FMRI syntax...

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 24, 2019

Additional question: Is there any testing done using Solaris 11 systems? Could I be of service setting that up, too? I don't yet understand your testing process, though...

EDIT: However you test OS specific modules, I could provide you with a Packer template for a Solaris 11.4 Vagrant box, if that helps. A Solaris 11.4 installer ISO can be downloaded if you have a free Oracle account, and the ISO can be used for development purposes free of charge via the Oracle Technology Network (OTN) license. So using a Solaris Vagrant box for testing purposes should be good to go.

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 24, 2019

Also, it seems the module had issues with installing FMRIs with wildcards before I started...

'present': {
'filter': lambda p: not is_installed(module, p),
'subcommand': 'install',
},

def is_installed(module, package):
rc, out, err = module.run_command(['pkg', 'list', '--', package])
return not bool(int(rc))

# If no pkg FMRIs containing "package_substring" are installed, this will return rc 1
# If any pkg FMRI containing "package_substring" is installed, this will return rc 0
# even if another pkg FMRI containing "package_substring" is not installed yet
pkg list -- *package_substring*

So installing packages using wildcards basically doesn't work yet. Let's see if I can fix that, too...That will get complicated though.

I'll try with pkg list -a and parsing both stdout and stderr and checking the requested FMRIs for wildcards.

@mator
Copy link
Contributor

mator left a comment

:deleted

@ansibot ansibot added needs_revision and removed needs_triage labels Mar 28, 2019

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

Sorry, I'm still changing code but forgot to close the PR in the meantime...
Once I'm done, I'm gonna reopen this one, document Solaris pkg behaviour, the old behaviour of the pkg5 module and the new behaviour of the pkg5 module here.

@danowar2k danowar2k closed this Mar 29, 2019

danowar2k added some commits Mar 29, 2019

- The filtering of FMRI patterns plus the extraction of concrete pack…
…ages to install or change now happens in two functions

- State "latest": all requested FMRI patterns are passed to pkg install; pkg takes care of wildcards and version requests, errors
- State "absent": You have to filter the patterns of packages that don't exist or aren't installed because missing packages won't be ignored but an error will be thrown. So I filter the FMRI patterns to ones that match installed packages and then uninstall them with pkg uninstall
- State "present": The hardest part...
I filter the FMRI patterns so that the list of patterns to install contains all patterns with "@latest" and all patterns that do not match any possible package (to not hide any user error).
Then I fetch a list of all matching package FMRIs that are or aren't installed using the remaining patterns.
If no matching package is installed, install the latest one or the most specific of the requested versions
If any package version is installed,
- do nothing if the pattern is just a package name
- if a version is requested and the installed version doesn't match, add package+version to the patterns to install
Always fail if different versions are requested

Wildcards: Previously, if a FMRI pattern contained wildcards, and there were multiple matching packages and only one of them was installed, the module didn't install the other one but filtered it away ("pkg list -- *wildcarded_package*" -> yes, there is one package that matches, don't need to do anything).

@danowar2k danowar2k reopened this Mar 29, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 29, 2019

The test ansible-test sanity --test pylint [explain] failed with 7 errors:

lib/ansible/modules/packaging/os/pkg5.py:180:0: anomalous-backslash-in-string Anomalous backslash in string: '\$'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:180:0: anomalous-backslash-in-string Anomalous backslash in string: '\-'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:180:0: anomalous-backslash-in-string Anomalous backslash in string: '\-'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:198:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:203:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:266:0: anomalous-backslash-in-string Anomalous backslash in string: '\*'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:271:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with the error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc -t module pkg5" returned exit status 0.
>>> Standard Error
<unknown>:180: SyntaxWarning: invalid escape sequence \-
<unknown>:198: SyntaxWarning: invalid escape sequence \s
<unknown>:203: SyntaxWarning: invalid escape sequence \s
<unknown>:266: SyntaxWarning: invalid escape sequence \*
<unknown>:271: SyntaxWarning: invalid escape sequence \s

The test ansible-test sanity --test import --python 3.8 [explain] failed with 5 errors:

lib/ansible/modules/packaging/os/pkg5.py:180:0: SyntaxWarning: invalid escape sequence \-
lib/ansible/modules/packaging/os/pkg5.py:198:0: SyntaxWarning: invalid escape sequence \s
lib/ansible/modules/packaging/os/pkg5.py:203:0: SyntaxWarning: invalid escape sequence \s
lib/ansible/modules/packaging/os/pkg5.py:266:0: SyntaxWarning: invalid escape sequence \*
lib/ansible/modules/packaging/os/pkg5.py:271:0: SyntaxWarning: invalid escape sequence \s

The test ansible-test sanity --test pep8 [explain] failed with 12 errors:

lib/ansible/modules/packaging/os/pkg5.py:180:135: W605 invalid escape sequence '\-'
lib/ansible/modules/packaging/os/pkg5.py:180:137: W605 invalid escape sequence '\-'
lib/ansible/modules/packaging/os/pkg5.py:180:139: W605 invalid escape sequence '\$'
lib/ansible/modules/packaging/os/pkg5.py:180:161: E501 line too long (167 > 160 characters)
lib/ansible/modules/packaging/os/pkg5.py:198:44: W605 invalid escape sequence '\s'
lib/ansible/modules/packaging/os/pkg5.py:203:53: W605 invalid escape sequence '\s'
lib/ansible/modules/packaging/os/pkg5.py:220:161: E501 line too long (169 > 160 characters)
lib/ansible/modules/packaging/os/pkg5.py:234:161: E501 line too long (169 > 160 characters)
lib/ansible/modules/packaging/os/pkg5.py:252:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/packaging/os/pkg5.py:266:32: W605 invalid escape sequence '\*'
lib/ansible/modules/packaging/os/pkg5.py:271:38: W605 invalid escape sequence '\s'
lib/ansible/modules/packaging/os/pkg5.py:285:12: E713 test for membership should be 'not in'

click here for bot help

@dagwieers

This comment has been minimized.

Copy link
Member

dagwieers commented Mar 29, 2019

@danowar2k Before we can accept any changes to existing modules, we need to have integration tests so it gives some confidence this is not breaking existing functionality. Especially since not a lot of people in the community are involved in the Ansible community.

See: https://github.com/ansible/community/wiki/Solaris

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

Okay, the commit message contains a description of the changes.

Now to document pkg behaviour:
pkg install FMRI_PATTERN

Variant A:

  • pattern contains no version (no "@" part)
  • pattern contains @latest
    -> requested_package_version=latest

Variant B:

  • pattern contains "@specific_version_part"
    -> requested_package_version=specific_version_part

  • FMRI_PATTERN without wildcards

    • no matching package
      • stderr contains " FMRI_PATTERN\n"
    • one matching package, not installed
      • Variant A: installs requested_package_version version of package if allowed
      • Variant B: installs requested_package_version if allowed, fail otherwise (can't update to version)
    • one matching package, installed but not the requested_package_version
      • Variant A: updates to the requested_package_version
      • Variant B: updates/downgrades to the requested_package_version if allowed, fail otherwise
    • one matching package, installed in the requested_package_version
      • "Nothing to do"
    • multiple matching packages
      • fails with error: ambiguous because multiple packages match
  • FMRI_PATTERN with wildcards

    • no matching package

      • stderr contains " FMRI_PATTERN\n"
    • one matching package, not installed

      • installs requested_package_version of package if allowed
    • one matching package, installed but not requested_package_version

      • updates/downgrades to the requested_package_version if possible
    • one matching package, installed in requested_package_version

      • "Nothing to do"
    • multiple matching packages

      • for each matched package, proceed like "one matching package"
  • multiple FMRI_PATTERNS matching the same package but different requested_package_version

    • fails with error: can't satisfy conditions
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 29, 2019

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/modules/packaging/os/pkg5.py:204:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:209:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:283:0: anomalous-backslash-in-string Anomalous backslash in string: '\*'. String constant might be missing an r prefix.
lib/ansible/modules/packaging/os/pkg5.py:288:0: anomalous-backslash-in-string Anomalous backslash in string: '\s'. String constant might be missing an r prefix.

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with the error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc -t module pkg5" returned exit status 0.
>>> Standard Error
<unknown>:204: SyntaxWarning: invalid escape sequence \s
<unknown>:209: SyntaxWarning: invalid escape sequence \s
<unknown>:283: SyntaxWarning: invalid escape sequence \*
<unknown>:288: SyntaxWarning: invalid escape sequence \s

The test ansible-test sanity --test import --python 3.8 [explain] failed with 4 errors:

lib/ansible/modules/packaging/os/pkg5.py:204:0: SyntaxWarning: invalid escape sequence \s
lib/ansible/modules/packaging/os/pkg5.py:209:0: SyntaxWarning: invalid escape sequence \s
lib/ansible/modules/packaging/os/pkg5.py:283:0: SyntaxWarning: invalid escape sequence \*
lib/ansible/modules/packaging/os/pkg5.py:288:0: SyntaxWarning: invalid escape sequence \s

The test ansible-test sanity --test pep8 [explain] failed with 4 errors:

lib/ansible/modules/packaging/os/pkg5.py:204:44: W605 invalid escape sequence '\s'
lib/ansible/modules/packaging/os/pkg5.py:209:53: W605 invalid escape sequence '\s'
lib/ansible/modules/packaging/os/pkg5.py:283:32: W605 invalid escape sequence '\*'
lib/ansible/modules/packaging/os/pkg5.py:288:38: W605 invalid escape sequence '\s'

click here for bot help

@dagwieers dagwieers changed the title #54281 speedup pkg5 module WIP: #54281 speedup pkg5 module Mar 29, 2019

@ansibot ansibot added the WIP label Mar 29, 2019

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

Okay, now to document the way the ansible pkg5 module used to work. I'll only document where the expected behaviour differed from the actual behaviour.

  • FMRI_PATTERN without wildcards

    • one matching package, installed but not the requested_package_version
      • if @latest is specifically requested (even when state is set to "present", which is also the default state, this is entirely possible to happen), nothing is done because is_installed returns true
    • multiple matching packages, none is installed
      • fails like usual
    • multiple matching packages, at least one is installed
      • is_installed returns true, no error is reported, user doesn't know multiple packages match
  • FMRI_PATTERN with wildcards

    • one matching package, installed but not requested_package_version
      • if @latest is specifically requested (even when state is set to "present", which is also the default state, this is entirely possible to happen), nothing is done because is_installed returns true
    • multiple matching packages, none is installed
      • installs all matching packages in the requested version if possible, fails otherwise (installs all or nothing)
    • multiple matching packages, at least one is installed
      • is_installed returns true, no error is reported, user doesn't know multiple packages match and the not installed package is never installed

To summarize, the pkg5 seems to have had a few issues with patterns containing wildcards and @latest behaviour when requesting state = "present". I hope my pull request addresses most, if not all of that.

Other stuff:

  • the function is_latest returns true if updates are available and returns false when updates are not available. The naming seems to be the wrong way round.

Next: pkg5 module behaviour of my pull request

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

A problem seems to present itself: My pull request was for speeding up the pkg5 module, but some other bugs seem to have come up. What do I do when that happens and I think my code does both (fixes bugs and speeds up pkg5)?

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

Example for bugs:
Use pkg5 module in Ansible playbook on a Solaris VM:

# This won't update the system because entire is always installed in some version
- hosts: all
  name: Run the pkg5 module for tests
  tasks:
    - name: Explicitly install the latest entire package (Do a Solaris upgrade)
      become: true
      pkg5:
        name:
          - entire@latest
        state: present

# This won't update the system because entire is always installed in some version
    - name: Install the latest entire package without mentioning the state
      become: true
      pkg5:
        name:
          - entire@latest

# This won't update the system because entire is always installed in some version
    - name: Install the latest entire package with wildcards
      pkg5:
        name:
          - "*entire*@latest"
        state: present

# Precondition:
# $ pkg list -a *dns/bind*
# NAME (PUBLISHER)                                  VERSION                    IFO
# network/dns/bind                                  9.10.8.1.0-11.4.3.0.1.1.0  i--
# service/network/dns/bind                          9.10.8.1.0-11.4.3.0.1.1.0  ---
# This won't fail with the below message, but instead will say "Nothing to do"
    - name: Install the one and only dns/bind package (but multiple packages match)
      pkg5:
        name:
          - "dns/bind"
        state: present

# This won't install anything because at least one matching package is installed
    - name: Install all packages matching dns/bind (won't install anything)
      pkg5:
        name:
          - "*dns/bind*"
        state: present

Example for ambiguous package name error:

# pkg install -n dns/bind

pkg install: 'dns/bind' matches multiple packages
        pkg://solaris/service/network/dns/bind
        pkg://solaris/network/dns/bind

Please provide one of the package FMRIs listed above to the install command.
@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

How my pkg5 module works different:

  • State "latest" just tries to install/update all matching packages to latest
    • Discussion: Should the combination of state:"latest" + FMRI patterns with specific requested versions lead to an error message ("Why would you do that? Conflicting intentions?")
  • State "present" installs all matching not installed packages to the requested versions, updates all packages matching versioned patterns to the requested versions if possible and leaves other installed packages alone
@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

Okay, I'm done for now. Please tell me if and where I can assist.

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

@danowar2k Before we can accept any changes to existing modules, we need to have integration tests so it gives some confidence this is not breaking existing functionality. Especially since not a lot of people in the community are involved in the Ansible community.

See: https://github.com/ansible/community/wiki/Solaris

I've read up on testing Ansible in the docs. The integration testing page seems a bit more sparse than the other ones.

I'm trying to get a feeling for this. If I'm really trying to show that the module works as intended I think I need a generated Solaris VM to test on. I don't yet understand what I have to do to integrate a Solaris VM in your testing suite. Shippable looks like it generates a lot of VMs each with a different OS version. I wonder if and how I could integrate a Solaris VM into this.

Meanwhile, I surely can restructure more of my code into OS-independent functions, leaving only the functions containing the run_command calls untested. I can mock output generated by pkg list etc. and then feed the functions this output. Would that be good for a start?

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 29, 2019

If I understand this right, you have a Shippable CI configuration which runs unit tests, sanity tests, etc. . It also declares tests on certain systems which seem to be using Docker containers. Solaris and Docker doesn't seem to work together, so no Docker image for Solaris exists. So I can't add that.

I'm developing playbooks for Solaris x86 machines using a pipeline of
Packer generated VirtualBox ISO + Vagrant box -> running the Vagrant box in VirtualBox -> applying Ansible playbooks to the VM / developing applications in the VM

If there is a CI provider that can run VirtualBox images and Vagrant, one could generate a second configuration for that CI provider to run Solaris specific tests on the started Solaris VirtualBox VM.

What do you think?

@danowar2k

This comment has been minimized.

Copy link
Author

danowar2k commented Mar 30, 2019

Gitlab can use VirtualBox:
https://docs.gitlab.com/runner/executors/virtualbox.html

Gitlab can be activated with Github:
https://docs.gitlab.com/ee/ci/ci_cd_for_external_repos/github_integration.html

Maybe a way to facilitate testing with Solaris VMs?

@mavit

This comment has been minimized.

Copy link
Contributor

mavit commented Apr 2, 2019

Have a look at #51651, which introduces tests for the pkgutil module. In this example a pre-existing Solaris host is required to test with. This is no good for automated testing (since we don't have any Solaris VMs dedicated to CI (yet?)), but does mean that a human who has access to Solaris machines can run the tests manually before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.