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

Split front-end and back-end containers #173

Merged
merged 1 commit into from
Jul 3, 2017

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Jun 29, 2017

Depends on ManageIQ/manageiq#15470

The frontend ManageIQ pod will receive all web traffic, all frontend replicas must run the UI, API and WS roles (no change here).

Backend replicas will not receive any UI / API / WS requests, so those roles should not be enabled by default. Backend replicas should also wait for the frontend service to become available before attempting to start the application. Backend replicas should be set to 0 by default and users can scale it based on their needs.

This change allows us to scale with a smaller footprint since we usually need more MiqServers for operations and inventory roles than for UI/API roles.

@bdunne bdunne requested a review from carbonin June 29, 2017 19:21
@bdunne bdunne force-pushed the split_manageiq_pod branch 3 times, most recently from 09f67e9 to 6753ebc Compare June 30, 2017 19:56
Copy link
Member

@carbonin carbonin left a comment

Choose a reason for hiding this comment

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

So I think there is quite a bit that should be able to be removed from the "backend" Dockerfile right?

Hopefully we don't need the SUI stuff and don't need to install all the node deps.

Also we can probably remove the EXPOSE line from that one.

@@ -0,0 +1,69 @@
FROM bdunne/miq-pods-test:backend-latest
Copy link
Member

Choose a reason for hiding this comment

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

I think we'll need this changed before we merge.

Also, on that note, should we change the directory in this repo to manageiq-app-backend?

WORKDIR ${APP_ROOT}
RUN source /etc/default/evm && \
export RAILS_USE_MEMORY_STORE="true" && \
rake update:bower && \
Copy link
Member

Choose a reason for hiding this comment

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

Can this stuff come out of the other Dockerfile?

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 wish, but manageiq-ui-classic

@@ -2,6 +2,7 @@

/usr/sbin/crond &

container-initialize.sh
Copy link
Member

Choose a reason for hiding this comment

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

I think we can do better naming this. Maybe check-dependent-services.sh?

annotations:
description: "Defines how to deploy the ManageIQ appliance"
spec:
serviceName: "${NAME}-backend"
Copy link
Member

Choose a reason for hiding this comment

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

Don't think this has a service right?

@@ -107,7 +107,7 @@ objects:
spec:
containers:
- name: manageiq
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to change this dc and container to be named manageiq-frontend?

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 don't think so. I don't want to give it a name that suggests it is limited to only serving the UI / API / WS roles.

requests:
storage: "${APPLICATION_VOLUME_CAPACITY}"


Copy link
Member

Choose a reason for hiding this comment

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

Bonus whitespace?

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've been adding a break between some of these things for readability

Copy link
Member Author

Choose a reason for hiding this comment

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

These files are impossible

value: "${NAME}"
-
name: "MEMCACHED_SERVER"
value: "${MEMCACHED_SERVICE_NAME}:11211"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this in the backend?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I'm not sure.

value: "${MEMCACHED_SERVICE_NAME}:11211"
-
name: "MEMCACHED_SERVICE_NAME"
value: "${MEMCACHED_SERVICE_NAME}"
Copy link
Member

Choose a reason for hiding this comment

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

Same, I'm not sure we need this.

value: "${DATABASE_SERVICE_NAME}"
-
name: "DATABASE_REGION"
value: "${DATABASE_REGION}"
Copy link
Member

Choose a reason for hiding this comment

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

If this pod is never going to initialize the region I don't think we need to provide this. I think the region id will be detected using the existing database sequence.

# Check readiness of external services
check_svc_status ${MEMCACHED_SERVICE_NAME} 11211
check_svc_status ${DATABASE_SERVICE_NAME} 5432

Copy link
Member

Choose a reason for hiding this comment

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

Bonus whitespace 😉

By default, set the backend replica count to 0
@miq-bot
Copy link
Member

miq-bot commented Jun 30, 2017

Checked commit bdunne@8d8bf40 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy Fryguy dismissed carbonin’s stale review July 3, 2017 17:42

I think all of @carbonin's objections have been covered.

@Fryguy Fryguy merged commit d997b69 into ManageIQ:master Jul 3, 2017
@Fryguy Fryguy added this to the Sprint 64 Ending Jul 3, 2017 milestone Jul 3, 2017
@bdunne bdunne deleted the split_manageiq_pod branch July 3, 2017 17:43
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants