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

Revert july9 prs #154

Merged
merged 3 commits into from
Jul 12, 2020
Merged

Revert july9 prs #154

merged 3 commits into from
Jul 12, 2020

Conversation

gabemontero
Copy link
Contributor

@frenzymadness @lvarin

Since the combination of PRs

merged earlier today, we've started seeing docker builds from openshift templates defined in this repo start to fail.

I've posted the full build log in https://projects.engineering.redhat.com/browse/RHELPLAN-48796

It looks like there is a disconnect with the sqlite version that django expects. Perhaps there is a pending image update that is necessary?

Can you all report back with any analysis, including either determination of which commit(s) caused the disconnect, or what additional items are needed to get that build working again?

We can give it some time, but in the interim, any OpenShift customer attempting to use the template in question will encounter errors, so if fixing this gets too prolonged, we will to revert your recent changes until things can be sorted out.

thanks

/assign @bparees
@coreydaley FYI

This reverts commit bd5e352, reversing
changes made to 97c98a0.
…corn-19.5.0"

This reverts commit 97c98a0, reversing
changes made to c57ec85.
This reverts commit c57ec85, reversing
changes made to 6702b2b.
@frenzymadness
Copy link
Member

Hello. I am sorry for the trouble this causes you. The problem is expected although I did not know about OS template building directly from this repository.

The problem is (as you can see in the error message) that newer Django (>=2.2) needs SQLite 3.8.3 but RHEL/Centos 7 have 3.7.17. The reason why I've updated this repository to Django 2.2 is that this version is LTS and all older versions 1.11 LTS, 2.0 and 2.1 are no longer supported.

We were aware of this incompatibility for older RHEL/Centos so I've created a separated branch 1.11.x so users of RHEL/Centos 7 can still use the old 1.11.x version of Django.

Would it be possible to:

  • keep templates for RHEL/Centos 8 pointing to master branch with new LTS Django and
  • switch templates for RHEL/Centos 7 to the 1.11.x branch?

@bparees
Copy link
Contributor

bparees commented Jul 10, 2020

@gabemontero is this only impacting the django template? (not the django-postgresql template which uses a postgresql image, not sqlite)

samples operator doesn't appear to ship the pure django template, so we could be ok here if that's the case. (at least from a customer impact perspective).
https://github.com/openshift/cluster-samples-operator/tree/master/tmp/build/assets/operator/ocp-x86_64/django/templates

(we'd still have to decide what to do about the imageeco e2e test)

@gabemontero
Copy link
Contributor Author

@gabemontero is this only impacting the django template? (not the django-postgresql template which uses a postgresql image, not sqlite)

samples operator doesn't appear to ship the pure django template, so we could be ok here if that's the case. (at least from a customer impact perspective).
https://github.com/openshift/cluster-samples-operator/tree/master/tmp/build/assets/operator/ocp-x86_64/django/templates

(we'd still have to decide what to do about the imageeco e2e test)

@bparees the only template e2e's are failing on is django-psql-example ... which is one we ship. As I recall, there was an initiative to only ship templates with persistence if possible.

The test is doing a oc new-app django-psql-example -e WEB_CONCURRENCY=1 and the associated BC build run fails.

We do not manually create that template and then instantiate it. It is a template that is installed.

@bparees
Copy link
Contributor

bparees commented Jul 10, 2020

The test is doing a oc new-app django-psql-example -e WEB_CONCURRENCY=1 and the associated BC build run fails.

ok. I assumed since the error was on sqllite, that a deployment using psql would not have an issue, but i guess i'm wrong.

So if we really want to do the "right" thing here for customers we need to:

  1. revert master and keep master pristine since customer clusters have the template that references master, and also OCP4.2-4.5 will be continuing to ship that template for the next year.

  2. introduce a new branch of the django sample that does the updates

  3. update the template to use the branch (assuming the published SCL images can work w/ the updated code.. i'm not entirely clear on the python image side of the issues here)

  4. declare the master branch dead/legacy for now (must stay pinned to where it's at so we don't re-break the templates that are out in the wild and reference it)

  5. someday when we're confident no one is using a template that refs the master branch any more, we can resume updating the master branch (but probably want to leave the template referencing an explicit branch so we can avoid hitting this problem again in the future)

@gabemontero
Copy link
Contributor Author

The test is doing a oc new-app django-psql-example -e WEB_CONCURRENCY=1 and the associated BC build run fails.

ok. I assumed since the error was on sqllite, that a deployment using psql would not have an issue, but i guess i'm wrong.

So if we really want to do the "right" thing here for customers we need to:

1. revert master and keep master pristine since customer clusters have the template that references master, and also OCP4.2-4.5 will be continuing to ship that template for the next year.

OK, so we have this PR for that

2. introduce a new branch of the django sample that does the updates

So I believe this is the opposite of what @frenzymadness noted he did in #154 (comment)

And this is necessary to support users on shipped versions of openshift ... namely 4.2 to 4.5

@bparees @frenzymadness does that make sense ... do you all understand and agree?

If so, @frenzymadness , can you take on creating this new branch which includes the commits from the PRs from yesterday?

3. update the template to use the branch (assuming the published SCL images can work w/ the updated code.. i'm not entirely clear on the python image side of the issues here)

So this means in the new, "not master" branch in this repo, we want https://github.com/sclorg/django-ex/blob/master/openshift/templates/django-postgresql.json#L98 to point to the new branch but again not master branch .... we want the URL to changes such that "master" is the new branch name.

That means that https://github.com/sclorg/django-ex/blob/master/openshift/templates/django-postgresql.json#L456-L460 would need to be updated (again, substitute the new branch name for master) to specify a default now ... namely our new branch

Is that something you can take on @frenzymadness as well?

4. declare the master branch dead/legacy for now (must stay pinned to where it's at so we don't re-break the templates that are out in the wild and reference it)

5. someday when we're confident no one is using a template that refs the master branch any more, we can resume updating the master branch (but probably want to leave the template referencing an explicit branch so we can avoid hitting this problem again in the future)

Presumably this is when 4.5 goes EOL, which would be either 2 or 3 years from now (don't remember off the top of my head.

@bparees
Copy link
Contributor

bparees commented Jul 10, 2020

Presumably this is when 4.5 goes EOL, which would be either 2 or 3 years from now (don't remember off the top of my head.

should be less than that. I believe 4.5 will EOL when 4.8 ships, so 9-12 months. Luckily this isn't in 4.6 since that's the extended support release (assuming we get this fixed up before 4.6 ships). see: https://access.redhat.com/support/policy/updates/openshift#dates

@bparees
Copy link
Contributor

bparees commented Jul 10, 2020

So I believe this is the opposite of what @frenzymadness noted he did in #154 (comment)

yeah. we need master to work on the old version because master is what all the templates in the field are already pointing to.

@frenzymadness
Copy link
Member

Although I try to understand where the problem is (I am sorry, I am not OpenShift user, just s2i-python-container maintainer) I don't like the idea that we'll show an obsoleted version with probably more obsoleted dependencies (with security vulnerabilities) to users coming to this repository looking for a good example. That's the idea why I've updated the master branch to supported versions and kept the older one harder to find.

I can definitely switch the two branches so the new code will be in 2.2.x and the old one moved from 1.11.x to master. But only if there is no better solution.

@frenzymadness
Copy link
Member

Side note: There were issues and PRs for updates years old so It'd be nice to maintain this example repository more proactively. There are also a lot of dead links to OS documentation.

@bparees
Copy link
Contributor

bparees commented Jul 10, 2020

Although I try to understand where the problem is (I am sorry, I am not OpenShift user, just s2i-python-container maintainer) I don't like the idea that we'll show an obsoleted version with probably more obsoleted dependencies (with security vulnerabilities) to users coming to this repository looking for a good example.

I agree i'm not thrilled with having to leave master in such a state either, but the sample is primarily consumed by openshift customers, so breaking it for all of them isn't a viable path forward. Again, switching the template to explicitly ref a non-master branch is a good solution to get us out of this eventually (when we feel a sufficient portion of our customers would no longer be using the old template).

If there is another alternative, i'm certainly open to it.

Side note: There were issues and PRs for updates years old so It'd be nice to maintain this example repository more proactively. There are also a lot of dead links to OS documentation.

Absolutely agreed. This sample is owned by the SCL team that owns the SCL images it is run on top of. Which it sounds like you're a part of as the s2i-python maintainer. I appreciate that you've gone through and addressed the long-standing idle PRs, that is what the SCL team was supposed to be doing all along. The reason @gabemontero and I are engaged is because we(openshift) run tests to confirm that those templates keep working, and in this case these changes broke those tests (Which means they broke the templates for our customers also). Ideally the SCL team (which also owns the template itself) is confirming they do not merge changes to the images, templates, or the examples the templates consume, that break them, though I understand that can be challenging to ensure, which is part of why we keep running the tests on the openshift side.

There's no finger pointing here, just trying to find the healthiest way to move forward w/ the least impact to our users.

@frenzymadness
Copy link
Member

My plan is:

  1. Create a new branch 2.2.x from the current master branch
  2. Reset current master branch to commit 6702b2b (from Apr 14) which is the HEAD of 1.11.x branch now & force-push it.
  3. Remove branch 1.11.x
  4. Update notes in readmes in both branches to reflect actual state.

Does that sound good to you?

@gabemontero
Copy link
Contributor Author

gabemontero commented Jul 11, 2020 via email

@bparees
Copy link
Contributor

bparees commented Jul 11, 2020

Reset current master branch to commit 6702b2b (from Apr 14) which is the HEAD of 1.11.x branch now & force-push it.

merging a revert is generally preferred to rewriting git history, since rewriting history screws up anyone who's got a fork of this repo, or other branches.

@frenzymadness frenzymadness merged commit be79965 into sclorg:master Jul 12, 2020
@frenzymadness
Copy link
Member

  • changes in master are reverted by this PR
  • there is a new branch with the latest updates 2.2.x
  • branch 1.11.x has been removed
  • both branches contain notes about their status in the readme

Is that enough to fix OpenShift troubles? Could you please take a look on #155 ? Is it safe to merge it?

@gabemontero
Copy link
Contributor Author

I started off a test job in openshift/cluster-samples-operator#300 @frenzymadness

the e2e-aws-image-ecosystem will run the test we need to confirm things

I personally do not know enough about django/python to know if #155 is safe based on just looking at it. But when I have a cluster up on Monday I'll see about hand crafting an OpenShift build that runs off of your PRs branch and let you know the results.

@gabemontero gabemontero deleted the revert-july9-prs branch July 13, 2020 11:54
@bparees
Copy link
Contributor

bparees commented Jul 13, 2020

thanks @frenzymadness @gabemontero !

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.

None yet

3 participants