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

docs: Add example to pod placement #413

Merged
merged 10 commits into from
Nov 14, 2023
Merged

Conversation

sbernauer
Copy link
Member

Description

Please add a description here. This will become the commit message of the merge request later.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes

Author

Preview Give feedback

Reviewer

Preview Give feedback

Acceptance

Preview Give feedback

@sbernauer sbernauer self-assigned this Nov 8, 2023
@sbernauer sbernauer requested a review from lfrancke November 8, 2023 08:30
@@ -104,7 +104,7 @@ In the examples above `cluster-name` is the name of the HBase custom resource th
NOTE: It is important that the `hdfsConfigMapName` property contains the name the HDFS cluster. You could instead configure ConfigMaps of specific name or data roles, but for the purpose of pod placement, this will lead to faulty behavior.

== Use custom pod placement
xref:concepts:operations/pod_placement.adoc[] describes how you can configure pod placement in general.
xref:concepts:operations/pod_placement.adoc[The concepts page on Pod Placement] describes how you can configure pod placement in general.
Copy link
Member

@lfrancke lfrancke Nov 8, 2023

Choose a reason for hiding this comment

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

I'm sorry but: How does this look in a rendered version now?
I wanted to avoid the duplication of "Pod placement"
And: It should be "placement" not "Placement"

And: You have "Pod" once and "pod" another time. We should be consistent

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed lower and upper case.

Currently it reads

The concepts page on Pod placement describes how you can configure Pod placement in general.

We could change it to e.g.

This concepts page describes how you can configure Pod placement in general.

but I have been told to not have "click this" links, but instead tell the user where the link is pointing too.
But I don't have a strong opinion and let you decide

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion:

For general configuration of Pod placement, see the [Pod placement concepts] page.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied in 2996c2f

Comment on lines 10 to 12
1. Co-locate all the HBase Pods (weight 20)
2. Co-locate HBase regionservers with the underlying HDFS datanodes (weight 50)
3. Distribute all Pods within the same role (masters, regionservers, rest servers) (weight 70)
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, reading this again now, I have a new suggestion.

Suggested change
1. Co-locate all the HBase Pods (weight 20)
2. Co-locate HBase regionservers with the underlying HDFS datanodes (weight 50)
3. Distribute all Pods within the same role (masters, regionservers, rest servers) (weight 70)
1. Co-locate all the HBase Pods (weight 20)
2. Co-locate HBase regionservers with the underlying HDFS datanodes (weight 50)
3. Distribute all Pods within the same role across nodes so multiple instances don't end up on the same Kubernetes node (masters, regionservers, rest servers) (weight 70)

Point 1 needs clarification but I don't know exactly how. What is it we do exactly there?
And we should point out (I thought we did somewhere) that this is "only" preferred not required

Copy link
Member Author

Choose a reason for hiding this comment

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

See 30eba1a

Copy link
Member

Choose a reason for hiding this comment

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

Thank you. But what does the first point actually mean?
"Co-locate al the HBase Pods"

Copy link
Member Author

Choose a reason for hiding this comment

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

Its the first rule in the example below

affinity:
  podAffinity:
    preferredDuringSchedulingIgnoredDuringExecution:
    - podAffinityTerm:
        labelSelector:
          matchLabels:
            app.kubernetes.io/instance: cluster-name
            app.kubernetes.io/name: hbase
        topologyKey: kubernetes.io/hostname
      weight: 20
  podAntiAffinity:
    preferredDuringSchedulingIgnoredDuringExecution:
    - podAffinityTerm:
        labelSelector:
          matchLabels:
            app.kubernetes.io/component: master
            app.kubernetes.io/instance: cluster-name
            app.kubernetes.io/name: hbase
        topologyKey: kubernetes.io/hostname
      weight: 70

I just pulls all HBase Pods together, so that e.g. hbase master + regionserver + restserver run on the same node to reduce latency.

@sbernauer sbernauer requested a review from lfrancke November 13, 2023 12:54
@sbernauer
Copy link
Member Author

Thanks!

@sbernauer sbernauer added this pull request to the merge queue Nov 14, 2023
Merged via the queue into main with commit e3c69a6 Nov 14, 2023
29 checks passed
@sbernauer sbernauer deleted the docs/improve-pod-placement branch November 14, 2023 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants