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: Update OpenShift (OKD) GSG to use OLM operator #15608

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

errordeveloper
Copy link
Contributor

No description provided.

@errordeveloper errordeveloper added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 8, 2021
@errordeveloper errordeveloper requested a review from a team as a code owner April 8, 2021 14:46
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Apr 8, 2021
Documentation/conf.py Outdated Show resolved Hide resolved
@errordeveloper errordeveloper force-pushed the update-okd-docs-use-operator branch 2 times, most recently from b1cf7a8 to a60fcb7 Compare April 12, 2021 09:27
Documentation/conf.py Outdated Show resolved Hide resolved
Documentation/gettingstarted/k8s-install-openshift-okd.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/k8s-install-openshift-okd.rst Outdated Show resolved Hide resolved
Documentation/gettingstarted/k8s-install-openshift-okd.rst Outdated Show resolved Hide resolved
@maintainer-s-little-helper

This comment has been minimized.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 13, 2021
Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 13, 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.

Thanks, looks good on my side

@qmonnet qmonnet removed their assignment Apr 13, 2021
@errordeveloper errordeveloper added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 13, 2021
@qmonnet qmonnet requested a review from aanm April 13, 2021 14:38
@errordeveloper
Copy link
Contributor Author

Just realised that this needed for 1.10, so added the appropriate label.

@qmonnet
Copy link
Member

qmonnet commented Apr 14, 2021

FYI I took note of the ready-to-merge, but I wanted to give @aanm a chance to see your answer and (possibly) reply before I merge the PR.

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

@errordeveloper We should discuss the UX around this installation overall as running bash commands instead of using cilium-cli or helm seems a little bit laborious.

Comment on lines +127 to +128
curl --silent --location --fail --show-error "https://github.com/cilium/cilium-olm/archive/${cilium_olm_rev}.tar.gz" --output /tmp/cilium-olm.tgz
tar -C /tmp -xf /tmp/cilium-olm.tgz
Copy link
Member

Choose a reason for hiding this comment

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

For the main Cilium instructions, we have had instructions like this for developers (ie latest docs only, not for the branch docs). Is that the case here? Is it normal to use instructions like this for a plugin install into a OpenShift environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is mostly normal for installing software into OpenShift during cluster bootstrap. At least one other networking "plugin" does a very similar thing. The other way is to install an installer operator, but for Cilium it would imply network migration, which is best to avoid for the purpose of this guide.
User needs to pass manifests during cluster bootstrap for the operator that installs Cilium, the most convent way to handle this was to check rendered manifest in the cilium-olm repo. What is being installed is an operator (for now we are using RedHat's Helm operator. Going forward this will become our own installer operator, and copying of manifests from a repo maybe then replaced with some invocation of new Cilium CLI that generates them.
The reason master branch is referenced here is just to avoid having to update the docs each time there is a relevant change, in production user might choose to pin to a commit. The cilium-olm repo doesn't not have release branches, but it does contain manifests for most of 1.8.x releases, all of 1.9.x release and all others going forward (until we move away from RedHat's operator implementation). There is more nuance to this, but that's the idea overall.

/cc @aanm as this might address some of your concerns also.

Comment on lines +139 to +147
.. note::

If you wish to set any custom Helm values, you can do it by editing ``${CLUSTER_NAME}/manifests/cluster-network-07-cilium-ciliumconfig.yaml``.

It's also possible to update Helm values once the cluster is running by changing ``CiliumConfig`` object,
e.g. with ``kubectl edit ciliumconfig -n cilium cilium``. You may need to restart Cilium agent pods for
certain options to take effect.

.. note::
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to call these out in indented note:: sections? Too many callouts will force users to ignore everything else. In general I would prefer to use these sparingly, and just write everything that the user may need to know directly in regular text (unless there's some exceptional circumstance that we really must highlight to users).

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 do generally agree, but some of the other callouts are really trying to address some nuances of different modes of installation, like cloud provider auths quirks that have little to do with Cilium, yet are gaps in OpenShift installer UX that I felt we should share with the users. This one in particular is kind of in the same territory, albeit a little more connected to Cilium, but it is mostly optional. Generally, I am quite sure that skipping most of the callouts will be fine for a lucky user who have installed OpenShift before.
We can review these more closely, but as I said in the other comment, the world of OpenShift torn with UX hindrances, and I don't think it's worse fixing. There are aspects worth addressing, but probably best to have separate discussions. Namely, we need to look at how to fix the issue with ports and drop hacky scripts that this guide currently asks user to run (exposing a flag to set VXLAN port would help there), and moving to our own operator is another aspect that can be addressed in 1.11.

@qmonnet qmonnet removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 14, 2021
@errordeveloper
Copy link
Contributor Author

@errordeveloper We should discuss the UX around this installation overall as running bash commands instead of using cilium-cli or helm seems a little bit laborious.

@aanm yes, but there are several BUTs 😉

What is needed here is manifests, there is no API access at this point. cilium-cli assumes API access. Helm was used previously, but now we are not installing Cilium components, just the installer operator. Previous version used helm, and it was not less complex.

Additionally, there are many more UX hindrances that OpenShift installation has, this is honestly not one of them.

I am happy to discuss some improvements for 1.11, but we need to get this into 1.10 docs to start using the certified operator.

@aanm aanm added this to the 1.10.0-rc.1 milestone Apr 16, 2021
@qmonnet qmonnet merged commit 95fe1bf into cilium:master Apr 16, 2021
1.10.0 automation moved this from In progress to Done Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants