honour tag element within rock-on json. Fixes #2014

[phillxnet]

Works by simply appending the already processed 'tag' json element within Rock-on definition files to docker image related commands.

Summary:

• honour existing "tag" element as defined in rockon-registry repo
• add debug logging to the main docker commands.
• minor docstring addition.

Fixes #2014

Ready for review by contributor with docker and Rock-on code experience.

Testing:

A test case Rock-on used was the existing duplicati2-canary.json as it has, unlike almost all our Rock-ons, a 'tag' element specified.
This Rock-on was installed pre-pull request and the indicated image used was latest (Docker Hub default):

CONTAINER ID	IMAGE	COMMAND	CREATED	STATUS	PORTS	NAMES
b0967279e794	intersoftlab/duplicati	"/entrypoint.sh"	6 minutes ago	Up 6 minutes	0.0.0.0:8200->8200/tcp	duplicati2-canary

Post pr we get the stipulated image variant (via it's tag):

CONTAINER ID	IMAGE	COMMAND	CREATED	STATUS	PORTS	NAMES
f017b9e812eb	intersoftlab/duplicati:canary	"/entrypoint.sh"	About a minute ago	Up About a minute	0.0.0.0:8200->8200/tcp	duplicati2-canary

Caveats:

When removing and re-installing this Rock-on, having applied the changes proposed here. we are left with the old image:

docker image list

REPOSITORY               TAG                 IMAGE ID            CREATED             SIZE

intersoftlab/duplicati   latest              276bb62b547d        6 months ago        477MB

intersoftlab/duplicati   canary              bbeb28956224        9 months ago        476MB

Currently published affected Rock-ons:

• owncloud (suspected broken currently for unrelated reasons)
• duplicati2-canary

Haproxy-letsencrypt, and Collabora Online both also have the tag element but it is set to 'latest'.

[phillxnet]

@FroggyFlox I'm popping this one in now as it's a dependency for another pr I'm working on in rockon-registry. I will rebase later if need be as we go as I know you have made a number of changes in this area that are both pending in pr and pre pr.

[FroggyFlox]

Thanks for fixing this! I didn't even realize that we had the "tag" supported and parsed... I should have paid more attention.

Regarding the image uninstall, I believe it is "simply" due to the fact that we do not remove the docker image upon Rock-on uninstall. Luckily the docker rmi command support tags so if/when we implement that, we would be able to easily deal with such situation. Should we open a new issue to remove docker images? That would need a quick usage check for the image before removing it, but I believe we have already all elements in our database to do that.

I know you have made a number of changes in this area that are both pending in pr and pre pr

I do, indeed, have extensive additions and edits to rockon_helpers.py in "pre-PR" state, but they won't be ready that soon so do not worry about these for now.

I'll test and make a proper Github review of this one if you want, but that seems good at first glance. I'll have a better look at it as soon as possible.

[phillxnet]

@FroggyFlox Thanks for the review offer. Much appreciated.
As per orphaned image / container clean-up, there were plans to use a 'new at the time' docker command to do just that, can't find the discussion now unfortunately. May have just been in my notes.
Also see #1795 for a prior discussion on leaving images post Rock-on uninstall. We could re-visit that of course as ultimately it's a non sustainable practice. Have a think and we can spin off another issue if need be. Pretty much a corner case with regard to this issue but I wanted to mention it as it's kipple.

[phillxnet]

@FroggyFlox We can always just lookup which share is our Rock-ons-root as we do when blocking that share's deletion. So we can likewise look to a snapshots parent share to see if it is thus. At least I think so (bit late here now).

Plus I found the prior chat re image pruning via docker:
See pr: #1653 and it's comments.
Plus the following forum thread referenced there:
https://forum.rockstor.com/t/solved-docker-hub-applications/447/11

docker system prune

And it's variations is what we were waiting for and now have. More details in forum thread.

[FroggyFlox]

Thanks for all the pointers and references of prior discussions, @phillxnet ; as I didn't know about them, it's always very interesting and useful ;-)

Also see #1795 for a prior discussion on leaving images post Rock-on uninstall. We could re-visit that of course as ultimately it's a non sustainable practice. Have a think and we can spin off another issue if need be. Pretty much a corner case with regard to this issue but I wanted to mention it as it's kipple.

Agreed to re-visit this... I'll be meaning to open a discussion about that (here or forum), especially after repeatedly discussing in the forum the use of an "advanced scripts" section that could include delete-rockon and docker prune tools... Off topic here, but definitively worth laying out the best way to implement a solution to these issues.

We can always just lookup which share is our Rock-ons-root as we do when blocking that share's deletion. So we can likewise look to a snapshots parent share to see if it is thus. At least I think so (bit late here now).

I forgot about that one... I've been so focus on the network side of Rockstor lately that I can't see anything else. I'll keep this for the corresponding issue too.

I'll try to come back with a review to the current PR as soon as possible.

[FroggyFlox]

I started testing your PR and it does work as intended.

First, thanks for adding log=true to these commands. This is something I almost always do in my dev environment but always ignore when committing as I thought it would not have been desired behavior. It is extremely helpful not only for debugging but also for troubleshooting issues any user may have so I'm definitely happy to see this :-)

Regarding your PR, I'll submit a proper review with comments where I do have some, but those are only that: comments. The code works as indicated.

There is one comment I have that is not related to your commits, but actually concerns how we are parsing the "tag" object from the json. Note also that it is a "fringe" case that I do not really see happening in reality.

The code in question is from rockon.py:

            io, created = DImage.objects.get_or_create(name=c_d['image'],
                                                       defaults=defaults)

As stated above, we are feeding the DImage model filtering by image name, for each container in the json, so this does not allow to have the same image with different tags in the same rock-on, such as in the following test json:

		"containers": {
			"alpine36": {
				"image": "alpine",
				"tag": "3.6",
				"launch_order": 1,
				"ports": {},
				"volumes": {},
				"environment": {}
			},
  			"alpine_edge": {
				"image": "alpine",
				"tag": "edge",
				"launch_order": 2,
				"ports": {},
				"volumes": {},
				"environment": {}
			},
			"alpine3": {
				"image": "alpine",
				"tag": "3",
				"launch_order": 3,
				"ports": {},
				"volumes": {},
				"environment": {}
			}

As mentioned above, however, I'm not sure this would happen in real use, but should we prepare for that? It does not break the system, only prevents the use of different tags.

Notably, it also indicates another issue more likely to arise as the above applies if two different rock-ons use the same image but with a different tag. The first rock-on to be processed with take priority in this case, such that the second one will use the tag of the first one.

Again, these are not directly related to the issue your PR addresses, so I can create a dedicated one if you prefer.

Cheers,

[phillxnet]

@FroggyFlox Thanks for taking a look at this pr and checking functionality. Much appreciated.

Re your comment on our image/tag db table (storageadmin_dimage) that's a nice find, missed that myself, and definitely deserves a focused issue. But I agree with you that it's rather in the corners, but if we have the issue we can more easily keep it in mind. Especially as we get more Rock-ons / images.

Re the log=True I think is the way to go for this as it's still very much in development (by you mainly) and only logs if debug is enabled anyway so shouldn't affect production releases. And as you say makes for easier diagnosis 'in the field' as when debug is on we can see those docker commands and their call order.

Cheers.

[FroggyFlox]

As mentioned in the specific comments, changes in openvpn_install and owncloud_install are ok, but I can't seem to find a usage for these methods anymore. I'll try to test whether they are still required when I get the chance.
Changes in the PR are OK, tested in 3.9.2-47

[phillxnet]

@FroggyFlox Let me know if you manage to confirm the openvpn / owncloud related code changes as I'm pretty sure I found they were not in dead code, as per my replies to your code comment review.

Thanks for the review by the way and see how you get on with that confirmation.
I can then mark as resolved at this end.

[phillxnet]

@FroggyFlox Thanks for being so diligent on this by the way.

[FroggyFlox]

@FroggyFlox Let me know if you manage to confirm the openvpn / owncloud related code changes as I'm pretty sure I found they were not in dead code, as per my replies to your code comment review.

I finally tracked them down... I should have been more attentive while reading this, but here's where these methods are called:
In rockon_helpers.py:

def install(rid):
    new_state = 'installed'
    try:
        rockon = RockOn.objects.get(id=rid)
        globals().get('%s_install' % rockon.name.lower(),
                      generic_install)(rockon)

Thanks a lot for finding these out, @phillxnet!

[phillxnet]

@FroggyFlox Well done tracking that down, good to have gotten that sorted. So are we happy with this pull request's changes as they stand, review wise; so we can put it to bed? At least then we can 'mostly' honour tags going forward, given your nice fine re limitation of scope bug and the linked issue you opened.

[FroggyFlox]

Yes, I'm all ok with your changes, sorry I sidetracked the conversation :-(