-
Notifications
You must be signed in to change notification settings - Fork 380
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
Revise Multi-cluster Gateway descriptions in user docs #3899
Conversation
b7dfe06
to
02c0bf6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, two nits:
docs/multicluster/user-guide.md
Outdated
its Multi-cluster Gateway. Multi-cluster Service traffic are routed among clusters | ||
through the tunnels between Gateways. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra empty line?
docs/multicluster/user-guide.md
Outdated
there are several possibilities: | ||
|
||
* By default, the K8s Node's `InternalIP` is used as `gatewayIP` too. | ||
* You can choose to use the K8ds Node's `ExternalIP` as `gatewayIP`, by changing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* You can choose to use the K8ds Node's `ExternalIP` as `gatewayIP`, by changing | |
* You can choose to use the K8s Node's `ExternalIP` as `gatewayIP`, by changing |
docs/multicluster/user-guide.md
Outdated
Gateways. | ||
remote Gateways of other member clusters. Multi-cluster Controller discovers the | ||
IP addresses from the K8s Node resource of the Gateway Node. It will always use | ||
use `InternalIP` of the K8s Node as the Gateway's `internalIP`. For `gatewayIP`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra use
Codecov Report
@@ Coverage Diff @@
## main #3899 +/- ##
==========================================
+ Coverage 35.16% 35.18% +0.01%
==========================================
Files 122 122
Lines 17123 17123
==========================================
+ Hits 6022 6024 +2
+ Misses 10602 10601 -1
+ Partials 499 498 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
02c0bf6
to
3ff19be
Compare
3ff19be
to
44ff64e
Compare
@luolanzone : I made a few more revisions, and added description about encap mode and tunnel type. |
44ff64e
to
8a5db87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, two nits:
exported network information. For example, in cluster `test-cluster-west`, you | ||
you can see a `ClusterInfoImport` CR with name `test-cluster-east-clusterinfo` | ||
is created for cluster `test-cluster-east`. You can check the | ||
`ClusterInfoImport` with command: `kubectl get clusterinfoimport -o yaml`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`ClusterInfoImport` with command: `kubectl get clusterinfoimport -o yaml`, | |
`ClusterInfoImport` with command: `kubectl get clusterinfoimport test-cluster-east-clusterinfo -o yaml`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel no need to specify name here. Anyway readers can understand they can add name if they want. In most cases, they will not, to list all clusters.
8a5db87
to
4df2049
Compare
docs/multicluster/user-guide.md
Outdated
the ClusterNetworkPolicy spec they wish to be replicated. The ResourceExport should be created in the | ||
Namespace which implements the Common Area of the ClusterSet. In future releases, some additional tooling | ||
may become available to automate the creation of such ResourceExport and make ACNP replication easier. | ||
To achieve such ACNP replication across clusters, admins can, in the acting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this section to remove "Common Area" as it is never mentioned in this doc.
in leader cluster will create two `ResourceImport` contains all exported Service and | ||
Endpoints'. you can check the created resources in the leader cluster which should be | ||
like below: | ||
For example, once you export the `default/nginx` Service in member cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revised this section to remove "Multi-cluster Controller", etc. and some internal behaviors, as no need to talk about design in user guide.
Service from `test-cluster-east` will be routed to the backend `nginx` Pods in | ||
`test-cluster-west` through the Multi-cluster Gateways. | ||
|
||
As part of the Service export/import process, in the leader cluster, two |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel no need to talk about this internal logic in user doc, but anyway we can keep it.
Then you can go to the member cluster `test-cluster-east` to check the new created | ||
Service and Endpoints with name `kube-system/antrea-mc-nginx` and a ServiceImport named | ||
`kube-system/nginx`. If there is already an existing Service created by users in the | ||
member cluster `test-cluster-east` also named `nginx` in Namespace `kube-system`, which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean a Service with name antrea-mc-nginx
? It is rather uncommon. Guess no need to mention that.
For example, once you export the `default/nginx` Service in member cluster | ||
`test-cluster-west`, it will be exported to member cluster `test-cluster-east` | ||
through the leader cluster. A Service and an Endpoints with name | ||
`default/antrea-mc-nginx` will be created in `test-cluster-east`, as well as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed Namespace to "default", as not common to create Services in kube-system. Could you check?
docs/multicluster/user-guide.md
Outdated
multi-cluster Service resources will be updated accordingly. A few cases below | ||
are worth to note: | ||
|
||
1. When there are more than one ServiceExports created for a single Service, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to revise the description (as hard to understand the previous version). Could you check?
19267b0
to
3516019
Compare
docs/multicluster/user-guide.md
Outdated
like below: | ||
For example, once you export the `default/nginx` Service in member cluster | ||
`test-cluster-west`, it will be exported to member cluster `test-cluster-east` | ||
through the leader cluster. A Service and an Endpoints with name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
through the leader cluster. A Service and an Endpoints with name | |
through the leader cluster. A Service and an Endpoint with name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resource kind is indeed Endpoints
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, we can keep it, I am actually confused with this one since I saw both an Endpoints
and an Endpoint
in our docs and comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I already removed all "Endpoint" or changed to "endpoint".
docs/multicluster/user-guide.md
Outdated
Service resources will be updated accordingly. The two cases below are worth to | ||
note: | ||
|
||
1. When there are more than one ServiceExports created for a single Service, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. When there are more than one ServiceExports created for a single Service, the | |
1. When there are more than one ServiceExports from different member clusters created for a single Service, the |
docs/multicluster/user-guide.md
Outdated
|
||
1. When there are more than one ServiceExports created for a single Service, the | ||
Service might not be exported correctly, and updates to the Service will not | ||
be handled, until users remove the redundant ServiceExports. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be handled, until users remove the redundant ServiceExports. | |
be handled, until users remove the redundant ServiceExports or correct the Service to make its definition consistent in each member cluster. |
docs/multicluster/user-guide.md
Outdated
creation of ResourceExports for ACNPs, and provide a user-friendly way to define | ||
Multi-cluster NetworkPolicies to be enforced in the ClusterSet. | ||
|
||
replication easier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed
Revise the descriptions about Multi-cluster Gateway configuration. Add descriptions about the `gateway-ip` annotation. Remove unnecessary descriptions about implementations from the user documents. Signed-off-by: Jianjun Shen <[email protected]>
3516019
to
dc1df80
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/skip-all |
Revise the descriptions about Multi-cluster Gateway configuration.
Add descriptions about the
gateway-ip
annotation.Remove unnecessary descriptions about implementations from the user
documents.
Signed-off-by: Jianjun Shen [email protected]