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

Allow multiple versions for a docker image. Fixes #2017. #2069

Merged

Conversation

FroggyFlox
Copy link
Member

Note: Although this pull-request (PR) is working as expected after testing of the underlying logic, I still need to test on openSUSE variants so I'm submitting this as Draft for now. I will update once done.

Building on #2014, this pull request proposes to consider the docker container image tag in combination to the image name when parsing a rock-on JSON definition file. This would thus allow the use of different versions (tag) for the same docker container within a given rock-on, or across rock-ons.

Aims & Logic

While both the image name and the tag is taken into account when parsing the information from the rock-on JSON definition file, only the image name is used to select the DImage object that needs to be created/updated. As a result, any image tag information from a given image will overwrite the image tag of any pre-existing DImage object if it shares the same name (i.e. same docker project).
This PR fixes this simply by also accounting for the image tag when selecting the DImage object to be created/updated.

Demonstration

On a Rockstor install freshly updated to upstream master, the following custom rock-on was uploaded to the local rockons-metastore:

{
	"Alpine With AddStorage Single": {
		"containers": {
			"alpine36": {
				"image": "alpine",
				"tag": "3.6",
				"launch_order": 1,
				"opts": [
					[
						"-it",
						""
					]
				],
				"ports": {},
				"volumes": {},
				"environment": {}
			},
  			"alpine_edge": {
				"image": "alpine",
				"tag": "edge",
				"launch_order": 2,
				"opts": [
					[
						"-it",
						""
					]
				],
				"ports": {},
				"volumes": {},
				"environment": {}
			},
			"alpine3": {
				"image": "alpine",
				"tag": "3",
				"launch_order": 3,
				"opts": [
					[
						"-it",
						""
					]
				],
				"ports": {},
				"volumes": {},
				"environment": {}
			},
			"alpinesingle": {
				"image": "alpine",
				"launch_order": 4,
				"opts": [
					[
						"-it",
						""
					]
				],
				"ports": {},
				"volumes": {},
				"environment": {}
			}
		},
		"description": "Alpine test tags Rock-on.",
		"ui": {
			"slug": ""
		},
		"volume_add_support": true,
		"website": "",
		"version": "1.0"
	}
}

As you can see above, this rock-on is simply designed to start 4 different containers of the same docker project (alpine) but each using a different image version. While only the last json entry was retained previously (as each new image entry was overwriting the previous one), the current PR allows all these containers to be started as intended:

[root@rockdev ~]# docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
alpine              3                   961769676411        3 weeks ago         5.58MB
alpine              latest              961769676411        3 weeks ago         5.58MB
alpine              edge                70997d35b3ed        4 weeks ago         5.58MB
alpine              3.6                 43773d1dba76        6 months ago        4.02MB

[root@rockdev ~]# docker ps
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
1443f4ad2c71        alpine:latest       "/bin/sh"           39 minutes ago      Up 39 minutes                           alpinesingle
119400f1b6ba        alpine:3            "/bin/sh"           39 minutes ago      Up 39 minutes                           alpine3
469e70da98b1        alpine:edge         "/bin/sh"           39 minutes ago      Up 39 minutes                           alpine_edge
4ad024b3c1ae        alpine:3.6          "/bin/sh"           39 minutes ago      Up 39 minutes                           alpine36

Of note, the image tag information is also accounted for when using the delete-rockon script so that the correct image is removed from the system if the script is used.

@phillxnet, please note that I also added a few log=True flags to some docker run commands, in the same "spirit" than your recent PR (#2014). feel free to remove them if you think it's not necessary. I do find it useful when debugging.

Testing

Soon...

… of rock-on JSON definition file.

Add image tag to docker rmi command to ensure the correct image is deleted if two different image versions are present on the system.
@FroggyFlox
Copy link
Member Author

I now just tested it in both Leap15.1 and Tumbleweed and confirmed it works as expected as well.
@phillxnet, I now think it is ready for review; let me know if I forgot anything.

@FroggyFlox FroggyFlox marked this pull request as ready for review September 14, 2019 17:19
@phillxnet
Copy link
Member

@FroggyFlox Thanks, this looks like a neat fix. I'll have a better look shortly and hopefully we can get it merged soon. Thanks for testing all around. Much appreciated.

@FroggyFlox
Copy link
Member Author

Fixes #2017.

@phillxnet
Copy link
Member

@FroggyFlox Apologies for the delay on this one. I'm getting through my build infrastructure backlog and should be ready to merge / release this one soon.

Thanks again for your effort on this one. Will be great to finally have it in place and released.

@phillxnet
Copy link
Member

phillxnet commented Nov 12, 2019

Quick test of how this pull request behaves over an rpm update.

storageadmin_dimage:
Using your provided demo example rock-on, much appreciated:

3.9.2-50

id name tag repo
41 adtac/gollum-alpine latest na
78 alpine edge na
28 aptalca/zoneminder-1.29 latest na
...

with:

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
titpetric/netdata   latest              236a0771591f        3 hours ago         298MB
alpine              edge                7eacb6761fa1        6 weeks ago         5.59MB

rpm update 3.9.2-50 to 3.9.2-50.2069

id name tag repo
41 adtac/gollum-alpine latest na
81 alpine 3 na
80 alpine 3.6 na
79 alpine latest na
78 alpine edge na
28 aptalca/zoneminder-1.29 latest na
...
docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
titpetric/netdata   latest              236a0771591f        3 hours ago         298MB
alpine              edge                7eacb6761fa1        6 weeks ago         5.59MB

But stop, uninstall, re-install exmple json and we have our expected full complement:

docker images
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
titpetric/netdata   latest              236a0771591f        3 hours ago         298MB
alpine              3                   965ea09ff2eb        3 weeks ago         5.55MB
alpine              latest              965ea09ff2eb        3 weeks ago         5.55MB
alpine              edge                7eacb6761fa1        6 weeks ago         5.59MB
alpine              3.6                 43773d1dba76        8 months ago        4.02MB

@phillxnet
Copy link
Member

@FroggyFlox This is an excellent pull request and well done on finding and fixing this bug. And thanks for providing your proof of fix. Very helpful.

Much appreciated.

Happy to merge and again apologies for taking far too long to get around to this.

@phillxnet phillxnet merged commit ce3412f into rockstor:master Nov 12, 2019
@FroggyFlox
Copy link
Member Author

Thanks a lot, @phillxnet, especially for providing testing of its behavior across rpm update... I should have included that as well in my report.

@FroggyFlox FroggyFlox deleted the Issue2017_Rock-on_parse_Image_Tag branch February 3, 2020 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants