Skip to content

Conversation

@hendolim
Copy link
Contributor

Supporting python 3 on all platform.
Added simple checks to see if python3 is properly installed and python2 is still the default python.
For redhat, we have to explicitly set python2 as default after installing python3, otherwise python3 will be the default python.

Copy link
Contributor

@mikedickey mikedickey left a comment

Choose a reason for hiding this comment

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

Am I right that this would put 2 python interpreters inside the containers, one for v2 and one for v3? I don't think this is good idea. We should just pick one and stick with it.

@bb03
Copy link
Contributor

bb03 commented Aug 14, 2019

This is the first step to ensure python 2/3 compatibility. The changes to the interpreter will come later. At that point we will transition to python 3 only.

@mikedickey
Copy link
Contributor

OK, that makes sense but I think we should keep these changes out of develop. Maybe put it into an epic branch instead? And merge to develop only after the migration to v3 is complete?

@bb03
Copy link
Contributor

bb03 commented Aug 14, 2019

@mikedickey Why? Since this is our develop branch, I think its reasonable to merge this in given our existing test coverage. If there are any additional tests that you think @hendolim should run that would give you additional piece of mind, we can run those now and/or incorporate them into our CI

@mikedickey
Copy link
Contributor

Best practice is generally to only merge completed items into the develop branch, so that you are feasibly able to ship develop at any time. This change in particular introduces a material regression in image size, which will impact other work streams that rely on being able to test from develop. I would consider this state to be "unshippable" and feel it should be isolated in its own branch until completed. Perhaps the missing tests here are to ensure image size stays below a certain threshold?

@bb03
Copy link
Contributor

bb03 commented Aug 15, 2019

@mikedickey @hendolim
Since this is a measurable issue, lets take a look at what the current file sizes are, as built on a local laptop

splunk-debian-9:latest              dd93e9930393        24 seconds ago      1.25GB
splunk-debian-9:python3             a4007d808dd5        9 minutes ago       1.29GB
bare-debian-9:latest              87fc154e9b66        32 seconds ago      1.24GB
bare-debian-9:python3             5d6c61925800        9 minutes ago       1.27GB
minimal-debian-9:latest              586f7f20cb23        55 seconds ago      497MB
minimal-debian-9:python3             699fb3e39f10        10 minutes ago      531MB
base-debian-9:latest              a2bb93026118        2 minutes ago       219MB
base-debian-9:python3              ef7eb8301dae        15 minutes ago      252MB

So it looks like this adds ~40 MB (overall, consistently across the image layers). This increases the image layer by ~ 3.8%

Because file size is the only complaint raised, lets make a small adjustment. We'll keep the python2 interpreter - which will be in splunk/splunk:edge and then we'll add another build layer for python3 which we can call splunk/splunk:edge-py23.
In this way we can start playing around with images that have both python 2+3 in them, the current "edge ready" code can be tested early and often, and the image size won't increase. Downstream consumers can have the choice of using edge or splunk/splunk:edge-py23 and will give a better debugging experience around issues if they can switch interpreters without needing to spin up a second container.

We can also use this PR to make sure that the image size stays below a certain threshold. Although given the current sizes, I don't have a reasonable expectation for that threshold. We can take a look at the current splunk/splunk:latest for the size. I have

splunk/splunk:latest              b771ccc31b0a        2 weeks ago         1.28GB

And it looks like edge would go over that by ~ 10 MB. So we can probably introduce a test along the lines of "edge shall be smaller than or equal to latest" or "edge should never be greater than 1.01*latest"

Sound reasonable?

EDIT: Formatting

@mikedickey
Copy link
Contributor

@bb03 Sounds like a good plan to me, although note that because this change is in the base image, we'd need to create a separate set of "edge-py23" containers for everything, not just splunk/splunk. An alternative might be to add the extra py3 layers to the splunk/splunk image only, and move it back up to base when it's time to get rid of py2 and make the switch over. Although it may be easier to just use an epic branch and build the different variants out of there...

Ideally, I think the unit tests should be added for each image to ensure the larger size of splunk/splunk doesn't hide more significant regression in the other variants. Maybe something along the lines of checking:

  • base < 225MB
  • minimal < 500MB
  • splunk < 1.26GB

I think hard numbers would be better because it would help protect against bloat from gradually creeping in over time. Of course, hard numbers would have to be different for each base..

@bb03
Copy link
Contributor

bb03 commented Aug 15, 2019

@mikedickey The alternative plan was what I was thinking of, since the changes here are relatively self-contained, we could just add a single image layer that contains the python 3 code. Then when it's time to do the switchover, we can change it just use python 3 and delete python 2.

@bb03
Copy link
Contributor

bb03 commented Aug 17, 2019

@mikedickey Are your review comments addressed?

bb03
bb03 previously approved these changes Aug 19, 2019
mikedickey
mikedickey previously approved these changes Aug 19, 2019
@hendolim hendolim dismissed stale reviews from mikedickey and bb03 via 7c64d3e August 19, 2019 20:47
@hendolim hendolim requested a review from mikedickey August 20, 2019 00:08
@hendolim hendolim requested a review from arctan5x August 20, 2019 20:29
@hendolim hendolim merged commit 83b8759 into develop Aug 20, 2019
@nwang92 nwang92 deleted the port-to-python3 branch October 29, 2019 21:01
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.

4 participants