-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add Spark Operator into Theia Helm chart #31
Conversation
1d62394
to
6b9f279
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.
Thanks Yanjun working on this, helm support will simplify the deployment and usage steps for users of policy recommendation feature. So I think this PR could merge first before the implementation PR.
One thing I'm not sure here is whether let Spark Operator be installed by default.
@@ -99,3 +99,13 @@ grafana: | |||
- pod_to_external_dashboard.json | |||
- node_to_node_dashboard.json | |||
- networkpolicy_dashboard.json | |||
sparkOperator: | |||
# -- Install the Spark Operator. It is required to run Network Policy Recommendation jobs. | |||
enable: false |
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.
Could we enable the spark operator by default?
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.
We can do that. If so, do we also update our flow-visibility.yaml to include the Spark Operator? With this, the YAML file users need to apply spark-operator-crds.yaml together with the clickhouse operator before they apply flow-visibility.yaml.
Would also like to hear from @salv-orlando about whether to include it by default.
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.
If so, do we also update our flow-visibility.yaml to include the Spark Operator? With this, the YAML file users need to apply spark-operator-crds.yaml together with the clickhouse operator before they apply flow-visibility.yaml.
I prefer not to include the Spark Operator in flow-visibility.yaml
, because we didn't include the clickhouse operator crd either.
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.
The Spark Operator case is a little bit different comparing the ClickHouse Operator, as besides CRDs, it contains some Deployment and Service Account.
If we do not include the Spark Operator in flow-visibility.yaml
, there will be a gap that by default, Helm install includes the Spark Operator, but flow-visibility.yaml
does not. If the YAML file users would like to enable the Spark Operator, they need apply spark-operator-crds.yaml and generate a flow-visibility.yaml
with Spark Operator Deployment by running ./hack/generate_manifest.sh --spark-operator
.
I'm not sure whether this may make users confused.
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.
Got it, if so let's keep consistent with current steps. If we are moving forward to include spark operator in helm by default, let's include the spark operator deployments in flow-visibility.yaml and remind yaml file users apply the crd of both clickhouse and spark operator before applying flow-visibility.yaml
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.
my take is that flow-visibility.yaml
should be for users that install Antrea the "hard way". These would likely more be users that try to get a good understand of how the various pieces work and integrate each other.
So I do not see as a must having the spark operator in flow-visibility.yaml. But being able to add it won't do any harm.
Every other user would like to install Antrea (and manage it after installation) in the simplest possible way, and they will likely go for helm charts.
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.
Got it, also according to our discussion in #36, it's ok to let users set mandatory parameters in values.yaml
before running helm install
. We could keep sparkOperator enable: false
here by default.
6b9f279
to
7a110bc
Compare
This PR will introduce some conflicts with PR #25 and PR #27 . The later merged one should take care of the conflicts.
|
7a110bc
to
242748a
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.
overall LGTM
hack/generate-manifest.sh
Outdated
@@ -24,6 +24,7 @@ _usage="Usage: $0 [--mode (dev|release)] [--keep] [--help|-h] | |||
Generate a YAML manifest for the Clickhouse-Grafana Flow-visibility Solution, using Helm and | |||
Kustomize, and print it to stdout. | |||
--mode (dev|release) Choose the configuration variant that you need (default is 'dev') | |||
--spark-operator Generate a manifest with spark-operator support enabled |
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 feel it would be better to highlight that only the Deployment and Service Account will be included in the generated yaml. The crd part will not be included/need to separately apply spark-operator-crds.yaml etc.
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.
Thanks. I added some descriptions, but still feel it might be strange as we do not mention that the ClickHouse CRDs are required before applying flow-visibility in this script.
Actually, I'm considering about injecting the spark-operator-crds.yaml into flow-visibility.yaml with Kustomize in this script when using --spark-operator
option. As the installation order does not matter, this might make the deployment easier. Would like to hear some suggestions.
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'm considering about injecting the spark-operator-crds.yaml into flow-visibility.yaml with Kustomize in this script when using --spark-operator option.
I think it's a good idea
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'm considering about injecting the spark-operator-crds.yaml into flow-visibility.yaml with Kustomize in this script when using --spark-operator option.
Sounds good to me.
but still feel it might be strange as we do not mention that the ClickHouse CRDs are required before applying flow-visibility in this script.
Similar cases with the above one, since we already included the ClickHouse deployment in flow-visibility.yaml
, what about include ClickHouse CRDs inside flow-visibility.yaml
?
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.
We cannot include ClickHouse CRDs inside flow-visibility.yaml
, as ClickHouse CRDs must be applied before we use the Kind ClickHouseInstallation
.
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 did not follow the story of this PR too much, so I'm not sure if there is still any work in progress. For what is worth, I think it's good enough to merge
This PR is ready for review. It can be merged if no more comment, |
1c4d8ed
to
eb1a7d9
Compare
Signed-off-by: Yanjun Zhou <zhouya@vmware.com>
eb1a7d9
to
317ef97
Compare
This PR adds Spark Operator into Theia Helm chart. Users can install Spark Operator to run our Network Policy Recommendation jobs by enable the spark operator in Theia Helm chart.
Signed-off-by: Yanjun Zhou zhouya@vmware.com