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

Don't use nginx image for the Pod example #10618

Merged
merged 1 commit into from
Aug 6, 2018

Conversation

mburke5678
Copy link
Contributor

@mburke5678 mburke5678 commented Jul 3, 2018

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 3, 2018
@@ -596,7 +596,7 @@ metadata:
spec:
containers:
- name: myfrontend
image: dockerfile/nginx
image: dockerfile/redis
Copy link

Choose a reason for hiding this comment

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

let's not change this one. it doesn't run with dockerfile/nginx anyway and the volumeMounts below don't make sense with redis.

ports:
- name: web
containerPort: 80
volumeMounts:
- name: gluster-vol1
mountPath: /usr/share/nginx/html
mountPath: /usr/share/hello-openshift/html
Copy link

Choose a reason for hiding this comment

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

this volumeMount doesn't make sense for hello-openshift. might leave this one as-is since the volumeMount is needed to demonstrate using the gluster volume.

- name: gluster-nginx-priv
image: fedora/nginx
- name: gluster-S3-priv
image: fedora/S3
Copy link

Choose a reason for hiding this comment

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

not sure what the mechanism of this image are

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mdshuai Any thoughts on this change ^^

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we have an image with this name.

# docker pull fedora/nginx
Using default tag: latest
Trying to pull repository registry.access.redhat.com/fedora/nginx ...
Trying to pull repository registry.reg-aws.openshift.com:443/fedora/nginx ...
Pulling repository registry.reg-aws.openshift.com:443/fedora/nginx
Trying to pull repository docker.io/fedora/nginx ...
latest: Pulling from docker.io/fedora/nginx
......

# docker pull fedora/S3
Error parsing reference: "fedora/S3" is not a valid repository/tag

# docker pull fedora/s3
Using default tag: latest
Trying to pull repository registry.access.redhat.com/fedora/s3 ...
Trying to pull repository registry.reg-aws.openshift.com:443/fedora/s3 ...
Pulling repository registry.reg-aws.openshift.com:443/fedora/s3
Trying to pull repository docker.io/fedora/s3 ...
Pulling repository docker.io/fedora/s3
Error: image fedora/s3:latest not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liangxia @sjenning
Can we leave the original gluster-nginx-priv here as the section specifically requires "A user with the cluster-admin role binding." Does this cover the super-user privileges that was the original concern of the submitter, @nak3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK to revert this specific one.

@mburke5678
Copy link
Contributor Author

@mdshuai PTAL

@mburke5678
Copy link
Contributor Author

@nak3 @sjenning Should this change roll back to 3.1, which the version cited in the issue originally?

@sjenning
Copy link

i don't see the harm in it if it isn't too much trouble

@mburke5678
Copy link
Contributor Author

@mdshuai PTAL

@mdshuai
Copy link

mdshuai commented Aug 1, 2018

@liangxia help storage example

@liangxia
Copy link
Member

liangxia commented Aug 6, 2018

$ oc exec -ti nginx-pod /bin/sh
$ cd /usr/share/nginx/html
$ oc exec -ti hello-openshift-pod /bin/sh
$ cd /usr/share/hello-openshift/html
Copy link
Member

Choose a reason for hiding this comment

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

----

. Now `curl` the pod again and it should still have the same data as before.
Note that its IP address may have changed:
+
----
# curl http://10.37.0.0
Hello World from GlusterFS!!!
Hello OpenShift!!!
Copy link
Member

Choose a reason for hiding this comment

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

The string coming from https://github.com/openshift/openshift-docs/pull/10618/files#diff-492418d8b4b3aa3ae12668611e127d51R55
They should be sync. Update both of them or none of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mburke5678, is this the right string?

@@ -98,5 +98,5 @@ $ cd /var/lib/heketi/mounts/vg_f92e09091f6b20ab12b02a2513e4ed90/brick_d8c06e606f
$ ls
index.html
$ cat index.html
Hello World from GlusterFS!!!
Hello OpenShift!!!
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

- name: nginx-nfs-pod
image: fedora/nginx <2>
- name: hello-openshift-nfs-pod
image: fedora/S3 <2>
Copy link
Member

Choose a reason for hiding this comment

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

We do not have image fedora/S3. Perhaps use openshift/hello-openshift here.

ports:
- name: web
containerPort: 80
volumeMounts:
- name: nfsvol <3>
mountPath: /usr/share/nginx/html <4>
mountPath: /usr/share/S3/html <4>
Copy link
Member

Choose a reason for hiding this comment

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

This is a path for web which should not be updated.

ports:
- containerPort: 80
volumeMounts:
- name: workdir
mountPath: /usr/share/nginx/html
mountPath: /usr/share/S3/html
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 6, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 6, 2018
@mburke5678 mburke5678 merged commit b22495c into openshift:master Aug 6, 2018
@mburke5678
Copy link
Contributor Author

/cherrypick enterprise-3.10

@openshift-cherrypick-robot

@mburke5678: new pull request created: #11397

In response to this:

/cherrypick enterprise-3.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mburke5678
Copy link
Contributor Author

/cherrypick enterprise-3.11

@openshift-cherrypick-robot

@mburke5678: new pull request created: #11398

In response to this:

/cherrypick enterprise-3.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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.

9 participants