Skip to content

Conversation

@tzumainn
Copy link
Contributor

@tzumainn tzumainn commented Mar 6, 2018

No description provided.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. retest-not-required-docs-only needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 6, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 6, 2018

@tomassedovic This does not yet contain any content updates; just a re-organization to see if it works for you. Besides the additions we talked about, I have the following comments. Let me know what you think!

  • can we remove flannel references?
  • do we actually provide load balancer servers? pretty sure no
  • access control - do we link to anything useful?
  • get rid of 'TODOs' mentioned in README.md under 'Advanced Configuration'?
  • manage_packages variable - doesn't exist in openshift-ansible?
  • break off post-install section into its own document?
  • DNS and cinder sections are untouched, may benefit from a cleaning
  • openshift_master_default_subdomain - not sure what the doc is trying to say

@tomassedovic
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 6, 2018
Copy link
Contributor

@tomassedovic tomassedovic left a comment

Choose a reason for hiding this comment

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

Thanks! I like the direction you've taken with it, with the slight exception of some of the optiona post-deployment stuff.

I've only done a rough read through so there's going to be more coming later, but I've pointed a few things inline.

Regarding your questions:

  • yes, let's remove flannel for now
  • we kinda sorta do provide lb servers in the case of more than one master. It's okay to drop that and I'll add the docs in #7209
  • wrt access control we can link this doc: https://docs.openshift.org/latest/install_config/configuring_authentication.html
  • yea manage_packages can be removed
  • yes please, let's move post-install to its own doc -- or at least the optional bicts
  • we can address dns, cinder and openshift_master_defaultt_subdomain in followup patches. If you want to do it here, I can guide you

for production
* The keypair for SSH must be available in OpenStack
* `keystonerc` file that lets you talk to the OpenStack services
* NOTE: only Keystone V2 is currently supported
Copy link
Contributor

Choose a reason for hiding this comment

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

This is no longer true. I've been using Keystone v3 for a couple of months now.

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!

* `keystonerc` file that lets you talk to the OpenStack services
* NOTE: only Keystone V2 is currently supported

You may also optionally want:
Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, it is technically optional, but I'd word it a little more strongly perhaps? Something like "you'll very likely also want these, but they're not strictly speaking necessary" but more concise.

If you can't figure out a way to say that nicely, just leave what you've got.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 'Strongly recommended'

ansible-playbook -i <inventory> openshift-ansible-contrib/playbooks/provisioning/openstack/custom-actions/add-cas.yml --extra-vars '{"ca_files": [<absolute path to ca1 file>, <absolute path to ca2 file>]}'
```
Please consider contributing your custom playbook back to openshift-ansible-contrib!
Copy link
Contributor

Choose a reason for hiding this comment

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

it's openshift-ansible now

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. Should we move the custom playbooks that are all still in openshift-ansible-contrib over here at some point?

### Logging in Using the Command Line

```
oc login --insecure-skip-tls-verify=true https://master-0.openshift.example.com:8443 -u user -p password
Copy link
Contributor

Choose a reason for hiding this comment

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

This must match the value in /etc/hosts so console.openshift.example.com.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified!

You can also create your own custom playbook. Here are a few examples:
#### Adding additional YUM repositories
Copy link
Contributor

Choose a reason for hiding this comment

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

I get why the DNS, oc client, UI etc. are back in the README, but this is really something I don't expect most users to be doing.

Maybe we could add a post-deployment document and put this stuff there? If push comes to shove, we can leave it here, but I'd prefer the README to be straightforward and just showing the steps you absolutely need ("look, openshift on opesntakc is easy") and putting all the optional bits elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agree! Just wanted to make sure people were okay with the configuration doc structure before doing something similar with the post-install stuff.

* `openshift_openstack_etcd_flavor`

* `openshift_openstack_external_network_name` OpenStack network providing external connectivity.
* `openshift_openstack_private_network_name` OpenStack network providing admin/control access for Ansible. It can be merged with other
Copy link
Contributor

Choose a reason for hiding this comment

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

openshift_openstack_private_network_name is no longer being used anywhere. Feel free to delete this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

In `inventory/group_vars/OSEv3.yml`:

* `openshift_disable_check` List of checks to disable.
* `openshift_master_cluster_public_hostname` Custom entrypoint; for example, `api.openshift.example.com`. Note than an empty hostname does not work, so if your domain is `openshift.example.com` you cannot set this value to simply `openshift.example.com`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I need to figure out if we can work around that. I'd love to be able to do that.

* `openshift_openstack_provider_network_name` Provider network name. Setting this will cause the `openshift_openstack_external_network_name` and `openshift_openstack_private_network_name` parameters to be ignored.


## Security Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

We're no longer deploying a DNS and the ingress_cidr values are not super useful for anything else (we control access with security groups).

And manage_packages is no longer used either, so this entire section can go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed!

After a successful deployment, the cluster is configured for Cinder persistent
volumes.

### Validation
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be greatly simplified. No need to create volume claims or any of that any more. If you deploy any OpenShift provided template with a persistent volume, it will be connected to Cinder automatically (since OpenShift 3.7).

But if you don't want to dig around, this is fine for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay - I'll probably iterate on this after the next round of updates for this PR.

* The Cinder volume should be deleted


## Cinder-Backed Registry Configuration
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another holdout from openshift-ansible-contrib that hasn't made it into openshift-ansible yet.

This PR will add the necessary support and update the docs: #6713

So feel free to just delete this section (but keep the PV and existing registry ones!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, gotcha. I moved the existing registry documentation under this heading.

@bogdando
Copy link
Contributor

bogdando commented Mar 6, 2018

Coud you please also verify this doesn't break external references given in openshift/openshift-docs#6700 and openshift/openshift-docs#6020 ?

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 6, 2018

@bogdando As far as I can tell this change should not affect that documentation, so I think we're good!

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 6, 2018

@tomassedovic Thanks for the review! I've updated the PR to take your suggestions into account, other than the cinder/DNS cleanup and openshift_master_default_subdomain clarification. I'll work on adding a few missing items, and then do the remaining cleanup.

@tzumainn tzumainn changed the title [WIP] Re-organized OpenStack documentation Re-organized OpenStack documentation Mar 6, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 6, 2018

@tomassedovic I've made the remaining changes (in separate commits so it's easier to spot them). I think that the DNS/cinder edits may best belong in a follow-up PR to separate out any potential discussion there. Let me know what you think!

OpenShift requires two public DNS records to function fully. The first one points to
the master/load balancer and provides the UI/API access. The other one is a
wildcard domain that resolves app route requests to the infra node. A private DNS
server and records are not required and not managed here.
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally speaking, they are. Though not for the scope of that guide.

[get-the-oc-client]: ./post-install.md#get-the-oc-client
[log-in-using-the-command-line]: ./post-install.md#log-in-using-the-command-line
[access-the-ui]: ./post-install.md#access-the-ui
[scale-deployment]: ./post-install.md#scale-deployment
Copy link
Contributor

Choose a reason for hiding this comment

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

This refers to unsupported contrib repo. Let's please remove such references.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, missed this one - removed, thanks!

Copy link
Contributor

@bogdando bogdando left a comment

Choose a reason for hiding this comment

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

Well Done! LGTM, but please remove all references to unsupported contrib repo and align the external links with openshift-docs in dependency patches

@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 7, 2018

@bogdando Thanks for the review! I removed the link to the scaling actions. The only openshift-ansible-contrib links left are in the custom playbook section, and I think that's fair because it references playbooks that only exist in openshift-ansible-contrib right now. I checked the PRs you linked earlier, and I didn't see any references to 'advanced-configuration', but let me know if I missed something!

Copy link
Contributor

@tomassedovic tomassedovic left a comment

Choose a reason for hiding this comment

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

Looking good!

@tomassedovic
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2018
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2018
@tzumainn
Copy link
Contributor Author

tzumainn commented Mar 7, 2018

@tomassedovic I updated the DNS and cinder sections - those should be the last changes to this PR!

@tzumainn tzumainn force-pushed the openstack-doc-update branch from 29d5723 to 5319e50 Compare March 7, 2018 20:18
@tzumainn tzumainn force-pushed the openstack-doc-update branch from 5319e50 to 7b22f41 Compare March 8, 2018 02:28
@openshift-ci-robot
Copy link

@tzumainn: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/logging 7b22f41 link /test logging

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

@tomassedovic
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2018
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit a2f35f7 into openshift:master Mar 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm Indicates that a PR is ready to be merged. retest-not-required-docs-only size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants