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.

Sign up for GitHub

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

Jump to bottom

HPCC-25955 implemented static pvs #17607

Closed
wants to merge 37 commits into from
Closed

HPCC-25955 implemented static pvs #17607

wants to merge 37 commits into from

Conversation

rrao4
Copy link
Contributor

@rrao4 rrao4 commented Jul 19, 2023

Implemented static pvs in order to create storage beyond the eks cluster.
Signed off by [email protected]

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:

I tested the changes using AWS EKS.

ghalliday and others added 19 commits June 15, 2023 17:48
@ghalliday
Split off 9.2.0
fa75b12 
Signed-off-by: Gavin Halliday <[email protected]>
@ghalliday
Merge remote-tracking branch 'origin/candidate-9.2.x'
c71f587 
Signed-off-by: Gavin Halliday <[email protected]>

# Conflicts:
#	helm/hpcc/Chart.yaml
#	helm/hpcc/templates/_helpers.tpl
#	helm/hpcc/templates/dafilesrv.yaml
#	helm/hpcc/templates/dali.yaml
#	helm/hpcc/templates/dfuserver.yaml
#	helm/hpcc/templates/eclagent.yaml
#	helm/hpcc/templates/eclccserver.yaml
#	helm/hpcc/templates/eclscheduler.yaml
#	helm/hpcc/templates/esp.yaml
#	helm/hpcc/templates/localroxie.yaml
#	helm/hpcc/templates/roxie.yaml
#	helm/hpcc/templates/sasha.yaml
#	helm/hpcc/templates/thor.yaml
#	version.cmake
@jakesmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
cf6c53d 
Signed-off-by: Jake Smith <[email protected]>
@ghalliday
Merge remote-tracking branch 'origin/candidate-9.2.x'
de2a431 
Signed-off-by: Gavin Halliday <[email protected]>
@ghalliday
Merge remote-tracking branch 'origin/candidate-9.2.x'
37168d0 
Signed-off-by: Gavin Halliday <[email protected]>
@GordonSmith
HPCC-29828 Dev docs failing to build/publish
cf1aa44 
Signed-off-by: Gordon Smith <[email protected]>
@ghalliday
Merge pull request hpcc-systems#17512 from GordonSmith/HPCC-29828-DEV…
5516499 
…_DOCS

HPCC-29828 Dev docs failing to build/publish

Reviewed-by: Gavin Halliday <[email protected]>
Merged-by: Gavin Halliday <[email protected]>
@GordonSmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
941d048 
Signed-off-by: Gordon Smith <[email protected]>
@GordonSmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
e289e14 
Signed-off-by: Gordon Smith <[email protected]>
@kenrowland
HPCC-29744 Remove existing 'optional' keywords from SCM files
4f349cb 
Removed 'optional' keywords from existing SCM files because they
were not added correctly. The keyword requires a string value in
the form optional("value") where "value" creates a category of
optional fields exposed via the interface when the category
"value" is included as a URL query parameter.

Signed-Off-By: Kenneth Rowland [email protected]
@ghalliday
HPCC-29858 Update the platform version rules
05ab14a 
Signed-off-by: Gavin Halliday <[email protected]>
@GordonSmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
9e395ea 
Signed-off-by: Gordon Smith <[email protected]>
@ghalliday
Merge pull request hpcc-systems#17505 from kenrowland/HPCC-29744
f179fd5 
HPCC-29744 Remove existing 'optional' keywords from SCM files

Reviewed-By: Shamser Ahmed <[email protected]>
Reviewed-By: Anthony Fishbeck <[email protected]>
Merged-by: Gavin Halliday <[email protected]>
@jakesmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
908a215 
Signed-off-by: Jake Smith <[email protected]>
@kenrowland
HPCC-29381 Improve XSD generation for ESP services
cd987c5 
Replaced explicit XSD and WSDL generation in genreated code by
using existing library function.

Signed-Off-By: Kenneth Rowland [email protected]
@GordonSmith
Merge pull request hpcc-systems#17534 from ghalliday/versiondetails
58c3c09 
HPCC-29858 Update the platform version rules
@kenrowland
Add dependency so unit tests will build
8a7a098
@GordonSmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
c9bd9c5 
Signed-off-by: Gordon Smith <[email protected]>

# Conflicts:
#	helm/hpcc/Chart.yaml
#	helm/hpcc/templates/_helpers.tpl
#	helm/hpcc/templates/dafilesrv.yaml
#	helm/hpcc/templates/dali.yaml
#	helm/hpcc/templates/dfuserver.yaml
#	helm/hpcc/templates/eclagent.yaml
#	helm/hpcc/templates/eclccserver.yaml
#	helm/hpcc/templates/eclscheduler.yaml
#	helm/hpcc/templates/esp.yaml
#	helm/hpcc/templates/localroxie.yaml
#	helm/hpcc/templates/roxie.yaml
#	helm/hpcc/templates/sasha.yaml
#	helm/hpcc/templates/thor.yaml
#	version.cmake
@GordonSmith
Merge pull request hpcc-systems#17349 from kenrowland/HPCC-29381
75bc83c 
HPCC-29381 Improve XSD generation for ESP services
@github-actions
Copy link

https://track.hpccsystems.com/browse/HPCC-25955
Jira not updated (user does not match)

@xwang2713
Copy link
Member

@g-pan please remove examples/efs/README.md

@GordonSmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
626c9dd 
Signed-off-by: Gordon Smith <[email protected]>

# Conflicts:
#	helm/hpcc/Chart.yaml
#	helm/hpcc/templates/_helpers.tpl
#	helm/hpcc/templates/dafilesrv.yaml
#	helm/hpcc/templates/dali.yaml
#	helm/hpcc/templates/dfuserver.yaml
#	helm/hpcc/templates/eclagent.yaml
#	helm/hpcc/templates/eclccserver.yaml
#	helm/hpcc/templates/eclscheduler.yaml
#	helm/hpcc/templates/esp.yaml
#	helm/hpcc/templates/localroxie.yaml
#	helm/hpcc/templates/roxie.yaml
#	helm/hpcc/templates/sasha.yaml
#	helm/hpcc/templates/thor.yaml
#	version.cmake
g-pan
g-pan reviewed Jul 25, 2023
View reviewed changes
Copy link
Member

@g-pan g-pan left a comment

Choose a reason for hiding this comment

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

Only one comment/question otherwise approved.

helm/examples/efs/README.md
helm uninstall myhpcc
```
## 2. Static storage within Kubernetes with values-retained-efs.yaml
In this method, storage lives on the Kubernetes cluster level. It uses the helm chart "hpcc-efs-dynamic-pv" to manually create PVCs. The PVs are dynamically generated. The PVCs will persist after the HPCC cluster is deleted, meaning that the storage can be reused across different HPCC clusters.<br/>
Copy link
Member

Choose a reason for hiding this comment

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

The PVs are dynamically, should it be PVCs ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the PVCs aren't dynamically created, in the hpcc-efs-dynamic-pv/templates/create-pvc.yaml, they are actually manually created.

xwang2713
xwang2713 reviewed Jul 26, 2023
View reviewed changes
helm/examples/efs/create-ap.sh Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Add comment that For RISK user tag "owner" and "owner_email" are may required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this just now to the README under the explanation of the 3rd storage method.

xwang2713
xwang2713 reviewed Jul 26, 2023
View reviewed changes
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.

Generally looks good. The tags need "owner" and "owner_email" for RISK users.

kenrowland and others added 9 commits July 27, 2023 08:22
@kenrowland
HPCC-29963 Incorrect XSD type output for TpQueryType in WsTopology se…
95e4c94 
…rvice

Changed TpQueryType to string to produce correct type in XSD

Signed-Off-By: Kenneth Rowland [email protected]
@GordonSmith
Merge pull request hpcc-systems#17612 from kenrowland/HPCC-29963
d2f29c6 
HPCC-29963 Correct XSD type conversion for TpQueryType in WsTopology Service
@GordonSmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
b0fa567 
Signed-off-by: Gordon Smith <[email protected]>

# Conflicts:
#	helm/hpcc/Chart.yaml
#	helm/hpcc/templates/_helpers.tpl
#	helm/hpcc/templates/dafilesrv.yaml
#	helm/hpcc/templates/dali.yaml
#	helm/hpcc/templates/dfuserver.yaml
#	helm/hpcc/templates/eclagent.yaml
#	helm/hpcc/templates/eclccserver.yaml
#	helm/hpcc/templates/eclscheduler.yaml
#	helm/hpcc/templates/esp.yaml
#	helm/hpcc/templates/localroxie.yaml
#	helm/hpcc/templates/roxie.yaml
#	helm/hpcc/templates/sasha.yaml
#	helm/hpcc/templates/thor.yaml
#	version.cmake
@richardkchapman
HPCC-30005 Refactor IMessageUnpackCursor
ea7a85c 
The way record lengths were fetched caused significant extra work for the localAgent
where a temporary roxie row would be allocated and released for each count.

Signed-off-by: Richard Chapman <[email protected]>
@richardkchapman
HPCC-29831 Allow localAgent (and other) setting to be set in workunit…
d167c56 
…/regression suite

These options are only applied when Roxie us run in workunit mode in containerized systems.

Signed-off-by: Richard Chapman <[email protected]>
@richardkchapman
HPCC-29831 Allow localAgent (and other) setting to be set in workunit…
da684f0 
…/regression suite

Fix some issues with setting topology when localAgent is false

Signed-off-by: Richard Chapman <[email protected]>
@kenrowland
HPCC-27303 Coverity scan reported new defects related to Configuration
d072902 
Changed loop to return immediately when element erased from map

Signed-Off-By: Kenneth Rowland [email protected]
@jakesmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
9a26847 
Signed-off-by: Jake Smith <[email protected]>

# Conflicts:
#	helm/hpcc/Chart.yaml
#	helm/hpcc/templates/_helpers.tpl
#	helm/hpcc/templates/dafilesrv.yaml
#	helm/hpcc/templates/dali.yaml
#	helm/hpcc/templates/dfuserver.yaml
#	helm/hpcc/templates/eclagent.yaml
#	helm/hpcc/templates/eclccserver.yaml
#	helm/hpcc/templates/eclscheduler.yaml
#	helm/hpcc/templates/esp.yaml
#	helm/hpcc/templates/localroxie.yaml
#	helm/hpcc/templates/roxie.yaml
#	helm/hpcc/templates/sasha.yaml
#	helm/hpcc/templates/thor.yaml
#	version.cmake
HPCC-29972 Add WsTopology.TpListLogFiles
ab55e84 
The existing code uses the FileSpray.FileList to list the log files
of each HPCC component in bare metal enviroment, which does not make
sense. In this PR, a similar funcitonality is added to WsTopology.

Signed-off-by: wangkx <[email protected]>
@xwang2713 xwang2713 requested a review from g-pan August 8, 2023 12:45
g-pan
g-pan reviewed Aug 8, 2023
View reviewed changes
Copy link
Member

@g-pan g-pan 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 comments in line.

helm/examples/efs/README.md Outdated
```
An example values file to be supplied when installing the HPCC chart.
NB: Either use the output auto-generated when installing the "hpcc-efs" helm chart, or ensure the names in your values files for the storage types match the PVC names created. "values-retained-efs.yaml" expects that helm chart installation name is "awsstorage". Change the PVC name accordingly if another name is used.
NB: Either use the output auto-generated when installing the "hpcc-efs" helm chart, or ensure the names in your values files for the storage types match the PVC names created. "values-retained-efs.yaml" expects that helm chart installation name is "awsstorage". Change the PVC name accordingly if another name is used.<br/>
Copy link
Member

Choose a reason for hiding this comment

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

Change: "values-retained-efs to: The "values-retained-efs

helm/examples/efs/README.md Outdated
```
## 3. Static storage beyond Kubernetes with values-retained-efs.yaml
In this method, the storage lives beyond the Kubernetes cluster. It uses the helm chart "hpcc-efs-static-pv" to manually create PVs and PVCs and to configure access points in EFS with the Kubernetes cluster. This means that even if the Kubernetes cluster is deleted, the storage will remain and can be reused across different Kubernetes clusters.<br/>
The create-ap.sh script creates five access points in EFS for each of dali, dll, sasha, data, and mydropzone, and it displays a description for each access point. You may need to add additional tags to the access points depending on your organization. For example, for RISK users, the "owner" and "owner_email" tags are required; add these to the script as needed.<br/>
Copy link
Member

Choose a reason for hiding this comment

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

grammar: remove the "and" before mydropzone, s/b: for each of dali, dll, sasha, data, mydropzone, and it displays a description for each access point.
or could rewrite
The create-ap.sh script creates five access points in EFS. One for each component dali, dll, sasha, data, and mydropzone. It also displays a description for each access point.

helm/examples/efs/README.md Outdated

helm uninstall will not delete EFS persistant volumes claims (PVC). You can either run "kubectl delete pv <pv name> or --all".
Again, the storage needs to be created before starting the HPCC cluster. Under the helm directory, run the following command:
Copy link
Member

Choose a reason for hiding this comment

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

remove the word "Again"

helm/examples/efs/README.md Outdated
Navigate to the EFS service and click "Create file system". Select the same VPC as the one your EKS cluster is running in.
### Configuring the Mount Targets
Click on the file system you just created and navigate to "Network". Your mount targets should be displayed.<br/>
Click on "Manage", and you should see that the security groups for the mount targets are the default security group. Replace these with the EKS cluster security group. To find the EKS cluster security group, navigate to EKS, click on your cluster, click on "Networking", and it is the security group displayed under "Cluster security group". After replacing the default security group for all mount targets, save your changes.
Copy link
Member

Choose a reason for hiding this comment

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

run on sentence: To find the EKS cluster security group, navigate to EKS, click on your cluster, click on "Networking", and it is the security group displayed under "Cluster security group".

Can rewrite as : To find the EKS cluster security group navigate to EKS. Click on your cluster. Click on "Networking" and find the security group displayed under "Cluster security group".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I've updated the parts you mentioned.

g-pan
g-pan approved these changes Aug 8, 2023
View reviewed changes
Copy link
Member

@g-pan g-pan left a comment

Choose a reason for hiding this comment

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

approved.

@ghalliday
Merge pull request hpcc-systems#17643 from kenrowland/HPCC-27303
49b8387 
HPCC-27303 Coverity scan reported new defects related to Configuration

Reviewed-By: Attila Vamos <[email protected]>
Merged-by: Gavin Halliday <[email protected]>
@ghalliday
Merge pull request hpcc-systems#17622 from wangkx/h29972
ed7561c 
HPCC-29972 Add WsTopology.TpListLogFiles

Reviewed-By: Anthony Fishbeck <[email protected]>
Merged-by: Gavin Halliday <[email protected]>
@ghalliday
Merge pull request hpcc-systems#17640 from richardkchapman/messageunp…
c9d6d57 
…acker

HPCC-30005 Refactor IMessageUnpackCursor

Reviewed-by: Gavin Halliday <[email protected]>
Merged-by: Gavin Halliday <[email protected]>
@ghalliday
Merge pull request hpcc-systems#17513 from richardkchapman/localagent-wu
1e7caef 
HPCC-29831 Allow localAgent setting to be set in workunit/regression suite

Reviewed-by: Jake Smith <[email protected]>
Reviewed-by: Gavin Halliday <[email protected]>
Merged-by: Gavin Halliday <[email protected]>
xwang2713
xwang2713 approved these changes Aug 10, 2023
View reviewed changes
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.

Ready to merge

@ghalliday
Copy link
Member

Please squash ready for merging

jakesmith and others added 2 commits August 11, 2023 10:22
@jakesmith
Merge remote-tracking branch 'origin/candidate-9.2.x'
651fcfa 
Signed-off-by: Jake Smith <[email protected]>

# Conflicts:
#	helm/hpcc/Chart.yaml
#	helm/hpcc/templates/_helpers.tpl
#	helm/hpcc/templates/dafilesrv.yaml
#	helm/hpcc/templates/dali.yaml
#	helm/hpcc/templates/dfuserver.yaml
#	helm/hpcc/templates/eclagent.yaml
#	helm/hpcc/templates/eclccserver.yaml
#	helm/hpcc/templates/eclscheduler.yaml
#	helm/hpcc/templates/esp.yaml
#	helm/hpcc/templates/localroxie.yaml
#	helm/hpcc/templates/roxie.yaml
#	helm/hpcc/templates/sasha.yaml
#	helm/hpcc/templates/thor.yaml
#	version.cmake
@rrao4
HPCC-25955 implemented static pvs
9f9b8f5
@rrao4
Copy link
Contributor Author

rrao4 commented Aug 11, 2023

Hi Gavin,

I rebased the commits into one, which is "HPCC-25955 implemented static pvs". Let me know if its ok to merge, or if I should create a new pull request.

Thanks,
Ryan Rao

@ghalliday
Copy link
Member

I think you need to execute something like:

git fetch upstream
git rebase --onto upstream/candidate-9.2.x HEAD~1

Assuming upstream refers to hpcc-systems/HPCC-Platform

To remove all the other commits from your PR. I suspect your local candidate-9.2.x branch is out of sync with the hpcc-systems branch.

@rrao4
Copy link
Contributor Author

rrao4 commented Aug 17, 2023

Closed, new PR #17687 has been created with the correct base and same changes.

@rrao4 rrao4 closed this Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@g-pan g-pan g-pan approved these changes

@xwang2713 xwang2713 xwang2713 approved these changes

@gfortil gfortil Awaiting requested review from gfortil

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rrao4 @xwang2713 @ghalliday @g-pan @jakesmith @GordonSmith @kenrowland @richardkchapman