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

Multi cluster doc improve #533

Merged
merged 14 commits into from
Nov 23, 2023
Merged

Conversation

zijun726911
Copy link
Contributor

@zijun726911 zijun726911 commented Nov 20, 2023

What type of PR is this? documentation

Which issue does this PR fix: Add new multi cluster doc and diagram to reflect the new controller behaviour, deprecate the outdated description

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Zijun Wang added 8 commits November 16, 2023 11:12
@zijun726911 zijun726911 force-pushed the multi-cluster-doc-improve branch from b7a97a0 to ed548e3 Compare November 20, 2023 21:44
Copy link
Contributor

@mikhail-aws mikhail-aws left a comment

Choose a reason for hiding this comment

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

Need to make it crisp(er). Documents describes "How" to setup, but does not explain "Why" it's recommended way.

Picture is complicated and mix of different ideas. Different names - "my-hotel", "my-httproute", "service". My-hotel looks foreign (or others?). We should have it clear what are k8s and aws/lattice concepts in picture. May be dont mix them together, if you want them on the same picture try to use layers.

@zijun726911
Copy link
Contributor Author

zijun726911 commented Nov 20, 2023

I can change "my-hotel" to "my-gateway". The old doc didn't mention why it's recommended. I can add few sentences in the new doc to explain it.

We should have it clear what are k8s and aws/lattice concepts in picture. May be dont mix them together, if you want them on the same picture try to use layers.

I think the only lattice resource needed in the graph is service network, since its not exactly 1:1 map to the k8s gateway. for other k8s resource I can use k8s icon and word "k8s" to make it clear. Not quite sure how the layers looks like

- Multiple `HTTPRoute(s)`,`GRPCRoute(s)`, that have rules backendRefs to `ServiceImport(s)` that referring kubernetes application service(s) in workload cluster(s)


You can see this similar production use case at Airbnb: [airbnb mullti-cluster setup](https://www.youtube.com/watch?v=1D8lg36ZNHs)
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 we should remove this, instead we should have good reasons for this recommendation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea on what good reasons we could say?

docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
@zijun726911 zijun726911 force-pushed the multi-cluster-doc-improve branch from 2ba72eb to 699a38c Compare November 22, 2023 22:15
Copy link
Contributor

@solmonk solmonk 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 the contents are fine, we can start from this. Just found a few expressions that could be better

docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
docs/guides/multi-cluster.md Outdated Show resolved Hide resolved
@zijun726911 zijun726911 force-pushed the multi-cluster-doc-improve branch from 28567cf to a93d35e Compare November 22, 2023 23:13
@zijun726911 zijun726911 force-pushed the multi-cluster-doc-improve branch from 657b78a to 5e5f66b Compare November 22, 2023 23:55
@zijun726911 zijun726911 merged commit 981ae25 into aws:main Nov 23, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants