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 info on scaling logic to typha overview #1360

Closed
wants to merge 5 commits into from

Conversation

jgilfoil
Copy link

@jgilfoil jgilfoil commented Mar 7, 2024

This information wasn't readily available anywhere in the docs that i could find, so I wanted to make it easier to find for users.

Product Version(s): 3.27

Issue: No issue on this that i'm aware of

Link to docs preview: Sorry, I don't know how to find this.
https://deploy-preview-1360--tigera.netlify.app/calico/latest/reference/typha/overview

SME review:

  • An SME has approved this change.

DOCS review:

  • A member of the docs team has approved this change.

Additional information:

Merge checklist:

  • Deploy preview inspected wherever changes were made
  • Build completed successfully
  • Test have passed

This information wasn't readily available anywhere in the docs that i could find, so I wanted to make it easier to find for users.
@jgilfoil jgilfoil requested a review from a team as a code owner March 7, 2024 22:07
@CLAassistant
Copy link

CLAassistant commented Mar 7, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for calico-docs-preview-next ready!

Name Link
🔨 Latest commit 644f71d
🔍 Latest deploy log https://app.netlify.com/sites/calico-docs-preview-next/deploys/65eb52474a742a00084b321f
😎 Deploy Preview https://deploy-preview-1360--calico-docs-preview-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 67 (🟢 up 9 from production)
Accessibility: 93 (no change from production)
Best Practices: 92 (no change from production)
SEO: 79 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview succeeded!

Built without sensitive environment variables

Name Link
🔨 Latest commit 644f71d
🔍 Latest deploy log https://app.netlify.com/sites/tigera/deploys/65eb524718ecb300082f923c
😎 Deploy Preview https://deploy-preview-1360--tigera.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 70 (🟢 up 1 from production)
Accessibility: 93 (no change from production)
Best Practices: 83 (no change from production)
SEO: 86 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

@jgilfoil
Copy link
Author

jgilfoil commented Mar 7, 2024

@tigera/docs

didn't intend for that to be a header
formatting got screwed up somehow when i removed the header
@lwr20
Copy link
Member

lwr20 commented Mar 8, 2024

Looks good.

I had a conversation with the other maintainers about whether we should document this implementation detail or not. Its a balance. The conclusion was that this table is fine, but it would be good to add a caveat that "we may change the algorithm as needs evolve". For example, at high scale we might reduce the number of typhas in future.

@lwr20
Copy link
Member

lwr20 commented Mar 8, 2024

Also, once we're happy with the text, we should copy it into calico/reference/typha/overview.mdx (so that the v3.28 versioned docs will get this change).

@lwr20
Copy link
Member

lwr20 commented Mar 8, 2024

I've added a link to the preview page so you can see your change as it will appear on the website.

Just noting that it could change in the future.
added so that it will go into the next version of the docs as well.
@jgilfoil
Copy link
Author

jgilfoil commented Mar 8, 2024

Thanks for the feedback. I've added a note about potential for change and added the update to the calico/reference/typha/overview.mdx file as well.

Copy link
Collaborator

@ctauchen ctauchen left a comment

Choose a reason for hiding this comment

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

@jgilfoil A few final changes for you. Some nits, a few comments and suggested deletions. Let me know if you have any questions. All comments apply to both files.

@@ -16,3 +16,31 @@ If you are using the Kubernetes API Datastore, we recommend using Typha. Althoug

- Since one Typha instance can support hundreds of Felix instances, it reduces the load on the datastore by a large factor.
- Since Typha can filter out updates that are not relevant to Felix, it also reduces Felix's CPU usage. In a high-scale (100+ node) Kubernetes cluster, this is essential because the number of updates generated by the API server scales with the number of nodes.

## Operator Scaling Logic
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## Operator Scaling Logic
## Operator scaling logic

We use sentence case for headings.


## Operator Scaling Logic

If you're using the Operator installation, then the operator will automatically scales Typha pods for optimal performance.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If you're using the Operator installation, then the operator will automatically scales Typha pods for optimal performance.
If you installed {{prodname}} using the operator, then the operator will automatically scale Typha pods to optimize performance.


If you're using the Operator installation, then the operator will automatically scales Typha pods for optimal performance.

- Small Cluster Considerations: Special handling is in place for clusters with fewer than five nodes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • It's not clear what this special handling is.
  • Whatever it is, we can keep the explanation as a regular sentence in the same paragraph as the previous sentence. For example, "For clusters with fewer than five nodes, we do things like this."
  • If the special handling is merely what's described in the table, let's just let the table do the work. We can strike out this line.


- Small Cluster Considerations: Special handling is in place for clusters with fewer than five nodes.

Here's the scaling logic it uses:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove. The table explains itself.

Comment on lines +42 to +46
:::note

This algorithm may change as needs evolve.

:::
Copy link
Collaborator

Choose a reason for hiding this comment

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

On second thought, let's remove this too. The same applies to pretty much everything in the docs. If we make a change, that should be documented just like any other change.

@ctauchen ctauchen closed this Oct 15, 2024
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.

4 participants