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

Add initial strategies document #556

Merged
merged 1 commit into from
Aug 12, 2021
Merged

Add initial strategies document #556

merged 1 commit into from
Aug 12, 2021

Conversation

k0da
Copy link
Collaborator

@k0da k0da commented Aug 11, 2021

  • This moves strategy options into a separate document page for better
    visibility.
  • Describes new geoip strategy option.

Signed-off-by: Dinar Valeev [email protected]

Copy link
Collaborator

@kuritka kuritka left a comment

Choose a reason for hiding this comment

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

Hi, my comment is not only about GeoIP, but overall strategies. Shouldn't we provide more detailed examples ? (e.g: some pictures + example configuration below). Make it more user friendly....

@k0da
Copy link
Collaborator Author

k0da commented Aug 11, 2021

@kuritka my intent is to provide a visible overview what is the difference between them. How to configure spec and architecture is available already.

kuritka
kuritka previously approved these changes Aug 11, 2021
@k0da
Copy link
Collaborator Author

k0da commented Aug 11, 2021

@ytsarev amended

@k0da k0da requested a review from kuritka August 12, 2021 12:32
@k0da
Copy link
Collaborator Author

k0da commented Aug 12, 2021

@ytsarev proposal commited, thanks

Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

Sorry for not being clear enough, I meant to use proper case for all the strategies - provided additional suggestions

docs/strategy.md Outdated
## failover
Pinned to a specified primary cluster until workload on that cluster has no available Pods, upon which the next available cluster's Ingress node IPs will be resolved. When Pods are again available on the primary cluster, the primary cluster will once again be the only eligible cluster for which cluster Ingress node IPs will be resolved

## roundRobin (*default*)
Copy link
Member

Choose a reason for hiding this comment

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

Please place this strategy first on the list. What do you mean by default ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ytsarev default means this is default strategy

Copy link
Member

Choose a reason for hiding this comment

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

@k0da yeah. But why is it default? we always specify the strategies explicitly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ytsarev inspired by docs I'm changing:
* **Round robin** - The default strategy

Copy link
Member

Choose a reason for hiding this comment

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

@k0da still I am challenging it :) It's not real default one :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ytsarev as you wish, amended

@k0da k0da requested a review from ytsarev August 12, 2021 13:49
@k0da k0da force-pushed the strategy branch 2 times, most recently from 39e8479 to b9173b7 Compare August 12, 2021 14:34
## failover
Pinned to a specified primary cluster until workload on that cluster has no available Pods, upon which the next available cluster's Ingress node IPs will be resolved. When Pods are again available on the primary cluster, the primary cluster will once again be the only eligible cluster for which cluster Ingress node IPs will be resolved

## roundRobin
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was lost during rebase, now it is there

* This moves strategy options into a separate document page for better
  visibility.
* Describes new geoip strategy option.

Signed-off-by: Dinar Valeev <[email protected]>
Co-authored-by: Yury Tsarev <[email protected]>
Copy link
Member

@ytsarev ytsarev left a comment

Choose a reason for hiding this comment

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

lgtm

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