[merged] Refactor Pull, Update, Install, Run by baude · Pull Request #825 · projectatomic/atomic

baude · 2017-01-16T21:47:14Z

Refactor several of the atomic verbs and subverbs to take advantage
of object refactoring.

Also, do not pull images with skopeo if the local image is already
at the latest.

$ sudo python ./atomic --debug pull busybox
Namespace(_class=<class 'Atomic.pull.Pull'>, assumeyes=False, debug=True, func='pull_image', image='busybox', reg_type=None, storage='docker')
Latest version of busybox already present.

baude · 2017-01-16T22:18:52Z

@giuseppe would you mind peeking at the system_container test failure? Maybe you can hint me on whats wrong there and I can fix it.

yuqi-zhang · 2017-01-16T23:16:38Z

The tests are failing due to the "secret" value that was set for "$RECEIVER" during install was not set, so $RECEIVER took the default value of "world". From a glance "self.args.setvalues" (which contained the secret value) was overridden at some point before it reached _do_checkout in syscontainers.py

rhatdan · 2017-01-17T13:25:46Z

Atomic/backends/_docker.py

+        insecure = True if util.is_insecure_registry(self.d.info()['RegistryConfig'], util.strip_port(registry)) else False
        trust = Trust()
-        trust.set_args(pull_args)
+        #trust.set_args(pull_args)


Why is this commented out?

Aaron and I speculated that these are no longer needed. I commented it out to test it. Just need to delete them. Will do now.

rhatdan · 2017-01-17T13:27:10Z

Atomic/backends/_docker.py

-    def update(self, name, force=False):
+    def update(self, name, force=False, **kwargs):
+        debug = kwargs.get('debug', False)
+        # Only delete containers if a new image is actually pulled.


Not sure what this comment means?

The comment was misplaced a little. I have moved it down. If you use update with --force, existing containers based on the old image are deleted. So in the case that a new image is not pulled, the containers should not be deleted.

rhatdan · 2017-01-17T13:31:31Z

Tests are failing

rhatdan · 2017-01-17T13:31:40Z

@giuseppe PTAL

giuseppe · 2017-01-17T14:43:08Z

I agree with @yuqi-zhang on this, could you please verify why the args are not passed correctly?

Also, it is a huge patch, I think it will make sense to split it in one for each refactored command. It will make easier to spot the problem.

giuseppe · 2017-01-17T14:27:46Z

Atomic/objects/image.py

    def __eq__(self, other):
-        if self.long_version == other.long_version:
+        if self.long_version == other.long_version \
+                and not any([True for x in [self.long_version, other.long_version] if x is None]) \


is this None not in [self.long_version, other.long_version] ?

Yes much better

giuseppe · 2017-01-17T14:28:53Z

Atomic/objects/image.py

-        if self.long_version == other.long_version:
+        if self.long_version == other.long_version \
+                and not any([True for x in [self.long_version, other.long_version] if x is None]) \
+                and not any([True for x in [self.long_version, other.long_version] if x is '']):


and this '' not in [self.long_version, other.long_version] ?

Yes much better

baude · 2017-01-17T14:56:54Z

Im looking into the tests. Because all of these commands are so linked, it does not work well to split up frankly. I would prefer to keep them together. if that is going to keep it from being merged, Ill see what I can do.

Refactor several of the atomic verbs and subverbs to take advantage of object refactoring. Also, do not pull images with skopeo if the local image is already at the latest. $ sudo python ./atomic --debug pull busybox Namespace(_class=<class 'Atomic.pull.Pull'>, assumeyes=False, debug=True, func='pull_image', image='busybox', reg_type=None, storage='docker') Latest version of busybox already present.

miabbott · 2017-01-17T19:45:51Z

If docker is not running, atomic pull throws a cryptic error; could be improved:

# systemctl status docker
● docker.service - Docker Application Container Engine
   Loaded: loaded (/usr/lib/systemd/system/docker.service; disabled; vendor preset: disabled)
   Active: inactive (dead) since Tue 2017-01-17 19:45:10 UTC; 3s ago
     Docs: http://docs.docker.com
  Process: 19000 ExecStart=/usr/bin/dockerd-current --add-runtime oci=/usr/libexec/docker/docker-runc-current --default-runtime=oci --containerd /run/containerd.sock --exec-opt native.cgroupdriver=systemd --user
 Main PID: 19000 (code=exited, status=0/SUCCESS)
# pwd
/root/atomic
# git rev-parse HEAD
f9a69999e3e7f2ba27fa033389577ed3dfed8aeb
# ./atomic pull docker.io/nginx
('Connection aborted.', error(2, 'No such file or directory'))

miabbott · 2017-01-17T20:00:49Z

The user gets different errors when trying to pull an image that does not exist and choosing the different storage backend

# ./atomic pull --storage ostree foobar
ReturnTuple(return_code=1, stdout='', stderr='time="2017-01-17T19:59:34Z" level=fatal msg="unauthorized: authentication required" \n')
# ./atomic pull foobar
Unable to resolve foobar

miabbott · 2017-01-17T20:13:10Z

Not sure how this should work, but it looks off...

When using atomic run to run a container that is already running, the command appears to start a new container with a different ID. However, the output from atomic containers list shows that the previously running container is intact and its ID has not changed.

I would suggest changing the output to indicate that the container is already running or otherwise notifying the user that the new container ID is not going to be used.

# ./atomic containers list
   CONTAINER ID IMAGE                COMMAND              CREATED          STATE     RUNTIME   
   d7843c1a7f8c docker.io/cockpit/ws /container/atomic-ru 2017-01-17 19:59 running   docker    
# ./atomic run docker.io/cockpit/ws
/usr/bin/docker run -d --privileged --pid=host -v /:/host docker.io/cockpit/ws /container/atomic-run --local-ssh

This container uses privileged security switches:

INFO: --pid=host 
      Processes in this container can see and interact with all processes on the host and disables SELinux within the container.

INFO: --privileged 
      This container runs without separation and should be considered the same as root on your system.

For more information on these switches and their security implications, consult the manpage for 'docker run'.

50736975bed2f07fa9eeea91228d57340b3feeb13118e74c9f8aba40a37a7a70
# ./atomic containers list
   CONTAINER ID IMAGE                COMMAND              CREATED          STATE     RUNTIME   
   d7843c1a7f8c docker.io/cockpit/ws /container/atomic-ru 2017-01-17 19:59 running   docker    
# ./atomic run docker.io/cockpit/ws
/usr/bin/docker run -d --privileged --pid=host -v /:/host docker.io/cockpit/ws /container/atomic-run --local-ssh

This container uses privileged security switches:

INFO: --pid=host 
      Processes in this container can see and interact with all processes on the host and disables SELinux within the container.

INFO: --privileged 
      This container runs without separation and should be considered the same as root on your system.

For more information on these switches and their security implications, consult the manpage for 'docker run'.

5ad399a88ebdfb3c8cd039d622a88e224844b2f927f704546bc9785f0e05616b
# ./atomic containers list
   CONTAINER ID IMAGE                COMMAND              CREATED          STATE     RUNTIME   
   d7843c1a7f8c docker.io/cockpit/ws /container/atomic-ru 2017-01-17 19:59 running   docker

rhatdan · 2017-01-17T20:32:36Z

Yes the second atomic run on the same container will just exec into the container if it is already running. So it should indicate that it is joining the existing container.

baude · 2017-01-17T22:12:34Z

Per agreement with @rhatdan and @miabbott , I will merge this and then xfer each of the identified issues and resolve them separately.

baude · 2017-01-17T22:16:32Z

@rh-atomic-bot r+

rh-atomic-bot · 2017-01-17T22:16:33Z

📌 Commit ffd28c9 has been approved by baude

rh-atomic-bot · 2017-01-17T22:16:39Z

⌛ Testing commit ffd28c9 with merge c7aafb3...

rh-atomic-bot · 2017-01-17T22:28:33Z

☀️ Test successful - status-atomicjenkins
Approved by: baude
Pushing c7aafb3 to master...

rhatdan · 2017-01-17T22:38:59Z

make PYTHON=python3 install

is blowing up.
************* Module Atomic.util
E:466,76: Module 're' has no 'MULTILINE' member (no-member)
E:483,67: Module 're' has no 'MULTILINE' member (no-member)
E:579,17: Module 're' has no 'UNICODE' member (no-member)

baude · 2017-01-17T23:18:53Z

@rhatdan Im not able to replicate that. Is it rawhide maybe?

rhatdan · 2017-01-18T14:16:54Z

What version of python3-pylint do you have?

rhatdan reviewed Jan 17, 2017

View reviewed changes

giuseppe reviewed Jan 17, 2017

View reviewed changes

baude force-pushed the no_pull_already_have branch 3 times, most recently from 97f802c to f9a6999 Compare January 17, 2017 17:18

baude force-pushed the no_pull_already_have branch from f9a6999 to ffd28c9 Compare January 17, 2017 21:25

baude mentioned this pull request Jan 17, 2017

'atomic images update' pulls latest image even if it is on the host #812

Closed

rh-atomic-bot changed the title ~~Refactor Pull, Update, Install, Run~~ [merged] Refactor Pull, Update, Install, Run Jan 17, 2017

rh-atomic-bot closed this in c7aafb3 Jan 17, 2017

jlebon mentioned this pull request Jan 18, 2017

atomic run regression #838

Closed

Conversation

baude commented Jan 16, 2017

Uh oh!

baude commented Jan 16, 2017

Uh oh!

yuqi-zhang commented Jan 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Jan 17, 2017

Uh oh!

rhatdan commented Jan 17, 2017

Uh oh!

giuseppe commented Jan 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Jan 17, 2017

Uh oh!

miabbott commented Jan 17, 2017

Uh oh!

miabbott commented Jan 17, 2017

Uh oh!

miabbott commented Jan 17, 2017

Uh oh!

rhatdan commented Jan 17, 2017

Uh oh!

baude commented Jan 17, 2017

Uh oh!

baude commented Jan 17, 2017

Uh oh!

rh-atomic-bot commented Jan 17, 2017

Uh oh!

rh-atomic-bot commented Jan 17, 2017

Uh oh!

rh-atomic-bot commented Jan 17, 2017

Uh oh!

rhatdan commented Jan 17, 2017

Uh oh!

baude commented Jan 17, 2017

Uh oh!

rhatdan commented Jan 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants