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-24970 Sasha K8s services. #14536
HPCC-24970 Sasha K8s services. #14536
Conversation
https://track.hpccsystems.com/browse/HPCC-24970 |
78aec5e
to
243c26e
Compare
|
6fe1503
to
59791f2
Compare
@wangkx - please review, in particular, the esp service changes to pick up the sasha service. This will relate to: https://track.hpccsystems.com/browse/HPCC-24968 |
@ghalliday - please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple of minor issues
59791f2
to
c2a855e
Compare
@richardkchapman - removed spurious changes to testing/helm/run.sh |
c2a855e
to
8144da1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakesmith looks good. A few minor comments.
{{- include "hpcc.addSashaVolumes" (dict "root" $ "me" .) | indent 6 }} | ||
{{- end }} | ||
{{- end }} | ||
{{ include "hpcc.addDaliVolume" $commonCtx | indent 6 }} | ||
{{ include "hpcc.addSecretVolumes" (dict "root" $ "categories" (list "system" ) ) | indent 6 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will likely need to include storage secrets (e.g. for xref, and possibly other uses)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will depend on the services included in Dali's pod.
The expectation is that it will just be the coalescer in the Dali pod, but if it's e.g. the archiver, or file-expiry, or xref, then it would need "storage" too. So I should detect and make conditional.
Will fix.
@ghalliday - please see new commit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakesmith a few minor comments. I'm not confident I have reviewed the helm files fully.
{{ include "hpcc.addSecretVolumeMounts" (dict "root" $ "categories" (list "system" ) ) | indent 8 }} | ||
{{- range .services -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an entry in the config for dali (and all sasha config files) that contains the vault information. Mentioned in Shamser's PR as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are no secrets/vaults generated for Dali in master at the moment.
I don't think Dali has needed any, until now with introduction of sasha services that may.
I can wait though for Shamser's PR to be merged, so it doesn't clash etc.., then use a common list.
6005f93
to
b7094b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think good from my perspective, but I would like someone else's (@richardkchapman ?) comments on the Helm changes.
{{ include "hpcc.addSecretVolumes" (dict "root" $ "categories" (list "system" ) ) | indent 6 }} | ||
{{- if or (has "data" .access) (has "dalidata" .access) -}} | ||
{{ include "hpcc.addSecretVolumes" (dict "root" .root "categories" (list "storage" ) ) | indent 6 -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should really be using a single call with a conditional list. If everything else is ok, then it can be merged and @shamser fix it in his PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakesmith one comment. I think ready for squashing and a final review.
Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>
c71be55
to
c0faec3
Compare
@ghalliday - squashed and rebased to latest master. |
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
Signed-off-by: Jake Smith jake.smith@lexisnexisrisk.com
Type of change:
Checklist:
Smoketest:
Testing: