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

Add Native Docker config #3575

Merged
merged 1 commit into from
Jan 24, 2023
Merged

Conversation

aht007
Copy link
Member

@aht007 aht007 commented Aug 25, 2022

ISSUE: openedx-unsupported/devstack#960
This PR is part of effort aimed at removing Ansible based configurations and replacing them with Dockerfile. Currently Devstack Docker images are built using Ansible based configurations in the configurations repository. Through this effort we will make sure that the Repo has its own Dockerfile which has all the necessary configurations to setup small production and dev environments.

Steps to run this Image with Devstack:

  • Build the Image locally first using the target dev i.e. docker build -t image-name-of-choice --target dev .
  • After the image is built successfully go to the docker compose file of devstack and replace the existing discovery image with the one that you built without changing any other configurations there.
  • Run make dev.up.discovery in the terminal while in the devstack directory.
  • Additional Note: For compiling static assets also run provisioning for discovery

@aht007 aht007 force-pushed the aht007/Ansible-to-Docker branch from 93bae51 to d5a7d3f Compare September 6, 2022 07:46
@mphilbrick211
Copy link

@aht007 Just following up on this. You have a failing test, do you plan to troubleshoot and try again? You can look at the details link next to the failed tests to try and find the issue, or check to see if you can find information in the forums.

@aht007 aht007 marked this pull request as draft October 26, 2022 08:44
@aht007
Copy link
Member Author

aht007 commented Oct 26, 2022

@mphilbrick211 This is still in progress and I will get back to it once we prioritize it.

@aht007
Copy link
Member Author

aht007 commented Dec 2, 2022

@rgraber SRE team told me that you might be working on making course-discovery deployable using the in-repo Dockerfile just like some of other services. Meanwhile I am also working on this repo's Dockerfile which will completely change its structure and make it compatible for working with devstack. I wanted to give you a heads up about the changes that would be coming as a result of this PR as well as for getting any comments/suggestions from your side if you have any.
Special mention that the PR is still in progress but it will more or less look like it is currently with just some minor tweaks here an there.

@aht007 aht007 force-pushed the aht007/Ansible-to-Docker branch from 3c98c0b to 3ed3da9 Compare December 6, 2022 08:43

###########################################################
# Define k8s target
FROM app as kubernetes
FROM prod as kubernetes
Copy link
Member Author

Choose a reason for hiding this comment

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

Using prod image as base here instead of app since our newer app image doesn't contain any code files or requirements. Previously the app image was using prod requirements hence I have used the production image as base image for k8s target.

docker_auth:
echo "$$DOCKERHUB_PASSWORD" | docker login -u "$$DOCKERHUB_USERNAME" --password-stdin

docker_push: docker_tag docker_auth ## push to docker hub
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find any usage of these images. For devstack image hosted on edxops is used. As far as I know we started pushing these images for decentralized devstack but that project wasn't completed and these targets have remained here since then.

@aht007 aht007 force-pushed the aht007/Ansible-to-Docker branch from 0cda65b to 7a11a7b Compare December 12, 2022 12:35
@aht007 aht007 marked this pull request as ready for review December 12, 2022 12:35
@aht007
Copy link
Member Author

aht007 commented Dec 12, 2022

Seems like I can't even tag reviewers on the PR 🥲
@rgraber @mphilbrick211 This PR is now ready for review.

@aht007
Copy link
Member Author

aht007 commented Dec 12, 2022

CI / pytest (py38, django32, mysql80, 3) (pull_request) Seems like its a random failure because I have no code changes related to this. Can someone with access to the workflow re trigger this as I don't have access to re run the failed tests.

@mphilbrick211
Copy link

Hi @aht007! Would you mind rebasing this to resolve the branch item? It's out-of-date with the base branch. Also, tagging @ansabgillani here to take a look at your comment re: the failing test and to review once the PR is rebased.

Thanks!

@aht007 aht007 force-pushed the aht007/Ansible-to-Docker branch 2 times, most recently from d4aea69 to 678e11a Compare December 13, 2022 11:22
@aht007
Copy link
Member Author

aht007 commented Dec 13, 2022

I have rebased the PR and now the check CI / pytest (py38, django32, mysql57, 3) (pull_request) failed which is different from the one that failed before. Certainly its because of the flakiness of the tests. It should pass on re run.

@mphilbrick211
Copy link

Hi @aht007 and @ansabgillani! Friendly ping on this :)

Copy link
Contributor

@Ali-D-Akbar Ali-D-Akbar left a comment

Choose a reason for hiding this comment

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

Can you elaborate on the testing instruction? Where does the image go after it has been built by this command and where exactly do we need to update the devstack docker-compose file? Thanks.

Comment on lines +41 to +51
# The current priority is to get the devstack off of Ansible based Images. Once that is done, we can come back to this part to get
# suitable images for smaller prod environments.
# - name: Build and push prod Docker image
# uses: docker/build-push-action@v1
# with:
# push: true
# username: ${{ secrets.DOCKERHUB_USERNAME }}
# password: ${{ secrets.DOCKERHUB_PASSWORD }}
# target: prod
# repository: edxops/discovery-prod
# tags: ${{ steps.get-tag-name.outputs.result }},${{ github.sha }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, the above comment is understandable but why not remove this piece of code for now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just added it so it serves as a reminder to anyone working on this. Also this is supposed to work but we don't just yet want to enable the push because we don't have bandwidth currently for testing and supporting these images just yet.

@aht007
Copy link
Member Author

aht007 commented Dec 22, 2022

@Ali-D-Akbar You can build the image using the instructions that I have mentioned in the description of the PR. Once that is done you can checkout to the branch in this PR openedx-unsupported/devstack#994. It has everything setup for you but you will need to change the image name on this line with the image name that you have built locally using this PR. After that you have to run provisions for the discovery service and start your service as you would do it normally on devstack

with:
script: |
const releasePrefix = 'refs/tags/open-release/';
const tagName = context.ref.split(releasePrefix)[1] || 'latest';
Copy link
Contributor

Choose a reason for hiding this comment

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

curious: what does context.ref imply?

Copy link
Member Author

Choose a reason for hiding this comment

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

GitHub context contains information about the workflow run and the event that triggered the run.
In this case we are extracting the ref variable from it which contains the Github branch that triggered the run. If that branch is an open-release branch we extract its release name e.g. maple or nutmeg else we know that it was run from master branch and hence use the latest tag.

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

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

I have not tested this locally yet. Will this effect the discovery k8s event bus deployment?

@aht007
Copy link
Member Author

aht007 commented Dec 23, 2022

@DawoudSheraz In theory it shouldn't effect the k8s deployment. But for being on safe side I already requested a review from @rgraber.

@rgraber
Copy link
Contributor

rgraber commented Jan 3, 2023

I'm in the process of testing this locally

@rgraber
Copy link
Contributor

rgraber commented Jan 3, 2023

I was able to deploy a course discovery worker locally with minikube.
For my own clarification, will this new image be picked up by the pipeline here (internal link) https://github.com/edx/edx-internal/blob/master/gocd/generated-pipelines/output/course-discovery.yaml ? That pipeline uses course-discovery-private, which uses course-discovery as a base.

@aht007
Copy link
Member Author

aht007 commented Jan 4, 2023

@rgraber In my view there shouldn't be any issues since the build stages are the same and I can also see the commands for building the image and everything looks fine to me. I will still check with an SRE team member for confirmation on this and will get back to you.

Copy link
Contributor

@rgraber rgraber left a comment

Choose a reason for hiding this comment

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

@nadeemshahzad clarified this for me. Looks good and worked locally.

@mphilbrick211
Copy link

Hi @aht007! Looks like you'll need to rebase to fix the branch date alert. Then it should be OK to merge.

CC: @ansabgillani for merging

@aht007 aht007 force-pushed the aht007/Ansible-to-Docker branch from 79b971c to 48e97a7 Compare January 5, 2023 06:30
@aht007
Copy link
Member Author

aht007 commented Jan 10, 2023

@openedx/incident-management I would need help with merging and monitoring this PR. For now this PR will not affect devstack since we wouldn't be using this Image for now for devstack but we would have to monitor event bus k8s deployment after this PR. For devstack I have a separate PR which I will merge later on once this goes smoothly.

@aht007 aht007 force-pushed the aht007/Ansible-to-Docker branch from 5ce22a6 to 6fef84d Compare January 24, 2023 06:10
@Ali-D-Akbar Ali-D-Akbar force-pushed the aht007/Ansible-to-Docker branch from 6fef84d to 2aabb09 Compare January 24, 2023 09:09
@Ali-D-Akbar Ali-D-Akbar merged commit d09d527 into openedx:master Jan 24, 2023
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.

7 participants