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

CDP PvC DS Pre-Check #2

Merged
merged 5 commits into from
Jul 27, 2022
Merged

CDP PvC DS Pre-Check #2

merged 5 commits into from
Jul 27, 2022

Conversation

Nishant-K-Raj
Copy link
Collaborator

CDP PvC DS Pre-Check

Chaffelson and others added 4 commits June 30, 2022 17:31
…ual usage

Updated ReadMe.md
Refactored entirety of preinstall_check.py reducing overall SLOC by 30%

Signed-off-by: Daniel Chaffelson <chaffelson@gmail.com>
correctly write cells to xlsxwriter
Add basic logging support
Add passwordless SSH check at start of script
Simplify ECS check sequencing logic

Signed-off-by: Daniel Chaffelson <chaffelson@gmail.com>
Reordered top level check sequence to make it easy to comment checks you want to skip
Moved SSH errors check to static value at top of Handler class

Signed-off-by: Daniel Chaffelson <chaffelson@gmail.com>
Copy link

@jimright jimright left a comment

Choose a reason for hiding this comment

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

Hi @Nishant-K-Raj
cc) @Chaffelson

I ran this PvC DS pre-checker utility on a small, test cluster. It's a great tool with some very informative output - thanks! Below is some feedback that might be helpful to improve the tool.

Just to note that my execution was running from my laptop which had passwordless ssh to all nodes in the cluster. Also, I was mainly looking from the point-of-view of usability and the contents of output rather than the type of checks performed.

Some improvements that would make the tool easier for my use-case:

  • A requirements.txt file with dependency Python packages would be good to include.
  • If it was possible to specify the input parameters in an input configuration file rather than editing the source directly, that would make the setup and reuse of the code easier.
  • For SSH to the remote nodes it would be nice to be able to optionally specify the username to be used with the SSH and SCP commands.
  • Also the option to disable SSH host key checking would be nice.

Thanks,
Jim


```

## Configure:

Choose a reason for hiding this comment

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

A note here about where these configuration settings should be made (in the preinstall_check.py script) would be good to include.

"ecs-5.company.com"]

# Define Postgres ECS DB Host
postgres_host = "ecs-db-host.company.com"

Choose a reason for hiding this comment

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

The postgres_host setting does not seem to be supported in the code.

@Chaffelson Chaffelson merged commit 4e90cbf into cloudera-labs:main Jul 27, 2022
pmereddy pushed a commit to pmereddy/toolkits that referenced this pull request Nov 2, 2022
* CDP PvC DS Pre-Check

* renamed version_check.py to preinstall_check.py to better reflect actual usage
Updated ReadMe.md
Refactored entirety of preinstall_check.py reducing overall SLOC by 30%

Signed-off-by: Daniel Chaffelson <chaffelson@gmail.com>

* Correctly quote spaces in cluster names
correctly write cells to xlsxwriter
Add basic logging support
Add passwordless SSH check at start of script
Simplify ECS check sequencing logic

Signed-off-by: Daniel Chaffelson <chaffelson@gmail.com>

* Added dns check for provided hostnames to fail early
Reordered top level check sequence to make it easy to comment checks you want to skip
Moved SSH errors check to static value at top of Handler class

Signed-off-by: Daniel Chaffelson <chaffelson@gmail.com>

Co-authored-by: Daniel Chaffelson <chaffelson@gmail.com>
Co-authored-by: @Nishant-K-Raj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants