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

HPCC-27610 Update Container Placements Documentation #17917

Merged
merged 1 commit into from
Dec 14, 2023

Conversation

g-pan
Copy link
Member

@g-pan g-pan commented Oct 18, 2023

Type of change:

  • This change is a bug fix (non-breaking change which fixes an issue).
  • This change is a new feature (non-breaking change which adds functionality).
  • This change improves the code (refactor or other change that does not change the functionality)
  • This change fixes warnings (the fix does not alter the functionality or the generated code)
  • This change is a breaking change (fix or feature that will cause existing behavior to change).
  • This change alters the query API (existing queries will have to be recompiled)

Checklist:

  • My code follows the code style of this project.
    • My code does not create any new warnings from compiler, build system, or lint.
  • The commit message is properly formatted and free of typos.
    • The commit message title makes sense in a changelog, by itself.
    • The commit is signed.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly, or...
    • I have created a JIRA ticket to update the documentation.
    • Any new interfaces or exported functions are appropriately commented.
  • I have read the CONTRIBUTORS document.
  • The change has been fully tested:
    • I have added tests to cover my changes.
    • All new and existing tests passed.
    • I have checked that this change does not introduce memory leaks.
    • I have used Valgrind or similar tools to check for potential issues.
  • I have given due consideration to all of the following potential concerns:
    • Scalability
    • Performance
    • Security
    • Thread-safety
    • Cloud-compatibility
    • Premature optimization
    • Existing deployed queries will not be broken
    • This change fixes the problem, not just the symptom
    • The target branch of this pull request is appropriate for such a change.
  • There are no similar instances of the same problem that should be addressed
    • I have addressed them here
    • I have raised JIRA issues to address them separately
  • This is a user interface / front-end modification
    • I have tested my changes in multiple modern browsers
    • The component(s) render as expected

Smoketest:

  • Send notifications about my Pull Request position in Smoketest queue.
  • Test my draft Pull Request.

Testing:

Successful unit testing: http://10.224.20.18/view/Docs-gp/job/DocBuild-GPRepo3/

@github-actions
Copy link

Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

A few comments inline

needs.</para>

<para>You can deploy these values either using the values.yaml file or you
can place into an file and have Kubernetes instead read the values from
Copy link
Contributor

Choose a reason for hiding this comment

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

a file, not an file

<orderedlist>
<para>The pods: [list] item can contain one of the following:</para>

<para><orderedlist>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be a numbered list. A table would be a better representation.

<emphasis>global.env</emphasis> portion of the provided HPCC Systems
values.yaml file. These values are specified as a list of name value
pairs as illustrated below.</para>
<para>In a Kubernetes container environment there are several ways to
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a comma :
In a Kubernetes container environment, there...

nodeSelector:
group: "hpcc"</programlisting></para>

<para>Note:the label: group:hpcc matches the node pool label:
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: s/b bold, then a space and capitalize the sentence. also no need for the quotes around hpcc.

Note: The label: group:hpcc matches the node pool label:hpcc.

<entry>JAVA_LIBRARY_PATH</entry>
</row>
<para>Taints and Tolerations are types of Kubernetes node constraints
also referred to by Node Affinity. Only one "affinity" can be applied
Copy link
Contributor

Choose a reason for hiding this comment

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

inconsistent capitalization of Node Affinity. Previously, it was node Affinity.

<entry>JAVA_LIBRARY_PATH</entry>
</row>
<para>Taints and Tolerations are types of Kubernetes node constraints
also referred to by Node Affinity. Only one "affinity" can be applied
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for quotes around affinity (2 places)

respectively. The Roxie pods will be evenly scheduled on the two node
pools.</para>

<para>After deployment you can verify by issuing the following:</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

After deployment you can verify by issuing the following command:


<para>There is no schema check for the content of affinity. Only one
affinity can be applied to a pod or job. If a pod/job matches
multiple placement 'pods' lists, then only the last affinity
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for the quotes around 'pods'


<para>Only one "schedulerName" can be applied to any pod/job.</para>

<para>A SchedulerName example:</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

Inconsistent capitalization of schedulerName

Copy link
Member

@xwang2713 xwang2713 left a comment

Choose a reason for hiding this comment

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

Looks good for me. Just Remove "Examples:" before "Taints and Tolerations" on p20

@g-pan g-pan requested a review from JamesDeFabia October 30, 2023 19:28
Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

Couple of minor comments inline

refer to Kubernetes scheduling as placements. Placements is a value in
an HPCC Systems configuration which is at a level above items, such as
the nodeSelector, Toleration, Affinity and Anti-Affinity, and
TopologySpreadConstratints.</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

DId you mean TopologySpreadConstraints?

TopologySpreadConstratints.</para>

<para>The placement is responsible for finding the best node for a pod.
Most often placement are handled automatically by Kubernetes. You can
Copy link
Contributor

@JamesDeFabia JamesDeFabia Oct 30, 2023

Choose a reason for hiding this comment

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

Subject/Verb agreement: S/B Most often placements are handled

<sect2 id="YAML_FileStruct_Placement">
<title>Placements</title>

<para>The Placements is a term used by HPCC Systems, which Kubernetes
Copy link
Contributor

Choose a reason for hiding this comment

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

remove word "The"

Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

Looks good from my POV

@g-pan g-pan requested a review from JamesDeFabia October 31, 2023 14:59
Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

Looks good from my POV

Copy link
Contributor

@JamesDeFabia JamesDeFabia left a comment

Choose a reason for hiding this comment

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

Looks OK from my POV


<entry>This is the "Deployment" metadata name from the name of
the array item of a type. For example, "eclwatch-", "mydali-",
"thor-thoragent" This can be a regular expression since
Copy link
Member

Choose a reason for hiding this comment

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

@xwang2713 - are regular expressions supported or just wildcards?

Copy link
Member

Choose a reason for hiding this comment

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

The regular expressions are preferred. But I think substrings are also OK . Helm docs (mustRegexMatch):
regexMatch, mustRegexMatch. Returns true if the input string contains any match of the regular expression.

<entry>The job name is typically a regular expression as well,
since the job name is generated dynamically. For example, a
compile job compile-54eB67e567e, could use "compile-" or
"compile-*" or the exact match "^compile-.$"</entry>
Copy link
Member

Choose a reason for hiding this comment

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

@xwang2713 - I want to clarify if this supports regular expressions or just wildcards, "compile-*" is not an example of a regular expression (well it is, but would match 'compile----' , or 'compile', not 'compile-blah')

Copy link
Member

@xwang2713 xwang2713 Dec 6, 2023

Choose a reason for hiding this comment

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

Either "compile-.*" or "compile-". "compile-*" is not valid regular expression or substring

Copy link
Member

Choose a reason for hiding this comment

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

Ok, then line 1308 needs changing to "compile-.*"
@g-pan

<para>NodeSelector, taints and tolerations, and other values can all
be placed on the same pods: [list] both per zone and per node on
Azure <programlisting>placements:
- pods: ["eclwatch","roxie-workunit","^compile-.*$","mydali"]
Copy link
Member

Choose a reason for hiding this comment

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

but 'compile-.$' is a regex, so is it that it does support regex's, but the example 'compile-' is wrong?

Copy link
Member

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

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

@g-pan - please see question re. regex/pattern match.

@ghalliday
Copy link
Member

Assigned to @xwang2713 to answer Jake's questions.

@stuartort
Copy link
Member

@g-pan, the jira is assigned to you. You have indicated that you have the information that you need

@g-pan g-pan requested a review from jakesmith December 7, 2023 19:51
@@ -1305,7 +1305,7 @@ thor:
<entry>The job name is typically a regular expression as well,
since the job name is generated dynamically. For example, a
compile job compile-54eB67e567e, could use "compile-" or
"compile-*" or the exact match "^compile-.$"</entry>
"compile-.*" or the exact match "^compile-.$"</entry>
Copy link
Member

Choose a reason for hiding this comment

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

minor: sorry, I did not notice this before:

For example, a compile job compile-54eB67e567e, could use "compile-" or "compile-.*" or the exact match "^compile-.$"

"exact match" in this sentence is misleading/inaccurate, and the regular expression "compile-.$" does not match.
I'd probably drop that qualifier and correct the regex :

For example, a compile job compile-54eB67e567e, could use "compile-", "compile-.*" or "^compile-.*$"

Copy link
Member

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

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

@g-pan - see follow-up comment.

@g-pan
Copy link
Member Author

g-pan commented Dec 11, 2023

added the last change and squashed (since it was minor) should be ready to merge now.

@g-pan g-pan requested a review from jakesmith December 11, 2023 13:25
<entry>The job name is typically a regular expression as well,
since the job name is generated dynamically. For example, a
compile job compile-54eB67e567e, could use "compile-" or
"compile-.*" or "^compile-.$"</entry>
Copy link
Member

Choose a reason for hiding this comment

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

@g-pan - still not quite right, missing * before $, see previous comment

Copy link
Member

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

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

@g-pan - back to you.

@g-pan
Copy link
Member Author

g-pan commented Dec 11, 2023

@g-pan - back to you.

good catch!

@g-pan g-pan requested a review from jakesmith December 11, 2023 20:27
Copy link
Member

@jakesmith jakesmith left a comment

Choose a reason for hiding this comment

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

@g-pan - looks good now.

@ghalliday ghalliday merged commit 547610b into hpcc-systems:candidate-9.2.x Dec 14, 2023
50 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.

6 participants