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

State pkg.latest called win pkg.install with list of pkgs and the required versions #47702

Merged
merged 5 commits into from May 19, 2018

Conversation

Projects
None yet
3 participants
@damon-atkins
Member

damon-atkins commented May 17, 2018

What does this PR do?

If the software is already installed windows pkg.install does not upgrade the software, as design.
However the state pkg.latest expected this behaviour.

The state pkg.latest determines already the software which needs to be updated and the version required. However does not use the version information.

This fix applied only to windows passes on this information (pkg + version) to pkg.installed.

What issues does this PR fix or reference?

Windows: pkg.latest state not updating packages. (#47484)

Previous Behavior

The state pkg.latest did not work unless the "Package Definition" used latest as the version number.

New Behavior

Works with normal version number and "latest"

Given the way Windows pkg system works. If pkg.latest is called and the current version of the software is not "Package Definition" the software may be downgraded to the latest version defined in the "Package Definition"

Tests written?

No New Tests

Commits signed with GPG?

Yes

pkg.install execution module on windows ensures the software package is
installed when no version is specified, it does not upgrade the software
to the latest. This is per the design. pkg.latest must provide the versions
to install to pkg.install

@salt-jenkins salt-jenkins requested a review from saltstack/team-suse May 17, 2018

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented May 17, 2018

Results of testing, showing the software is now upgraded to latest (left side)

#### pkg.install octopus-tentacle version=3.3.16 [pkg.install_octopus-tentacle_version=3.3.16_pkg_state_latest]
#### state.high {octopus-tentacle:{pkg:[latest]}} [state.high_{octopus-tentacle:{pkg:[latest]}}_pkg_state_latest]
[INFO    ] state.call Completed state [octopus-tentacle]     [INFO    ] state.call Completed state [octopus-tentacle]
at time 02:09:33.091000 duration_in_ms=37335.0             | at time 01:52:17.968000 duration_in_ms=34777.0
{                                                            {
    "local": {                                                   "local": {

"pkg_|-octopus-tentacle_|-octopus-tentacle_|-latest": {      "pkg_|-octopus-tentacle_|-octopus-tentacle_|-latest": {
            "comment": "The following packages were        |             "comment": "Package octopus-tentacle failed
successfully installed/upgraded: octopus-tentacle",        | to update.",
            "name": "octopus-tentacle",                                  "name": "octopus-tentacle",
            "start_time": "02:08:55.756000",               |             "start_time": "01:51:43.191000",
            "result": true,                                |             "result": false,
            "duration": 37335.0,                           |             "duration": 34777.0,
            "__run_num__": 0,                                            "__run_num__": 0,
            "__sls__": null,                                             "__sls__": null,
            "changes": {                                   |             "changes": {},
                "octopus-tentacle": {                      <
                    "new": "3.20.1",                       <
                    "old": "3.3.16"                        <
                }                                          <
            },                                             <
            "__id__": "octopus-tentacle"                                 "__id__": "octopus-tentacle"
        }                                                            }
    }                                                            }
}                                                            }

@damon-atkins damon-atkins changed the title from [WIP] State pkg.latest called win pkg.install with list of pkgs and the required versions to State pkg.latest called win pkg.install with list of pkgs and the required versions May 17, 2018

@rallytime rallytime requested a review from terminalmage May 17, 2018

fromrepo=fromrepo,
skip_verify=skip_verify,
pkgs=targeted_pkgs,
**kwargs)

This comment has been minimized.

@terminalmage

terminalmage May 17, 2018

Member

We should only need one call to pkg.install here. The only difference I see is that we are passing name=None on the Windows side, and name on non-Windows. But the pkg virtuals already ignore name when pkgs is not None, so we should safely be able to pass name=None on non-Windows platforms.

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented May 17, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 17, 2018

@damon-atkins This is causing 2 package tests to fail. Can you look into that please?

https://jenkins.saltstack.com/job/PR/job/salt-pr-linode-ubuntu16-py3/9968/

@damon-atkins damon-atkins force-pushed the damon-atkins:2017.7.6_fix_pkg.latest_state branch 2 times, most recently from 26554fb to dd1ce63 May 18, 2018

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented May 18, 2018

re-run ubuntu

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented May 18, 2018

@terminalmage Please have another look.

if not name:
# Note state checks name is a string, this should only happen if their is a bug
raise CommandExecutionError('Parameter name must contain label or package name.')

This comment has been minimized.

@terminalmage

terminalmage May 18, 2018

Member

I don't understand why you added this. It doesn't seem like it is necessary, for one thing, and it would result in an ugly traceback in the state results if it were ever triggered.

if kwargs.get('sources'):
return {'name': name,
'changes': {},
'result': False,
'comment': 'The "sources" parameter is not supported.'}
elif pkgs:
elif pkgs is not None:

This comment has been minimized.

@terminalmage

terminalmage May 18, 2018

Member

Again, why would this be necessary? We shouldn't be invoking this state with an empty list as input, so why do we need to specifically check if it is not None?

}
else:
desired_pkgs = [name]
desired_pkgs = [name]

This comment has been minimized.

@terminalmage

terminalmage May 18, 2018

Member

Again, this doesn't make sense. Invoking the state with empty pkgs is not a supported use, we should be returning the error dict here.

This comment has been minimized.

@damon-atkins

damon-atkins May 18, 2018

Member

It covers the following case
salt-call state.high {'strace':{'pkg':['latest']}} where function is call without setting pkgs and hence it is None
which was supported in previous versions.

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented May 18, 2018

if pkgs is not None covers off pkgs=0 which would result in a "Invalidly formatted" message vs trying to upgrade what is contain in variable name.
I have restored it back to if pkgs results shown below.

# salt-call state.single pkg.latest name=vim pkgs=0
[ERROR   ] pkg.latest latest - name: vim, pkgs: 0
[ERROR   ] pkg.latest No information found for 'vim'.
[ERROR   ] state.format_log No information found for 'vim'.
local:
----------
          ID: vim
    Function: pkg.latest
      Result: False
     Comment: No information found for 'vim'.
     Started: 04:30:07.624110
    Duration: 37397.384 ms
     Changes:

Summary for local
------------
Succeeded: 0
Failed:    1
------------
Total states run:     1
Total run time:  37.397 s
# salt-call state.single pkg.latest name=vim pkgs=strace
[ERROR   ] pkg.latest latest - name: vim, pkgs: strace
[ERROR   ] __init__.repack_dictlist Invalid input for repack_dictlist, data passed is not a list (strace)
[ERROR   ] state.format_log Invalidly formatted "pkgs" parameter. See minion log.
local:
----------
          ID: vim
    Function: pkg.latest
      Result: False
     Comment: Invalidly formatted "pkgs" parameter. See minion log.
     Started: 04:31:18.924653
    Duration: 10.632 ms
     Changes:

Summary for local
------------
Succeeded: 0
Failed:    1
------------
Total states run:     1
Total run time:  10.632 ms
# salt-call state.single pkg.latest name=vim
[ERROR   ] pkg.latest latest - name: vim, pkgs: None
[ERROR   ] pkg.latest No information found for 'vim'.
[ERROR   ] state.format_log No information found for 'vim'.
local:
----------
          ID: vim
    Function: pkg.latest
      Result: False
     Comment: No information found for 'vim'.
     Started: 04:33:15.291855
    Duration: 37435.706 ms
     Changes:

Summary for local
------------
Succeeded: 0
Failed:    1
------------
Total states run:     1
Total run time:  37.436 s
@terminalmage

Please perform a git reset --hard 83b4224c and apply the following patch:

diff --git a/salt/states/pkg.py b/salt/states/pkg.py
index 37ac5e5966..f58453837b 100644
--- a/salt/states/pkg.py
+++ b/salt/states/pkg.py
@@ -2455,10 +2455,10 @@ def latest(
             # upgrade the software to the latest. This is per the design.
             # Build updated list of pkgs *with verion number*, exclude
             # non-targeted ones
-            targeted_pkgs = [{x: targets[x]} for x in targets.keys()]
+            targeted_pkgs = [{x: targets[x]} for x in targets]
         else:
             # Build updated list of pkgs to exclude non-targeted ones
-            targeted_pkgs = list(targets.keys()) if pkgs else None
+            targeted_pkgs = list(targets)
 
         # No need to refresh, if a refresh was necessary it would have been
         # performed above when pkg.latest_version was run.
@terminalmage

This comment has been minimized.

Member

terminalmage commented May 18, 2018

Passing name=None requires that pkgs has contents, which it should since we've pre-determined the install targets. So the fix was as simple as getting rid of the if pkgs else None.

Also, there is no need to call .keys() because iterating over a dictionary (or calling list() on one) will simply iterate over the keys. Running list() .keys() allocates a list first on Python 2 and then we are iterating over that list, rather than just iterating over the keys.

@damon-atkins damon-atkins force-pushed the damon-atkins:2017.7.6_fix_pkg.latest_state branch from d15bc0c to 83b4224 May 18, 2018

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented May 18, 2018

FYI:

        if isinstance(pkgs, list) and len(pkgs) == 0:
            return {
                'name': name,
                'changes': {},
                'result': True,
                'comment': 'No packages to install provided'
            }

I cannot see how the above "if" will ever be true.

@terminalmage

This comment has been minimized.

Member

terminalmage commented May 18, 2018

That code is there for a reason. That if will evaluate as True in the case that someone intentionally passes invalid SLS:

foo:
  pkg.installed:
    - pkgs: []

Yes, some people actually have done this in the past and complained that we didn't properly report on their misconfiguration.

@damon-atkins

This comment has been minimized.

Member

damon-atkins commented May 18, 2018

Thanks

@rallytime rallytime merged commit 8a5b34f into saltstack:2017.7.6 May 19, 2018

4 of 9 checks passed

jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23002 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19125 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5068 — FAILURE
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10038 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25258 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17354 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21984 — SUCCESS
Details

rallytime added a commit that referenced this pull request May 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment