X Tutup
The Wayback Machine - https://web.archive.org/web/20201023084754/https://github.com/koding/koding/pull/11419
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

Dev. env. container-per-worker implementation for Kubernetes #11419

Merged
merged 8 commits into from Oct 23, 2017

Conversation

@mertaytore
Copy link
Member

@mertaytore mertaytore commented Aug 29, 2017

All workers run on different containers in backend pod, landing & client builds are done in frontend pod and external services (rabbitmq, postgres, redis, countly & mongo) run on their respective replication controllers for development environment.

Description

Details on some modifications:
generateKonfig.coffee: Kubernetes' API runs on 8080. Therefore, webserver needs to run on another port, configured to be 8040 for this implementation.
nginx.coffee: Added port 8040 to webserver's upstream for nginx.conf generation. This addition will provide the port for both build types, docker-compose and k8s.
bootstrap-container: Added build_k8s. bootstrap-container build command was not removed because the command can potentially be used in GitLab integration
kontrol.go: Kontrol is mislead by the environment variables that are generated by the Kubernetes service resource exposing Kontrol. Kontrol's environment loader tries to load KONTROL_PORT=tcp://<clusterIP-of-kontrol>:3000 as kontrol's port value but it fails since the value is not an integer to be parsed. This change did not result in any errors when Koding was built with docker-compose.

The change regarding kontrol.go is open for other workarounds.

Motivation and Context

This implementation is worked on top of #11338 with no supervisor execution.

How Has This Been Tested?

No warnings or errors occurred on coffeelint executions on the .coffee files that were modified

Types of changes

  • New feature (non-breaking change which adds functionality)
@mertaytore mertaytore requested review from cihangir and szkl Aug 29, 2017
export CHANGE_MINIKUBE_NONE_USER=true
sudo -E minikube start --vm-driver=none
sleep 60
envsubst < deployment/kubernetes/build-pod.yaml.template > deployment/kubernetes/build-pod.yaml

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

I missed this on previous PR. Could you use (or eliminate the necessity) something else other than envsubst MacOS does not come with it.

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

I think we can make it a required dependency. It is a part of gettext, actually. So, it might not be installed by GNU+Linux distros. What do you think?

brew install gettext
brew link --force gettext

It is located in $BREW_PREFIX/opt/gettext/bin/envsubst.

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

We might even not need this if we start working generating templates from JSON objects related

KONFIG.supervisorConf = (require '../deployment/supervisord.coffee').create KONFIG
KONFIG.kubernetesConf = (require '../deployment/kubernetes.coffee').create KONFIG
for name, options of KONFIG.workers when options.kubernetes?.ports?
fs.writeFileSync "./deployment/kubernetes/backend-pod/#{name}-svc.yaml", (require '../deployment/kubernetes.coffee').createWorkerServices name, options

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

Could you separate this into multiple lines where first you create the data to be written and write it later?

Copy link
Contributor

@cihangir cihangir left a comment

We need to work on the configuration.

return mountConf


generateVolumesSection = ->

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

could you generate this section from the KONFIG.workers.volumes?

kubernetes :
image : 'nginx'
command : " [ \"nginx\", \"-c\", \"#{KONFIG.projectRoot}/nginx.conf\" ] "
mounts : [ 'koding-working-tree', 'assets' ]

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

Could you define this mounts in KONFIG.worker as separate properties? We will have greater visibility in what we can mount in k8

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

While generating the mounts for kubernetes templates, check if the mount is pre-defined in KONFIG, if not fail.

ports :
containerPort : "#{KONFIG.kontrol.port}"
hostPort : "#{KONFIG.kontrol.port}"
envVariables : [

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

You dont need to define every env vars as key-value pairs. Just define the required konfig params and read them from KONFIG.

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

Also while reading from KONFIG, check if the required env var is in KONFIG, if not fail.

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

We can also consider re-configuring once service host is set.

@@ -116,13 +174,34 @@ module.exports = (KONFIG, options, credentials) ->
]
healthCheckURLs : [ "http://localhost:#{KONFIG.kloud.port}/healthCheck" ]
versionURL : "http://localhost:#{KONFIG.kloud.port}/version"
kubernetes :
image : 'koding/base'
command : " [ \"./run\", \"exec\", \"#{GOBIN_K8S}/kloud\" ] "

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

ps: You can use """ to remove double quote escaping.

mounts : [ 'koding-working-tree', 'root-kite-volume', 'generated-volume' ]
ports:
containerPort : "#{KONFIG.kloud.port}"
hostPort : "#{KONFIG.kloud.port}"

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

We can eliminate the host port in next iteration, we wont need it when we have service descriptions in place.

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

Until then you can get rid of redundant quotes.

@@ -116,13 +174,34 @@ module.exports = (KONFIG, options, credentials) ->
]
healthCheckURLs : [ "http://localhost:#{KONFIG.kloud.port}/healthCheck" ]
versionURL : "http://localhost:#{KONFIG.kloud.port}/version"
kubernetes :
image : 'koding/base'

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

We should not need the command section here. This worker definition already has it.

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

Same applies for ports section too.

mounts : [ 'koding-working-tree', 'root-kite-volume' ]
ports :
containerPort : '4560'
hostPort : '4560'

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

Much space.

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

My drunk coder detector is detecting a drunk coder :trollface:


containerSection =
"""
\n - name: #{container.name}

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

You dont need to work with strings. First generate an object then use a yaml generator to convert that to yaml.

@@ -15,6 +15,10 @@ createUpstreams = (KONFIG) ->
servers += '\n' if servers isnt ''
port = parseInt(port, 10)

if name is 'webserver' and options.kubernetes.ports?

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

This is not the place where we should handle port swapping. Please pass a new flag to configure and do this in first step.

@@ -41,6 +41,13 @@ function run() {
esac
}

function k8s_build() {

This comment has been minimized.

@cihangir

cihangir Sep 5, 2017
Contributor

please apply shfmt.

@szkl seems we need ci/check-lint from packer repo here as well...

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

Noted.

Copy link
Member

@szkl szkl left a comment

There are many places repeating the same information.

.gitignore Outdated

deployment/kubernetes/backend-pod/
deployment/kubernetes/frontend-pod/client-containers.yaml
deployment/kubernetes/build-pod.yaml

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

I think deployment/kubernetes would be enough.

@@ -255,7 +255,7 @@ module.exports = (options, credentials) ->
# TODO: average request count per hour for a user should be measured and a reasonable limit should be set
nodejsRateLimiter : { enabled: no, guestRules: [{ interval: 3600, limit: 5000 }], userRules: [{ interval: 3600, limit: 10000 }] } # limit: request limit per rate limit window, interval: rate limit window duration in seconds
nodejsRateLimiterForApi : { enabled: yes, guestRules: [{ interval: 60, limit: 5 }], userRules: [{ interval: 60, limit: 60 }] } # limit: request limit per rate limit window, interval: rate limit window duration in seconds
webserver : { port: 8080 }
webserver : { port: 8080, k8sPort: 8040 }

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

No way, Jose.

@@ -105,7 +105,12 @@ Configuration = (options = {}) ->
sh: (require './generateShellEnv').create KONFIG, options
json: JSON.stringify KONFIG, null, 2

if not fs.existsSync './deployment/kubernetes/backend-pod' then fs.mkdirSync './deployment/kubernetes/backend-pod'

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

You can add that directory to repository with a file named .empty in it.

@@ -5,14 +5,55 @@ module.exports = (KONFIG, options, credentials) ->
GOBIN = '%(ENV_KONFIG_PROJECTROOT)s/go/bin'
GOPATH = '%(ENV_KONFIG_PROJECTROOT)s/go'

GOBIN_K8S = "#{KONFIG.projectRoot}/go/bin"
GOPATH_K8S = "#{KONFIG.projectRoot}/go"

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

This should be avoided.

stopsignal : 'QUIT'
kubernetes :
image : 'nginx'
command : " [ \"nginx\", \"-c\", \"#{KONFIG.projectRoot}/nginx.conf\" ] "

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

JSON in a string is a no-go.

@@ -15,6 +15,10 @@ createUpstreams = (KONFIG) ->
servers += '\n' if servers isnt ''
port = parseInt(port, 10)

if name is 'webserver' and options.kubernetes.ports?
webserverPort = parseInt(options.kubernetes.ports.containerPort, 10)
servers += "\tserver 127.0.0.1:#{webserverPort + index} max_fails=3 fail_timeout=10s;\n"

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

This is basically duplicating line 22.

# more info on guest user update: https://www.rabbitmq.com/blog/2014/04/02/breaking-things-with-rabbitmq-3-3/
k8s_health_check $RABBITMQ_POD_NAME koding 5 120 rabbitmq
sleep 10
kubectl exec -n koding $RABBITMQ_POD_NAME -c rabbitmq -- bash -c "rabbitmqctl add_user test test && rabbitmqctl set_user_tags test administrator && rabbitmqctl set_permissions -p / test '.*' '.*' '.*'"

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

Break down into multiple lines.

else
echo "Unknown command: $1"
exit 1
fi

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

Missing a newline at the EOF.

kubectl delete -f $1
}

if [ "$1" == "k8s_connectivity_check" ]; then

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

You don't have to add a conditional clause for each command. See if you can just find out if there is a function defined with the same name as "$1".

@@ -116,13 +174,34 @@ module.exports = (KONFIG, options, credentials) ->
]
healthCheckURLs : [ "http://localhost:#{KONFIG.kloud.port}/healthCheck" ]
versionURL : "http://localhost:#{KONFIG.kloud.port}/version"
kubernetes :
image : 'koding/base'

This comment has been minimized.

@szkl

szkl Sep 5, 2017
Member

Same applies for ports section too.

@mertaytore mertaytore force-pushed the mertaytore:container-per-worker branch 22 times, most recently from 6d836a1 to 14f9f1a Sep 10, 2017
@szkl
szkl approved these changes Sep 12, 2017
@mertaytore mertaytore force-pushed the mertaytore:container-per-worker branch from 14f9f1a to d14d53a Sep 12, 2017
Copy link
Contributor

@cihangir cihangir left a comment

nothing blocker apart from -R and -r

export NAMESPACE_DIR="${KONFIG_PROJECTROOT}/deployment/kubernetes/namespace.yaml"
export BACKEND_DIR="${KONFIG_PROJECTROOT}/deployment/kubernetes/backend-pod/containers.yaml"
export EXT_SERVICES_DIR="${KONFIG_PROJECTROOT}/deployment/kubernetes/external-services/ -R"

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

what is this "-R" here?

@@ -1,15 +1,22 @@
export KONFIG_PROJECTROOT=/opt/koding

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

we may need to get rid of from this special config file sooner than it might seem ;)

envsubst < deployment/kubernetes/frontend-pod/client-containers.yaml.template > deployment/kubernetes/frontend-pod/client-containers.yaml
cp $BACKEND_DIR ${KONFIG_PROJECTROOT}/deployment/generated_files
cp $BACKEND_DIR ${KONFIG_PROJECTROOT}/deployment/generated_files -r

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

if this -r meant for recursive, please move it next to the command itself with its corresponding capital case.

kubectl exec -n $2 $1 -c $3 -- bash -c "./run is_ready" || exit 1
}
${KONFIG_PROJECTROOT}/scripts/k8s-utilities.sh create_k8s_resource ${KONFIG_PROJECTROOT}/deployment/kubernetes/external-services/mongo
export MONGO_POD_NAME=$(kubectl get pods --namespace koding -l "app=mongo-ext-service" -o jsonpath="{.items[0].metadata.name}")

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

you can create a new variable for general options that will be passed to each command ie: export KUBECTL_OPTS="--namespace koding" on your next iterations.

This comment has been minimized.

@mertaytore

mertaytore Sep 13, 2017
Author Member

Noted and will make the appropriate changes on the next iteration.

remaining_time=600

until eval "check_pod_response $1 $2 $3"; do
sleep 30

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

30 is a rather big number for an interval. 3-5 would be more suitable.

${KONFIG_PROJECTROOT}/scripts/k8s-utilities.sh create_k8s_resource ${KONFIG_PROJECTROOT}/deployment/kubernetes/external-services/countly
export COUNTLY_POD_NAME=$(kubectl get pods --namespace koding -l "app=countly-ext-service" -o jsonpath="{.items[0].metadata.name}")
${KONFIG_PROJECTROOT}/scripts/k8s-utilities.sh check_pod_state $COUNTLY_POD_NAME Pending
sleep 40

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

why we have sleeps around the code? check_pod_state already ensures that the state is in desired state.


KONFIG.kubernetesConf.spec.containers.push generateContainerSection name, workerOptions

KONFIG.kubernetesConf.spec.volumes = generateVolumesSection KONFIG, options

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

You should create the volumes spec from the KONFIG itself, not from your prior knowledge. This is the same reason that i commented earlier.

Anyways please create an issue to refactor these volume creation steps and we can leave this as is for now.

This comment has been minimized.

@mertaytore

mertaytore Sep 13, 2017
Author Member

I opened an issue for it and will change it on the next iteration.

@@ -0,0 +1,25 @@
apiVersion: v1

This comment has been minimized.

@cihangir

cihangir Sep 13, 2017
Contributor

ps: replication controller is shortened with "rc"

see your filename

@mertaytore mertaytore force-pushed the mertaytore:container-per-worker branch 3 times, most recently from e949e6f to e01d3df Sep 13, 2017
@mertaytore mertaytore force-pushed the mertaytore:container-per-worker branch 6 times, most recently from d3aa997 to 6e7c59e Sep 27, 2017
@mertaytore mertaytore force-pushed the mertaytore:container-per-worker branch from 6e7c59e to 1f6c7ff Oct 5, 2017
@tardyp
Copy link

@tardyp tardyp commented Oct 17, 2017

Looks like this is useful. Can we merge it?

@cihangir cihangir merged commit 0397e89 into koding:master Oct 23, 2017
2 checks passed
2 checks passed
codacy/pr Good work! A positive pull request.
Details
license/cla Contributor License Agreement is signed.
Details
@cihangir
Copy link
Contributor

@cihangir cihangir commented Oct 23, 2017

@mertaytore you can send the fixes for this PR on your next PRs. ie: removing command section from kubernetes config in workers.coffee

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.
X Tutup