Navigation Menu

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

Add an nginx docker image for use on the master. #6334

Merged
merged 1 commit into from Apr 2, 2015
Merged

Add an nginx docker image for use on the master. #6334

merged 1 commit into from Apr 2, 2015

Conversation

brendandburns
Copy link
Contributor

ArtfulCoder
ArtfulCoder reviewed Apr 2, 2015
View reviewed changes
cluster/images/nginx/nginx.json
"readOnly": true},
{ "name": "passwd",
"mountPath": "/usr/share/nginx",
"readOnly": false}
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need /usr/share/nginx to be writeable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@brendandburns
Copy link
Contributor Author

Comments addressed and PR updated. ptal.

@ArtfulCoder ArtfulCoder added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2015
@ArtfulCoder
Copy link
Contributor

LGTM
can be merged during business hours.

I didn't test the container with the readonly mounts, but I am guessing that Brendan manually verified it.

@brendandburns
Copy link
Contributor Author

updated, to actually activate the container via Salt.

Please wait to merge tomorrow, I want to run e2e. Will ping this PR when I have a passing e2e.

ArtfulCoder
ArtfulCoder reviewed Apr 2, 2015
View reviewed changes
cluster/saltbase/salt/nginx/init.sls
@@ -1,6 +1,8 @@
nginx:
{% if grains.cloud in ['gce'] %}
Copy link
Contributor

Choose a reason for hiding this comment

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

If gce, then nginx should not be installed isn't it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, yes. fixed.

@ArtfulCoder
Copy link
Contributor

Are you introducing this for gce only as a rollout strategy ? or is this change only for gce ?

@brendandburns
Copy link
Contributor Author

yes, only gce is a rollout strategy. I'll move to AWS and others next.

@ArtfulCoder ArtfulCoder removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2015
@brendandburns
Copy link
Contributor Author

Ok, this now passes e2e. I actually added back in the install of the nginx .deb since it provides much of the /etc/nginx/... (e.g. mime types) files that we rely on. We should really package all of this except the kubernetes site in the container image, but deferring that to a different PR (or when we eliminate nginx)

@brendandburns
Copy link
Contributor Author

ptal.

Thanks!
--brendan

@dchen1107
Copy link
Member

LGTM except I wish we have a Makefile introduced, so that we could track the versions of all bundled components including nginx here.

To do this, you can simple convert the README file to a Makefile.

@dchen1107
Copy link
Member

Also have you make sure cluster upgrade works too?

@ArtfulCoder
Copy link
Contributor

@brendanburns You would need to disable service explicitly for gce so that nginx in a pod, can start successfully.

@brendandburns
Copy link
Contributor Author

@dchen1107
Makefile added.
Tested upgrade with:

git checkout master
make clean; make quick-release
cluster/kube-up.sh

git checkout nginx
make clean; make quick-release
cluster/kube-push.sh

And validate that the cluster came up ok in both cases.

@ArtfulCoder nginx disabled by salt.

ptal.
THanks
--brendan

@brendandburns
Copy link
Contributor Author

Fixed after problems pointed out by @ArtfulCoder over IM.

@ArtfulCoder
Copy link
Contributor

LGTM

1 similar comment
@dchen1107
Copy link
Member

LGTM

@ArtfulCoder ArtfulCoder added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 2, 2015
ArtfulCoder added a commit that referenced this pull request Apr 2, 2015
@ArtfulCoder
Merge pull request #6334 from brendandburns/nginx
a918a71 
Add an nginx docker image for use on the master.
@ArtfulCoder ArtfulCoder merged commit a918a71 into kubernetes:master Apr 2, 2015
@dchen1107
Copy link
Member

Great! one more step move forward for #5011.

@brendandburns brendandburns mentioned this pull request Apr 3, 2015
Closed
@ArtfulCoder
Copy link
Contributor

@brendanburns can we roll this out for other cloud providers too ?

@ArtfulCoder
Copy link
Contributor

#6618

never mind.
we are getting rid of nginx

@brendandburns brendandburns deleted the nginx branch August 7, 2015 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@brendandburns @ArtfulCoder @dchen1107 @roberthbailey @googlebot