-
Notifications
You must be signed in to change notification settings - Fork 386
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
Update multi-cluster user guide and templates #3853
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3853 +/- ##
==========================================
+ Coverage 64.33% 64.42% +0.08%
==========================================
Files 290 288 -2
Lines 41225 41252 +27
==========================================
+ Hits 26524 26575 +51
+ Misses 12578 12548 -30
- Partials 2123 2129 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -1,15 +1,15 @@ | |||
apiVersion: multicluster.crd.antrea.io/v1alpha1 | |||
kind: ClusterClaim | |||
metadata: | |||
name: leadercluster-id |
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.
Could we change to clusterSet.k8s.io as we talked?
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.
yes, I will change this one today.
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.
Changed it to clusterset.k8s.io
due to upper case is not allowed in metadata's name.
18b8999
to
3ab0d4f
Compare
@luolanzone : I updated the quick-start doc. The biggest change is that it now talks about two clusters, with one to be both leader and member. Could you check? I plan to revise the user guide too. |
docs/multicluster/quick-start.md
Outdated
|
||
In this quick start guide, we will set up a Antrea Multi-cluster ClusterSet with | ||
two clusters. One cluster will serve as both the leader and a member cluster, | ||
while another cluster will be a member only. The diagram belows shows the 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.
@luolanzone : I feel we better to have a simple diagram to show the two clusters (with cluster ID), and ClusterSet (with leader and member cluster information).
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.
Sure, I haven't had the chance to draw the diagram yesterday, I will add it.
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.
A few comments for the diagram:
- What you think we have a box for the ClusterSet that contains the two clusters? We can add ClusterSet ID there.
- Add something like "leader" to the leader mc-controller to indicate it is the controller for leader.
- And some way to indicate the Gateway Node too.
- Could we add apiserver IP to Cluster A
- Could we indicate Cluster A is "leader and member", and Cluster B is "member"
- We should change the Node names in the diagram and commands (e.g. node-a1/a2 and node-b1/b2)
``` | ||
|
||
## Multi-cluster Gateway Configuration |
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 moved it here to be before the feature consumption sections. But have not review and update the section yet.
@@ -503,6 +591,17 @@ Events: | |||
Warning ACNPImportFailed 2m11s resourceimport-controller ACNP Tier random does not exist in the importing cluster test-cluster-west | |||
``` | |||
|
|||
## Build Antrea Multi-cluster Image |
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.
This is not really for users. But anyway, I moved it to the end of the doc.
docs/multicluster/user-guide.md
Outdated
cluster, one in leader mode and another in member mode. There is no deployment | ||
dependency between member and leader clusters when you have a dedicated leader | ||
cluster. But to run both leader and member controllers in one cluster, you need | ||
to follow the deployment sequence below: |
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.
Is it still true? @luolanzone
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 think there is no dependency even the cluster run both leader and member if user is using 'kubectl apply', but if user is using kubectl create -f
, CRD installation will fail since some of them are already existing like ClusterClaim
.
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.
If it is true, we should remove the words around ordering.
- clusterID: test-cluster-east | ||
namespace: antrea-mcs-ns | ||
- clusterID: test-cluster-leader | ||
- clusterID: test-cluster-member |
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.
@hjiajing Could you help to review these changes in templates? I hope it has minor impact to CI, thanks.
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.
@luolanzone : please update the ObjectMeta.Name first.
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.
sure, I am working the diagram, will submit a new commit once is done.
50352f7
to
82d7a67
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.
Re-paste my comments for the diagram:
- What you think we have a box for the ClusterSet that contains the two clusters? We can add ClusterSet ID there.
- Add something like "leader" to the leader mc-controller to indicate it is the controller for leader.
- And some way to indicate the Gateway Node too.
- Could we add apiserver IP to Cluster A
- Could we indicate Cluster A is "leader and member", and Cluster B is "member"
- We should change the Node names in the diagram and commands (e.g. node-a1/a2 and node-b1/b2)
|
||
```bash | ||
$kubectl apply -f https://github.com/antrea-io/antrea/releases/download/$TAG/antrea-multicluster-member.yml | ||
$kubectl apply -f https://github.com/antrea-io/antrea/releases/download/$TAG/antrea-multicluster-leader-global.yml |
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 we need to apply antrea-multicluster-leader-global.yml for a dual-role cluster? In the user guide, earlier you wrote it is necessary, as antrea-multicluster-member.yml already import all CRDs.
If we do need to apply all YAMLs and the order does not matter, I would apply leader YAMLs first, which is easier to understand. @luolanzone
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.
yes, we removed those unnecessary CRDs which was both in leader and member yamls, so we need to apply antrea-multicluster-leader-global.yml for a dual-role cluster, I will move it.
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.
Done, the diagram is also updated.
ae7696c
to
c4003f8
Compare
c247938
to
455968a
Compare
docs/multicluster/quick-start.md
Outdated
|
||
```bash | ||
$kubectl apply -f https://github.com/antrea-io/antrea/releases/download/$TAG/antrea-multicluster-leader-global.yml | ||
$kubectl apply -f https://github.com/antrea-io/antrea/releases/download/$TAG/antrea-multicluster-member.yml |
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 noticed you added this line in your last update, but antrea-multicluster-member.yml is already applied in line 60. Should we remove this line?
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 also revised the diagram a little. Check.
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 removed the extra step to apply member manifest. The revised diagram looks good to me, but I changed the background to white so it can show in browser's dark mode clearly.
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.
7850531
to
2c2813e
Compare
$sed 's/changeme/antrea-multicluster/g' antrea-multicluster-leader-namespaced.yml | kubectl apply -f - | ||
$kubectl apply -f https://github.com/antrea-io/antrea/releases/download/$TAG/antrea-multicluster-member.yml | ||
``` | ||
|
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.
It might be good to add a command to check the deployment results that can show both leader & member mc-controller Deployments and Pods.
Does something like work: kc get all -A -l="app=xyz"
What should be the "xyz".
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.
sure, I will add it, it can be kubectl get all -A -l="component=antrea-mc-controller"
, should I add a sample output?
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.
Yes, better to have a sample output.
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.
Done
@luolanzone Hi Lan. I changed |
@hjiajing , I will check, thanks for the reminder, please make sure e2e is passed after you change the ID. |
Add a quick start guide. Update the user guide with Multi-cluster Gateway. Signed-off-by: Lan Luo <luola@vmware.com> Co-authored-by: Jianjun Shen <shenj@vmware.com>
Hi @jianjuns I also changed the |
/skip-all |
Add a quick start guide.
Update the user guide with Multi-cluster Gateway.
Signed-off-by: Lan Luo luola@vmware.com
Signed-off-by: Jianjun Shen shenj@vmware.com