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

WIP Network operator #14084

Closed
wants to merge 1 commit into from
Closed

WIP Network operator #14084

wants to merge 1 commit into from

Conversation

ariordan-redhat
Copy link
Contributor

@openshift/team-documentation For peer review, please.

Added network operator file to _topic_map
@ariordan-redhat ariordan-redhat added peer-review-needed Signifies that the peer review team needs to review this PR branch/enterprise-4.1 labels Mar 13, 2019
@ariordan-redhat ariordan-redhat added this to the Future Release milestone Mar 13, 2019
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 13, 2019

//include::modules/configuration-resource-overview.adoc[leveloffset=+1]
//
//include::modules/networking-cluster-configuration.adoc[leveloffset=+1]
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason, even though this is commented out, the build is trying to include it. @ariordan-redhat did you want to take it out?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you need a basic "networking in OCP" module to start this topic off.

@vikram-redhat
Copy link
Contributor

@ariordan-redhat if these PRs have a jira or bug attached, add that as part of the title/comment.

@kalexand-rh kalexand-rh self-requested a review March 13, 2019 14:49
Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

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

I think that this assembly needs to be two assemblies, one about installing a cluster with network customizations and one about modifying the networking settings after installation.

The installation assembly should go in the installing/installing_aws folder and also incorporate all of the modules from https://docs.openshift.com/container-platform/4.0/installing/installing_aws/installing-aws-customizations.html. When the user installs a cluster, they'll only use this new assembly, not both the networking and the installation assemblies.

Please let me know if you have questions, want to work on the structure of the new assembly, or want another review of the updated PR. (There are some spots in the current installation assemblies that will need to change based on the configuration that you're adding.)

@@ -265,8 +268,8 @@ Distros: openshift-*
Topics:
- Name: Understanding networking
File: understanding-networking
- Name: Using cookies to keep route statefulness
File: using-cookies-to-keep-route-statefulness
- Name: Configuring the Network before starting a cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

Should Network be capitalized here?
Update this reference to match the assembly title.

// * networking/configuring-network-operator.adoc

[id='modifying-network-config-startup-{context}']
= Modifying Basic Network Configuration before Starting 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.

All headings need to be sentence case.

[id='modifying-network-config-startup-{context}']
= Modifying Basic Network Configuration before Starting the Cluster

You can modify the basic network configuration parameters in the `Network.config.openshift.io` object before you start 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.

can or must?


.Procedure

. Use the following command to create an `install-config-yaml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this is the right approach to this use case. I think that if you have to set the networking parameters before installation that it needs to be part of a customized installation flow. These parameters are already in https://docs.openshift.com/container-platform/4.0/installing/installing_aws/installing-aws-customizations.html#installation-aws-config-yaml-install-customizations-cloud, and if a user wants to customize networking, we should frame it as an installation task.


.Procedure

. Use the following command to create `manifests`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. The information that I have is that modifying the manifests isn't supported. https://docs.openshift.com/container-platform/4.0/installing/installing_aws/installing-aws-customizations.html#cloud-installations-install-customizations-cloud

It is possible to modify Kubernetes and the Ignition Configs that control the underlying Red Hat Enterprise Linux CoreOS (RHCOS) operating system during installation. However, no validation is available to confirm the suitability of any modifications that you make to these objects. If you modify these objects, you might render your cluster non-functional. Because of this risk, modifying Kubernetes and Ignition Configs is not supported.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@jboxman, as I said, the last I heard was that it was not supported. Here's where QE was discussing it: #13468 (comment)

If it is supported, that's fine, but we need to update the installation docs and get QE to confirm them.

You can only modify `kubeProxy` configuration parameters in
a running cluster.

The Network Operator reconciles the state of 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.

Is this helpful here?


.Prerequisites

* Login to a running cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

Log in

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to have the oc cli installed.

$ oc edit networkconfig
----
+
The example output below shows a `NetworkConfig.networkoperator.openshift.io` CR
Copy link
Contributor

Choose a reason for hiding this comment

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

not below.

+
[source,yaml]
----
spec:
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure that the new assembly contains sufficient information about the parameter values.

. Save the file and quit the text editor.
+
The Network Operator checks the syntax when you quit the file. If your
modifications contain a syntax error, the editor will re-open the file, and display an error
Copy link
Contributor

Choose a reason for hiding this comment

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

s/file,/file

@kalexand-rh kalexand-rh added peer-review-done Signifies that the peer review team has reviewed this PR and removed peer-review-needed Signifies that the peer review team needs to review this PR labels Mar 13, 2019
@@ -0,0 +1,60 @@
// Assembly filename: configuring-network-operator.adoc
// Explains network operator, shows default CRDs and modifications to setup.
[id='configuring-network-operator’]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there's a backtick here, too.

include::modules/nw-nwop-modifying-config-running.adoc[leveloffset=+1]
//
//Full operator config. Network operator uses 2 CRDs, so separated them into 2 docs.
//include::modules/network-annotated-crd.adoc[leveloffset=+1]
Copy link
Contributor

Choose a reason for hiding this comment

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

For each of the commented-out include statements, try replacing "::" with " ". That seems to make Travis happy.

@openshift-ci-robot
Copy link

@ariordan-redhat: PR needs rebase.

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 15, 2019
@jboxman jboxman changed the title Network operator WIP Network operator Mar 25, 2019
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 25, 2019
@jboxman
Copy link
Contributor

jboxman commented Mar 25, 2019

Shifting working copy to PR #14224. All fixes will be applied there.

@jboxman jboxman removed this from the Future Release milestone Mar 26, 2019
@kalexand-rh
Copy link
Contributor

Closing since there's a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. peer-review-done Signifies that the peer review team has reviewed this PR size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants