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

Missing core ansible feature json-filter in image #53

Open
jorgemoralespou opened this issue Nov 27, 2018 · 15 comments
Open

Missing core ansible feature json-filter in image #53

jorgemoralespou opened this issue Nov 27, 2018 · 15 comments

Comments

@jorgemoralespou
Copy link

I have an APB that I've had to pin to release-1.2 because of how the parameters where being provided when run manually.https://github.com/openshift-labs/starter-guides/blob/ocp-3.11/apb/Dockerfile#L1

Now, when I run this APB, using the provisioning playbook as documented (https://github.com/openshift-labs/starter-guides/tree/ocp-3.11/apb#provision-1), which has been running fine for months.

Now, I get the following error:

TASK [openshift_eclipse_che : set_fact] **********************************************************************************************************************************
fatal: [localhost]: FAILED! => {"msg": "You need to install \"jmespath\" prior to running json_query filter"}

Which maps to this imported role line: https://github.com/siamaksade/ansible-openshift-eclipse-che/blob/master/tasks/multi_user.yml#L100

This means, that the image is missing python-jmespath. Found this issue (https://bugzilla.redhat.com/show_bug.cgi?id=1484910) but that one should be fixed upstream in the version that is used in this image.

An additional question:

Is there a way to properly identify the images built on Dockerhub to tags on this repository? If I develop using release-1.2 tag of ansibleplaybookbundle/apb-base is there any guarantee the image will be stable through time? Is there non-rolling tags?

@djzager
Copy link
Contributor

djzager commented Nov 27, 2018

@jorgemoralespou python-jmespath should definitely be included in the apb-base image. However, the bugzilla you linked to is for Ansible Engine and isn't directly related to this project (and related images).

@jmontleon looking around at the apb-base-scripts RPM spec on master and release-1.2, it looks like I neglected to add python-jmespath to our dependencies. What do you think is the best way to fix this? Simply add python-jmespath to the RPM spec in master and release-1.2 branches?

Is there a way to properly identify the images built on Dockerhub to tags on this repository? If I develop using release-1.2 tag of ansibleplaybookbundle/apb-base is there any guarantee the image will be stable through time? Is there non-rolling tags?

release-1.2 of the apb-base comes from the release-1.2 branch of this project https://github.com/ansibleplaybookbundle/apb-base/tree/release-1.2

@jmontleon
Copy link
Contributor

jmontleon commented Nov 27, 2018

Yes, as long as it's available adding it to the rpm Requires seems like the way to fix it.

The release-x.y branches generally only receive bug fixes so they should be pretty stable.

@jorgemoralespou
Copy link
Author

@djzager but that release-1.2 is a branch means that if I build something today, and you push something to the branch tomorrow, I will never be able to reproduce it unless I know the exact commit. I would expect that every time an image is pushed to a registry (dockerhub) for wide consumption the code is properly tagged, for reproducibility and in case of needed to revert back, as well as the tags preserved on the registry, at least N-1, so a rolling tag can reference a new image, but it the image is not fine, someone can still use a non-rolling tag in the interim.

@jorgemoralespou
Copy link
Author

@djzager @jmontleon Can I expect a fixed image upstream soon(ish)? Or should I just move my apbs to pin to version release-1.1 for the time being?

@djzager
Copy link
Contributor

djzager commented Nov 27, 2018

but that release-1.2 is a branch means that if I build something today, and you push something to the branch tomorrow, I will never be able to reproduce it unless I know the exact commit. I would expect that every time an image is pushed to a registry (dockerhub) for wide consumption the code is properly tagged, for reproducibility and in case of needed to revert back, as well as the tags preserved on the registry, at least N-1, so a rolling tag can reference a new image, but it the image is not fine, someone can still use a non-rolling tag in the interim.

I don't wholly understand this expectation. Every time that a pull request is merged into the master branch, you'll see a corresponding canary build of apb-base and it's this image we use for testing and identifying issues. nightly builds are RPM installs done every night based on the source you would find in this project on a given day. latest builds are built from a "released" apb-base-scripts RPM that should always work. I would understand having rolling, N-1, or tags based on commit shas if we didn't rely so heavily on RPM packaging. As it stands, if you had an issue with a latest or release-1.x container image, you could very easily inspect the image to see what version of the apb-base-scripts package was installed and infer from where in this project's history you are.

It looks like the release-1.1 apb-base includes jmespath but it is not because it was required by apb-base-scripts:

bash-4.2# rpm -qa | grep 'jmes'
python2-jmespath-0.9.0-3.el7.noarch

bash-4.2# rpm -q --requires apb-base-scripts-1.1.5-1.20180202175344.el7.centos.noarch
/bin/bash
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsXz) <= 5.2-1

@djzager
Copy link
Contributor

djzager commented Nov 27, 2018

Can I expect a fixed image upstream soon(ish)? Or should I just move my apbs to pin to version release-1.1 for the time being?

No. I don't believe you will see a fixed image upstream, at least not one fixed to particular commits of this project. I think the best workaround for you would be to yum -y isntall python2-jmespath in the Dockerfile you mentioned before.

@jorgemoralespou
Copy link
Author

It is very discouraging that we recommend in our documentation to create APBs using FROM ansibleplaybookbundle/apb-base which would use the latest tag. Which can lead to this issues I had, and pretty sure some other would have, if they rebuild (or update) today and APB they had created a couple of months back, as an example.

Not only it has changed how extra-env are provided (#46) which makes every APB that is tested outside Ansible Service Broker to fail, but also it has dropped some ansible (jinja) core functionalities that should be available, and were there at some point (directly or indirectly).

To me this means that we should consider this image and how we (Red Hat) promotes using them can inject breaking changes at any point.

We will implement a solution of adding an additional intermediate image, to prevent from your potential breaking behavior, and adding all these dependencies that are missing. Sadly, we'll need to promote this "workaround" for users to have them on the safe side of things.

Thanks

@djzager
Copy link
Contributor

djzager commented Nov 28, 2018

First, I apologize that my changes (I was the one who originally added jmespath to apb-base canary images, failed to make the appropriate changes to the RPM spec, and also forced the use of --extra-vars '{ <json>}') have impacted you negatively. My desire is to have the opposite impact on your work and the work of others like you.

It is very discouraging that we recommend in our documentation to create APBs using FROM ansibleplaybookbundle/apb-base which would use the latest tag. Which can lead to this issues I had, and pretty sure some other would have, if they rebuild (or update) today and APB they had created a couple of months back, as an example.

I think I better understand your point here although I don't believe I am all the way there. My belief is that the release tags on our apb-base is designed to solve this problem, in that the only updates to these tags are legitimate bug-fixes, otherwise they are left alone. I am not sure though if this would alleviate your concerns.

I would like to make the appropriate changes on our master and release-1.2 branches to include jmespath (this was a mistake and should be corrected), but I also want to ensure I at least investigate how to wholly solve your problem. Aside from including jmespath in the latest and release-1.2 tags of the apb-base image, what more would you like to see?

@jmontleon
Copy link
Contributor

jmontleon commented Nov 28, 2018

@jorgemoralespou if you want to pin it to a more stable version you can do so. There is nothing stopping you from using apb-base:release-1.3, release-1.2, or release-1.1 (alternatively v3.11, v3.10, v3.9 tags, same content). These only receive updates for bugs so they should generally be much more stable.

Maybe that should be called out better in the documentation, but regardless the option is there.

As for why python2-jmespath fell off the image, it looks like older versions of ansible required the package:

$ docker run -it --entrypoint /bin/bash ansibleplaybookle/apb-base:v3.9
bash-4.2# rpm -q --whatrequires python2-jmespath
ansible-2.4.2.0-2.el7.noarch

In newer versions this is not the case and we did not miss the functionality so we didn't catch the change. If there is a compelling reason to add it for newer released versions we can add it in the Dockerfile for apb-base or as a Requires on one of our packages if it makes sense, but there's also no reason you cannot do something like this in your Dockerfile.

FROM ansibleplaybookbundle/apb-base:release-1.1

or

FROM ansibleplaybookbundle/apb-base:release-1.2
RUN yum -y install python2-jmespath && yum clean all

Bare in mind also that ansible can make use of several modules that require python libraries (shade, boto, jmespath, yum, dnf, most likely others...) not explicitly called out in Requires on the RPM. We can try to install them all, but at some point it becomes necessary to weigh the size of the images, which we also get complaints about when they get too large, versus the convenience of having many libraries available on the base image. Maybe jmespath for filters is core enough that we want to add it, I don't really have hard feelings about it either way, but it is not remotely a unique case.

@jorgemoralespou
Copy link
Author

I guess this should be better documented, as this complicates authoring of APBs if what you would expect to be there, which is anything required in core modules, is not there.

When one refers to ansible documentation for the specific filter: https://docs.ansible.com/ansible/2.5/user_guide/playbooks_filters.html#json-query-filter

At the top of the page it's stated:

In addition the ones provided by Jinja2, Ansible ships with it’s own and allows users to add their own custom filters.

So yes, I think this is an expectation any APB author would have when using the APB base images.

Also, I think the documentation should make clear what versions should be used, for which versions of ansible service brokers, and openshift, and any breaking change or release notes should be easier to find or more clear.

@djzager
Copy link
Contributor

djzager commented Nov 28, 2018

I guess this should be better documented

Apologies, it is not clear to me what you mean when you say "this". Could you expand please?

@jorgemoralespou
Copy link
Author

How versioning works and the explanation you've given me.
When reading APB authoring (https://github.com/ansibleplaybookbundle/ansible-playbook-bundle/blob/master/docs/developers.md#dockerfile) one assumes should use FROM ansibleplaybookbundle/apb-base and that's also what apb init creates. One don't know that should pin a version to the from tag to avoid these breaking changes.

I know it's different repository, but you're also maintainers of that one.

This is what I mean with "this". Authoring guidance to prevent these problems I had.

@djzager
Copy link
Contributor

djzager commented Nov 28, 2018

Awesome. Thank you. I'll work on creating some PRs to address those.

@djzager
Copy link
Contributor

djzager commented Nov 30, 2018

@jorgemoralespou after #54 is merged. We will be rebuilding the latest, v3.10, and v3.11 apb-base images. I don't foresee any changes to the v3.10 and v3.11 apb-base images, they should be as good as fixed (any changes would be the result of legit bugs filed in bugzilla).

I'll work on a doc update for APB authoring in the coming days. Thanks.

@jorgemoralespou
Copy link
Author

@djzager Thanks

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

No branches or pull requests

3 participants