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

Migrate dbs to imagestreams #122

Merged
merged 6 commits into from
May 23, 2019
Merged

Conversation

miguelsorianod
Copy link
Contributor

@miguelsorianod miguelsorianod commented May 14, 2019

This PR makes the following DeploymentConfig elements use ImageStreams:

  • backend-redis
  • system-redis
  • system-mysql
  • system-memcache

This makes all DeploymentConfigs of the platform use ImageStreams so we have a more homogeneous way of handling the image changes and has the following advantages:

  • Allow easier updates of the Images (with import-image command)
  • All elements of our platform would use the same method to obtain the images. This should simplify tasks like management of them and the 3scale-operator source code
  • It will be easier to start the steps of probably finally remove ImageStreams in a future step so the platform is compatible with Kubernetes (instead of starting with different scenarios and situations), so I think is a good first step to prepare for that if the need arises.
  • It will allows us to perform image tag management with ImageStream tags at this moment (create floating tags in ImageStreams, etc...)

For each DeploymentConfig element that has been modified we have:

  • Added it the 'amp' ServiceAccount to be used
  • Added it an ImageChange trigger
  • Created an ImageStream for it. The ImageStream has two tags: <3scale-release-version> and "latest" (floating tag that points to <3scale-release-version>
  • Set imagePullPolicy to IfNotPresent (if it was not already there)
  • Updated the image value to use the imagestreamname:latest

Important design decisions:

  • Even though Redis is used by both backend-redis and system-redis we have created two different ImageStreams for them. The reason for this is allowing us to independently change the image at different moments
    *** The versioned ImageStream tag for databases is the 3scale release version number instead of the version number of the database**. This has several consequences: Ideally for every new release we should update the tag version name and update the latest tag to point to the new version tag name, even if the database does not change. This would trigger reboots on the databases I think as with any other image change. Another option would be using the database version number as the tag name as we did with PostgreSQL for example (where we had tag '10' instead of 2.5). However the consequence of that is that for database the tag naming convention would be different for each database and we would need to have different code logic to handle it for each database and we would need to find a way to find the tag name (not sure how we would be able to do that without hardcoding it). What do you think @eguzki ? If we decide to use the 3scale release version for every element included databases I'll also update the PR to modify how PostgreSQL works and use the 3scaleversion number instead of "10" as it is done right now

also I understand that changing the image of an existing system-redis, system-mysql and system-memcache DeploymentConfig (it will trigger a redeploy of the pods) will not be a problem because before this changes we could already be doing it statically if we wanted (for an upgrade for example). Can someone from @3scale/system confirm this? Thank you.

@miguelsorianod miguelsorianod requested a review from eguzki May 14, 2019 13:37
@miguelsorianod miguelsorianod force-pushed the migrate-dbs-to-imagestreams branch 5 times, most recently from 5fb1bbb to d043201 Compare May 16, 2019 09:35
kind: ImageStream
metadata:
annotations:
openshift.io/display-name: System Memcache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memcached 👀

To be changed in many places as well.

@eguzki
Copy link
Member

eguzki commented May 16, 2019

👍 for different IS for redis backend and for redis system.

Regarding DB release version, I think we should stick to using database version to tag name of the imagestream. As you said, we could change 3scale release version and do not change database version, performing unneeded imagestream change and having whatever effect it triggers.

The operator logic might (not yet sure about this) be more complex. IMO we should always prioritize functionality and features eventhough that implies more complex logic to be implemented. IMO having database reboot as a toll for a simpler logic does not make sense.

I might, though, miss some other advantage of having 3scale release version as DB imagestream tag name.

@miguelsorianod
Copy link
Contributor Author

I've been doing a test where a new tag is created but the URL of the image is not updated (the usual case of databases) and I've found that even if you change the tag name, if the image is the same then no redeployment of the database is triggered so, even though that we have to do it for every release (we already do that for the other imagestreams), it would not reboot the databases unless there is a minor bugfix release, in which case a reboot would be desirable so customers get the latest changes.

I think we can apply the PR as is right now.

I also have thought about an alternative that would probably solve the problem of tying the tag to a 3scale release, but due to the possible impact is big, might affect other teams and we don't know how it might impact when we migrate to OLM I think for now this is a good step. Anyway, I leave here the proposed solution that might be discussed in a future issue/pr after commenting it with other teams:

The alternative would be:

For all the ImageStreams, even the ones that are NOT related to databases we would:

  • Have a tag named 'external' that would point to the real image located in the external registry (registry.redhat.io for example)
  • Have a tag 'latest' that would point to the 'external' tag

In this way, we would not tie the ImageStream tags to a specific version number and that would allow us to not having to change the ImageStream tag associated to an image if the image url does not change. In that case the new way to proceed would be:

  • If there's a new CVE or bugfix release inside the current release the only needed thing would be to re-import the images (with oc import-image) to get the changes, as it has been being done until now. This works because we namespace the URL names of the images with the release number (amp25/apicast for example)
  • If there's a new release we would tell the users to update the docker image URLs in the ImageStream whose image change for the release and leave those that don't unmodified.

@miguelsorianod miguelsorianod changed the title [WIP] Migrate dbs to imagestreams Migrate dbs to imagestreams May 23, 2019
@miguelsorianod miguelsorianod force-pushed the migrate-dbs-to-imagestreams branch from d043201 to 124d66e Compare May 23, 2019 14:35
@codeclimate
Copy link

codeclimate bot commented May 23, 2019

Code Climate has analyzed commit 124d66e and detected 17 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 3
Duplication 10
Style 4

View more on Code Climate.

@miguelsorianod miguelsorianod merged commit 6cdbdf1 into master May 23, 2019
@thomasmaas
Copy link
Member

thomasmaas commented May 24, 2019 via email

@miguelsorianod miguelsorianod deleted the migrate-dbs-to-imagestreams branch May 27, 2019 12:17
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.

3 participants