Skip to content

Conversation

@steveej
Copy link
Contributor

@steveej steveej commented Sep 20, 2018

This also improves the general structure a bit to decrease the likelihood of confusion.

Potentially conflicts with doc changes in #296.

Fixes #311.

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 20, 2018
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS 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 Sep 20, 2018
Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Hooray, docs :). A few nits inline.

Copy link
Member

Choose a reason for hiding this comment

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

Are we ok with dropping "limited" 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.

We could say that we support libvirt on Linux

Copy link
Member

Choose a reason for hiding this comment

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

You want to drop the .gz suffix from the final filename.

You also want a single space before the pipe (you currently have two).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks ;-)

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't render correctly (you can see the sh). What you want is a traditional fenced block with four-space indents:

  • Tell dnsmasq...

    For this example:

    echo server=/tt.testing/192.168.124.1 | sudo tee /etc/NetworkManager/dnsmasq.d/tectonic.conf

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, I misread that command to invoke sh, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

"... hasn't quite worked, please..." (with a comma).

Copy link
Member

Choose a reason for hiding this comment

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

"you're" -> "your"

Copy link
Member

Choose a reason for hiding this comment

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

nit: I'd rather have the URL on the same line as the reference, but GitHub seems to render this correctly, so I'm ok with you deciding to wrap it like this if you feel strongly ;).

Copy link
Member

Choose a reason for hiding this comment

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

Why the blank line?

nit: I like collating these entries by anchor (e.g. highlight them all and use sort-lines in Emacs ;), because then I don't have to think about where to insert new 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.

very helpful, thanks

@crawford
Copy link
Contributor

Why are e2e tests running on this PR? It's entirely documentation.

@steveej
Copy link
Contributor Author

steveej commented Sep 21, 2018

Why are e2e tests running on this PR? It's entirely documentation.

I had the same thought when I created this PR. @sallyom can you answer that?

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Sep 21, 2018

@crawford @steveej ci/prow/e2e-aws — Skipped ci/prow/e2e-aws-smoke — Skipped they don't seem to be running

Choose a reason for hiding this comment

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

There was a step in @sjenning first screencast instructing how to set enforcing=0 in the qcow image with virt-edit, if that's still necessary we should add that here.

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 was about to test it again with that image but the URL doesn't work anymore. Has it changed?

Copy link
Member

Choose a reason for hiding this comment

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

works for me.. are you still on the vpn?

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 don't remember being on the VPN for getting that image. Is there a public mirror?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a public mirror?

No, although I don't know what the reasoning is.

Copy link
Contributor

Choose a reason for hiding this comment

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

There was a step in @sjenning first screencast instructing how to set enforcing=0 in the qcow image with virt-edit, if that's still necessary we should add that here.

It is not necessary anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

did you meant to be at level 4 instead of 5 here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Another command to note would be $ virsh --connect qemu:///system net-dumpxml default to see how the default network has been configured

Copy link
Contributor

Choose a reason for hiding this comment

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

put my content from https://github.com/openshift/installer/pull/323/files in here please. it's all still entirely relevant.

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, thanks!

@bparees
Copy link
Contributor

bparees commented Sep 25, 2018

can you add something about tearing down the cluster?

Copy link
Contributor

Choose a reason for hiding this comment

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

needs doc on where to get this file. Also @derekwaynecarr showed me it's easier to point to a path than to try to inline the file:
pullSecretPath: "/home/bparees/git/gocode/src/github.com/openshift/installer/files/config.json"

Copy link
Contributor

Choose a reason for hiding this comment

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

(the reason i say we need doc on where to get this file is i was told not to get it from coreos.com (and i ran into problems trying to do so).

Copy link
Member

Choose a reason for hiding this comment

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

pullSecretPath: "/home/bparees/git/gocode/src/github.com/openshift/installer/files/config.json"

This is deprecated. Once we get the new installer and #320, you'll be able to use OPENSHIFT_INSTALL_PULL_SECRET="$(cat path/to/your/secret)".

@bparees
Copy link
Contributor

bparees commented Sep 25, 2018

can you add something about tearing down the cluster?

nm, i see it's there (tectonic destroy)

@steveej
Copy link
Contributor Author

steveej commented Sep 26, 2018

I'm not sure what we're waiting for here. Did I leave any comments unaddressed?

@ashcrow
Copy link
Member

ashcrow commented Sep 26, 2018

@crawford do the updates look good?

Copy link
Member

Choose a reason for hiding this comment

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

Nit, missing a trailing backtick here.

@wking wking dismissed crawford’s stale review September 26, 2018 21:12

All of his requested changes have been addressed.

Since we're instructing to use 192.168.122.1 for the libvirt URI, which
is apparently what's used by the clusterapi-controller to talk to
libvirt, the firewall has to match, otherwise it looks likt this in the
logs:

```
E0924 21:26:08.925983       1 controller.go:115] Error checking
existance of machine instance for machine object worker-fdtdg; Failed to
build libvirt client: virError(Code=38, Domain=7, Message='unable to
connect to server at '192.168.122.1:16509': Connection timed out')
```
@wking
Copy link
Member

wking commented Sep 26, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 26, 2018
@steveej
Copy link
Contributor Author

steveej commented Sep 26, 2018

/lgtm

You're gonna have to go again, fixed another typo

@wking
Copy link
Member

wking commented Sep 26, 2018

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: steveeJ, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 22f8f5f into openshift:master Sep 26, 2018
@steveej steveej deleted the docs-libvirt-troubleshooting branch September 26, 2018 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.