Added modules to manage Atomic Host Platform (host and image) #1902

krsacme · 2016-03-24T08:21:58Z

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

atomic_host
atomic_image

ANSIBLE VERSION

ansible 2.1.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = /usr/share/my_modules/

SUMMARY

Atomic Host Plaform provides atomic command to manage the host and to manage the container images on the host. Details of the command can be found at [1]. Added 2 modules in here:
atomic_host :- Module to mange the host itself, upgrade or rollback
atomic_image :- Module to manage the container images

[1] http://www.projectatomic.io/docs/usr-bin-atomic/, https://github.com/projectatomic/atomic/

gregdek · 2016-03-24T13:00:56Z

Thanks @krsacme for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

miabbott · 2016-04-14T21:08:06Z

system/atomic_host.py

+def main():
+    module = AnsibleModule(
+                argument_spec = dict(
+                    state = dict(default=None, choices=['rollback', 'status', 'upgrade', 'rebase'], required=True),


What about support for the deploy verb?

https://github.com/projectatomic/atomic/blob/master/docs/atomic-host.1.md

Also, to properly use the rebase verb, you'll also need to provide a refspec argument.

# atomic host rebase --help usage: atomic host rebase [-h] [--os OS] refspec ... positional arguments: refspec Origin refspec for new deployment args Additional arguments appended to the rebase method. Use `-- --OPTION=VAL` if you want to pass additional unsupported arguments to rpm-ostree. optional arguments: -h, --help show this help message and exit --os OS Operate on provided OSNAME

miabbott · 2016-04-14T21:24:11Z

I'd like to see the atomic host rollback/upgrade commands also support the automatic reboot option that they have. This may not be possible to implement because a reboot would sever the connection Ansible has made to the server, but still worth investigating.

See the reboot options to the commands below:

# atomic host rollback --help
usage: atomic host rollback [-h] [-r] ...

positional arguments:
  args          Additional arguments appended to the rollback method. Use `--
                --OPTION=VAL` if you want to pass additional unsupported
                arguments to rpm-ostree.

optional arguments:
  -h, --help    show this help message and exit
  -r, --reboot  initiate a reboot after rollback is prepared

# atomic host upgrade --help
usage: atomic host upgrade [-h] [-r] [--allow-downgrade] [--os OS]
                           [--check-diff]
                           ...

positional arguments:
  args               Additional arguments appended to the upgrade method. Use
                     `-- --OPTION=VAL` if you want to pass additional
                     unsupported arguments to rpm-ostree.

optional arguments:
  -h, --help         show this help message and exit
  -r, --reboot       if an upgrade is available, reboot after deployment is
                     complete
  --allow-downgrade  Permit deployment of chronologically older trees
  --os OS            Operate on provided OSNAME
  --check-diff       Check for upgrades and print package diff only

tima · 2016-04-14T21:31:41Z

system/atomic_image.py

+        description:
+          - Whether to install (C(install) or C(run) or C(update)), or remove (C(stop) or C(uninstall))
+        required: True
+        choices: ["install", "run", "stop", "update", "uninstall"]


It is considered best practice that Ansible module are declarative. Refrain from verb especially in a param called state like this. Look at other similar modules (don't look at the Docker module -- that one is an anti-pattern we are going to address soon enough) and you'll see state use values like "present", "latest", "absent" if its for deployment or package management. Use "started", "restarted" and "stopped" for process management. I'd hesitate about having a module that does both deployment and service management.

@tima, the atomic image management is similar to the docker, it uses pull (install), start (run). This particular utility is more of both deployment and service management. I will analyze further if it can be limited to only a particular type.
Having said, I will modify the verbs and will re-submit it.

krsacme · 2016-04-22T19:02:43Z

@miabbott As you mentioned, reboot cannot be done as part of this execution, if it required, probably the playbook should be written in such a way that on successful upgrade, the next step should be reboot and that will be the end of playbook. It is appropriate to add a caution as part of the description to remind the user about this. I am just thinking loud.

gregdek · 2016-05-04T22:26:23Z

Thanks @krsacme for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

krsacme · 2016-05-10T10:33:20Z

ready_for_review

gregdek · 2016-05-10T16:26:17Z

Thanks @krsacme for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

miabbott · 2016-05-10T17:09:51Z

system/atomic_host.py

+requirements:
+  - atomic
+options:
+    state:


Would you consider renaming this from state to action or command?

I feel like state does not accurately describe what is happening. Using any of deploy/rebase/rollback/upgarde is really just an action and not really a state of the system. These are more like transitions from one ostree deployment to another.

Agreed @miabbott. A param called state has specific connotations in good Ansible module design which this does not conform. You shouldn't use action either since that is a play directive and could get confusing. I recommend command which is commonplace for most modules like this one.

krsacme · 2016-05-18T10:25:40Z

@bcoca atomic host and image modules are just commands that will be executed. For example, referring to atomic install rhel7/rsyslog command, the expectation of this command is to execute the command assigned to LABEL INSTALL. And atomic run rhel7/rsyslog command does 2 things, checks if the container is running, if not executes the command under LABEL RUN to run the container. I have added the check for already running use case. Rest of the commands are based on the command result only.

krsacme · 2016-05-23T06:03:32Z

ready_for_review

miabbott · 2016-08-10T19:14:55Z

system/atomic_image.py

+version_added: "2.2"
+author: "Saravanan KR @krsacme"
+notes:
+    - Host should be an atomic platform (verified by existence of '/run/ostree-booted' file)


In the case of the atomic update command that is used below, there is no requirement that the host be an Atomic Host.

However, for the commands in the atomic_host.py module, the host does need to be an Atomic Host.

I would suggest moving this note to the atomic_host.py module.

detiber · 2016-08-16T15:49:34Z

shipit

miabbott · 2016-08-16T16:26:03Z

shipit

abadger · 2016-08-17T18:50:02Z

system/atomic_host.py

+    rc, out, err = module.run_command(args, check_rc=False)
+
+    if rc == 77 and revision == 'latest':
+        module.exit_json(msg="Already on latest")


Should specify changed=False explicitly here.

abadger · 2016-08-17T19:04:10Z

I found a couple bugs and had a few questions. Made inline comments for those.

needs_revision

krsacme · 2016-08-18T07:09:06Z

ready_for_review

gregdek · 2016-08-18T10:27:29Z

Thanks @krsacme for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

abadger · 2016-08-18T14:23:16Z

Further probing about the requirement on python-2.6.

needs_info

abadger · 2016-08-18T15:06:00Z

All issues addressed. Merged to devel. Thanks!

maxamillion · 2016-08-18T22:00:05Z

shipit

maxamillion · 2016-08-18T22:00:23Z

oh, ha ... my page hadn't refreshed yet :)

cevich · 2016-08-19T18:24:42Z

system/atomic_image.py

+        if started:
+            args = ['atomic', 'run', image]
+        else:
+            args = ['atomic', 'install', image]


Arguments-wise, IMHO, this doesn't feel very intuitive. It also assumes you can run 'install' w/o first 'uninstall' - that's entirely container-image dependent. How about making latest into a boolean, and reflect the desired state with state?

e.g. atomic_image: name=foo latest=true state=installed
(or uninstalled or running or something like that)

@cevich Thanks for your comments. Sorry for delayed response, stuck with other issues.
There was a discussion [1] with @detiber @tima @bcoca regarding how we should name the arguments. The suggestion was to not to mix package and process management together and finally, came up with above. Please let us know if it make sense.

[1] https://github.com/ansible/ansible-modules-extras/pull/1902/files/50eb61f2070dd19cc4975d2b6f3f705ff2949aa2#r74254464

tima · 2016-08-24T18:18:19Z

I just caught that this got merged.

Great work @krsacme! Thanks for all of your time and persistence on this contribution.

abadger · 2016-08-24T18:22:23Z

@krsacme See @cevich's note -- this will be in the 2.2 release so we have until early/mid september before we can't change the parameters anymore.

Also -- @bcoca mentioned today that this should be categorized in cloud similar to docker and lxd. So I'll move it from system/atomic into cloud/atomic today.

abadger · 2016-08-24T18:35:59Z

@krascme in case it wasn't apparent, if you want to make changes to address @cevich's notes, it would be a new PR. Feel free to ping me in IRC so that we can be sure to look at any backwards incompatible parameter changes before 2.2 goes out.

cevich · 2016-08-24T19:24:34Z

@abadger thanks for consideration. Sorry I didn't get to make it before merge.

cevich · 2016-08-29T13:14:27Z

system/atomic_image.py

+        default: null
+    upgrade:
+        description:
+          - Forcefully upgrade if the container, even if the container is already running


s/upgrade if the container/upgrade the container/

@cevich You are looking at an older version, please take a look at the later revision.

gregdek added new_plugin community_review labels Mar 24, 2016

miabbott reviewed Apr 14, 2016
View reviewed changes

tima reviewed Apr 14, 2016
View reviewed changes

gregdek added needs_revision and removed community_review labels May 4, 2016

krsacme closed this May 10, 2016

jimi-c removed the needs_revision label May 10, 2016

krsacme reopened this May 10, 2016

gregdek added the community_review label May 10, 2016

miabbott reviewed May 10, 2016
View reviewed changes

bcoca added the needs_revision label May 10, 2016

gregdek removed the community_review label May 10, 2016

miabbott reviewed Aug 10, 2016
View reviewed changes

Fixed review comments

3f2146d

tima mentioned this pull request Aug 16, 2016

Standing Agenda, New Modules Meeting ansible/community#92

Closed

abadger reviewed Aug 17, 2016
View reviewed changes

abadger added the needs_revision label Aug 17, 2016

gregdek removed the community_review label Aug 17, 2016

Fixed requirements and locale setting

1a1bc08

gregdek added community_review and removed needs_revision labels Aug 18, 2016

abadger merged commit aacbb97 into ansible:devel Aug 18, 2016

cevich reviewed Aug 19, 2016
View reviewed changes

cevich reviewed Aug 29, 2016
View reviewed changes

Added modules to manage Atomic Host Platform (host and image) #1902

Added modules to manage Atomic Host Platform (host and image) #1902

Conversation

krsacme commented Mar 24, 2016

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

SUMMARY

gregdek commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miabbott commented Apr 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krsacme commented Apr 22, 2016

gregdek commented May 4, 2016

krsacme commented May 10, 2016

gregdek commented May 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krsacme commented May 18, 2016

krsacme commented May 23, 2016

miabbott Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

detiber commented Aug 16, 2016

miabbott commented Aug 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abadger commented Aug 17, 2016

krsacme commented Aug 18, 2016

gregdek commented Aug 18, 2016

abadger commented Aug 18, 2016

abadger commented Aug 18, 2016

maxamillion commented Aug 18, 2016

maxamillion commented Aug 18, 2016

Choose a reason for hiding this comment

krsacme Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

tima commented Aug 24, 2016

abadger commented Aug 24, 2016

abadger commented Aug 24, 2016

cevich commented Aug 24, 2016

Choose a reason for hiding this comment

krsacme Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

miabbott commented Apr 14, 2016 •

edited

Loading

miabbott Aug 10, 2016 •

edited

Loading

krsacme Aug 29, 2016 •

edited

Loading

krsacme Aug 29, 2016 •

edited

Loading