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

Jump to bottom

Make the prep_local_devel_env script work for Kubernetes & Openshift #434

Merged
merged 7 commits into from Oct 2, 2017
Merged

Make the prep_local_devel_env script work for Kubernetes & Openshift #434

merged 7 commits into from Oct 2, 2017

Conversation

rthallisey
Copy link
Contributor

Most of the prep_local_devel_env.sh script was shared between the
two clusters. The main differences were:

  • etcd_port: 2379
  • kubernetes_port: 6443
  • routes
  • deployments
  • ca.crt vs service-ca.crt
  • oc to kubectl command

@rthallisey
Copy link
Contributor Author

Worked with Kubernetes. Testing with OpenShift.

@rthallisey
Copy link
Contributor Author

(25) Agnostic Runtime phase 2

@rthallisey
Copy link
Contributor Author

Working for both openshift and k8s

@rthallisey
Copy link
Contributor Author

rthallisey commented Sep 18, 2017 •
edited

Testing OpenShift

Make a new my_local_dev_vars

cp scripts/my_local_dev_vars.example  scripts/my_local_dev_vars

Run prep_local_devel_env script.

./scripts/prep_local_devel_env.sh

Run a local development env.

make run

Testing Kubernetes

Set KUBERNETES to any value. The broker also needs to run in insecure mode because the current kubernetes setup does not have ssl working.

export BROKER_INSECURE="true"
export KUBERNETES=" "

Follow the same steps as OpenShift.

@shawn-hurley shawn-hurley added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed needs-review labels Sep 19, 2017
@rthallisey rthallisey added needs-review and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Sep 25, 2017
@rthallisey
Copy link
Contributor Author

This is working with insecure. I'll need to add ssl shorly since inscure mode is being removed.

djzager
djzager requested changes Sep 27, 2017
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

I like the updates to make prep_local_dev_env work for both Kubernetes and OpenShift. My review comments center around consolidating the library functions, hope this helps.

scripts/prep_local_devel_env.sh Outdated
@@ -130,37 +136,38 @@ fi
echo "TLS Cert: tls.crt"
echo -e "Wrote \n${TLS_KEY_DATA}\n to: ${TLS_KEY}\n"
# Kill any running broker pods
oc scale deploymentconfig asb --replicas 0 -n ${ASB_PROJECT}
deployments scale asb --replicas 0 -n ${ASB_PROJECT}
Copy link
Member

Choose a reason for hiding this comment

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

Should it not be kubectl deployments ... ?

Copy link
Member

Choose a reason for hiding this comment

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

Now that I've tracked it down, I see that this is a function call that just so happens to look like the only thing that is missing is the kubectl in front. I think if you were to put those functions in scripts/lib/cluster.sh, conditionally defined for OpenShift vs Kubernetes, then asb::cluster::deployments would clearly read as a library function call.

scripts/kubernetes/resources.sh Outdated
kubectl get secret ${BROKER_SVC_ACCT_SECRET_NAME} -n ${ASB_PROJECT} -o jsonpath='{ .data.ca\.crt }'
}

function ectd-port {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be etcd-port?

scripts/openshift/resources.sh Outdated
kubectl get secret ${BROKER_SVC_ACCT_SECRET_NAME} -n ${ASB_PROJECT} -o jsonpath='{ .data.service-ca\.crt }'
}

function ectd-port {
Copy link
Member

Choose a reason for hiding this comment

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

Here too.

scripts/cluster-redirect.sh Outdated
@@ -0,0 +1,11 @@
#!/bin/bash
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 that this file + scripts/kubernetes/resources.sh and scripts/openshift/resources.sh should be combined into a single file at scripts/lib/cluster.sh. Looking something like:

if [ -n "${KUBERNETES}" ]; then
    echo "Kubernetes Cluster"
    function asb::cluster::deployments {
        ...
    }
    ...
else
    echo "OpenShift Cluster"
    function asb::cluster::deployments {
        ...
    }
    ...
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I never really liked this cluster redirect script. What I'm going to do is add it to the top of the prep_local_dev_env.sh.

Similar to deploy.sh, I like having the abstraction of a directory for $CLUSTER files. I think it makes it easy to separate $CLUSTER scripts without overloading a single file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@djzager -1 to the combining the files. while it does duplicate somethings it is way easier to see. Does this change kubernetes? open scripts/kubernetes files. Does it affect openshift? open scripts/openshift files. if they are combined you have to be more cognizant not to break the other worrying about the conditionals.

Copy link
Member

@djzager djzager Sep 29, 2017 •
edited

Choose a reason for hiding this comment

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

@jmrodri fair point.

My general feeling is that we are turning a single file into four files into three files (once this file is removed) to prepare the local environment, when there are only subtle differences between how to prepare the environment in k8s vs openshift and that strikes me as overkill.

Having said that, @rthallisey, delete scripts/openshift/resources.shs + scripts/cluster-redirect.sh and I'll change my vote.

EDIT: Added this file to be deleted as it's contents are at the the top of prep local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@djzager we do end up with 4, but prep_local_devel_env.sh is already hard to read and I think this does minimal damage to it's readability. I do think there is a bunch of overlap here and a lot of potential for improvements. This patch is mainly for functionality without changing how the script reads. If you want to improve it, there is definitely an opportunity to and I would encourage it! But, it wasn't my intention with this patch.

I'll remove the extra file.

Ryan Hallisey added 3 commits September 27, 2017 15:39
Make the prep_local_devel_env script work for Kubernetes & Openshift
cb6352a 
Most of the prep_local_devel_env.sh script was shared between the
two clusters. The main differences were:
  - etcd_port: 2379
  - kubernetes_port: 6443
  - routes
  - deployments
  - ca.crt vs service-ca.crt
Create the broker endpoint separately
04e3fb0 
If you create endpoint 'asb', it will be associated with
the service 'asb'. That service, doesn't connect to a running
pod so the endpoint gets no ip address. Instead, create another
endpoint that connect to the service on the same ip:port to use
as the endpoint to contact the broker.
Reorganize scirpts
21ab16a
djzager
djzager approved these changes Sep 29, 2017
View reviewed changes
Copy link
Member

@djzager djzager left a comment

Choose a reason for hiding this comment

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

LGTM

@jmrodri jmrodri merged commit 3b3c411 into openshift:master Oct 2, 2017
rthallisey pushed a commit to rthallisey/ansible-service-broker that referenced this pull request Oct 2, 2017
Pass in args to the deploy scripts
2854946 
The gate showed a false positive on openshift#434, this will fix it.
@rthallisey rthallisey mentioned this pull request Oct 2, 2017
Merged
rthallisey pushed a commit that referenced this pull request Oct 2, 2017
Pass in args to the deploy scripts (#462)
9ad6e9c 
The gate showed a false positive on #434, this will fix it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@djzager djzager djzager approved these changes

@jwmatthews jwmatthews Awaiting requested review from jwmatthews

@dymurray dymurray Awaiting requested review from dymurray

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@rthallisey @jmrodri @djzager @shawn-hurley