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

docs: various fixes to documentation, notably Getting Started Guides #16126

Merged
merged 3 commits into from May 25, 2021

Conversation

nbusseneau
Copy link
Member

@nbusseneau nbusseneau commented May 12, 2021

Please see individual commits. I suggest reviewing commits one by one: the last one is messy and is better served reviewed on its own.

@nbusseneau nbusseneau added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.10 labels May 12, 2021
@nbusseneau nbusseneau requested review from a team as code owners May 12, 2021 20:53
@nbusseneau nbusseneau requested a review from qmonnet May 12, 2021 20:53
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.0-rc2 May 12, 2021
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

A few nits below, but these look like nice improvements in my opinion, thanks!

Documentation/gettingstarted/install-cli.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/cli-status.rst Show resolved Hide resolved
Documentation/gettingstarted/k8s-install-openshift-okd.rst Outdated Show resolved Hide resolved
@@ -346,9 +346,3 @@ Troubleshooting
$ tc filter show dev eth0 ingress
filter protocol all pref 1 bpf chain 0
filter protocol all pref 1 bpf chain 0 handle 0x1 bpf_network.o:[from-network] direct-action not_in_hw id 1145 tag 51b408acf94aa23f jited

Disabling Encryption
Copy link
Member

Choose a reason for hiding this comment

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

Could you please open an issue so we can track this and add it back when the time is come?

Copy link
Contributor

Choose a reason for hiding this comment

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

NAK from me. Lets create an issue and fix it. The documentation has been this way from day 1 also when this happens is a specific to kernel versions and modes so correctly describing the conditions would require rework.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK! I am removing the commit introducing the change. Do we still need to create an issue for tracking this or will #15993 be sufficient?

NAME READY STATUS RESTARTS AGE
pod/mediabot 1/1 Running 0 14s
kubectl create -f \ |SCM_WEB|\/examples/kubernetes-dns/dns-sw-app.yaml
kubectl wait pod/mediabot --for=condition=Ready
Copy link
Member

Choose a reason for hiding this comment

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

[For what it's worth, I'm not entirely convinced about removing the prompts, as some processing will still be needed after copying to remove the output from the commands. I'm working on adding a new button to copy just the commands starting with the prompt symbol, but excluding the prompt from the copy itself. But this is not directly relevant to this PR, so OK for the change.]

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this won't work using the "copy all" button anyhow. The prompt removal is more so that one may copy/paste by highlighting a specific line: at the moment it also copy/pastes the $.
image

Documentation/gettingstarted/dns.rst Show resolved Hide resolved
Copy link
Member

@rolinh rolinh left a comment

Choose a reason for hiding this comment

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

Pretty nice overhaul 👍 , just one comment see below.
Btw, this makes me wonder whether we should not use exactly the same instructions for Cilium CLI and Hubble CLI?

Documentation/gettingstarted/cli-download.rst Outdated Show resolved Hide resolved
@aanm aanm added this to Needs backport from master in 1.10.0-rc3 May 17, 2021
@aanm aanm removed this from Needs backport from master in 1.10.0-rc2 May 17, 2021
@nbusseneau nbusseneau requested review from a team and glibsm May 17, 2021 14:47
Copy link
Contributor

@jrfastab jrfastab left a comment

Choose a reason for hiding this comment

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

Drop the disable encryption parts lets handle this as an issue.

@jrfastab
Copy link
Contributor

Did some poking around. More context for my above one liner ;). So the reason, as I understand it, to drop the encryption disable piece is to cover the issue where we add ARP PERM entries but weren't removing them. This had the potential to break a network on the next restart with IPSec disabled due to stale arp entries that would never be removed. This fix should cover the case, #15993 so I think we are safe to keep the encryption disable instructions. Thanks!

@jrfastab
Copy link
Contributor

jrfastab commented May 19, 2021

We currently still leave some stale state in our encryption routing table, ip r s t 200 but we do delete the ip rule link to it and its also guarded by mark bits set from IPSec BPF case so it should be harmless.

@aanm aanm added this to Needs backport from master in 1.10.1 May 19, 2021
@aanm aanm removed this from Needs backport from master in 1.10.0-rc3 May 19, 2021
@@ -140,4 +140,4 @@ installed:
* ``10.2.2.0/24 dev tun-172011760 proto 17 src 172.0.50.227``
* ``10.2.3.0/24 dev tun-1720186231 proto 17 src 172.0.50.227``

.. include:: k8s-install-connectivity-test.rst
.. include:: k8s-install-validate.rst
Copy link
Member Author

Choose a reason for hiding this comment

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

To reviewers: will kube-router allow for usage of Cilium CLI? If not, then we should revert to manually linking to

Suggested change
.. include:: k8s-install-validate.rst
.. include:: kubectl-connectivity-test.rst

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

A few more nits, but looks good overall.
[I don't know for kops/kube-router.]

Documentation/gettingstarted/hubble-install.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/kubectl-status.rst Outdated Show resolved Hide resolved
Also refactor hubble-cli installation accordingly.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
`parsed-literal` blocks are required for parsing RST references but they
don't play nice with console examples: they allow copy-pasting `$`
prefixes even though they're not part of the commands.

`shell-session` blocks play nice with console examples but do not work
with RST references...

This is compromise where we remove `$` prefixes from parsed-literal
blocks for easier copy/pasting and switch to proper `shell-session`
blocks otherwise.

Also reworked the `curl` commands to add `--max-time` for commands
supposed to fail so that user doesn't have to cancal, and switch to
`curl -I {url} | head -1` notation to avoid output flood.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
In cilium#15979, the old `k8s-install-validate.rst` and `k8s-install-connectivity-test.rst`
were refactored to use the CLI, which broke the flow of several pages:
in particular, all installations based on Helm were half-broken due to
referencing Cilium CLI commands when the user was never instructed to
install it.

This commit moves all CLI-related operations to independent `cli-*.rst`,
and then refactors `k8s-install-validate.rst` to have both the new CLI
status check and connectivity test and the older manual status check and
connectivity test.

It then refactors CLI-based installation guides to use the `cli-*.rst`
in the order that makes the most sense for each page.

Signed-off-by: Nicolas Busseneau <nicolas@isovalent.com>
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@nbusseneau nbusseneau closed this May 21, 2021
@nbusseneau nbusseneau deleted the pr/gsg-1.10-various-fixes branch May 21, 2021 21:54
@nbusseneau nbusseneau restored the pr/gsg-1.10-various-fixes branch May 21, 2021 22:00
@nbusseneau
Copy link
Member Author

Sorry, I made a mistake with my fork and accidentally closed all my PRs. Reopening.

@nbusseneau nbusseneau reopened this May 21, 2021
@nbusseneau
Copy link
Member Author

Let's merge this and not block on kube-router.

@nbusseneau nbusseneau added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 24, 2021
@twpayne twpayne merged commit 497ac33 into cilium:master May 25, 2021
@qmonnet qmonnet mentioned this pull request Jun 1, 2021
23 tasks
@aanm aanm moved this from Needs backport from master to Backport done to v1.10 in 1.10.1 Jun 16, 2021
@nbusseneau nbusseneau deleted the pr/gsg-1.10-various-fixes branch July 5, 2021 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.10.0
Awaiting triage
1.10.1
Backport done to v1.10
Development

Successfully merging this pull request may close these issues.

None yet

10 participants