8000 Remove redundant code form helmer by roytman · Pull Request #1561 · fybrik/fybrik · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove redundant code form helmer #1561

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
Merged
merged 2 commits into from
Jun 19, 2022
Merged

Remove redundant code form helmer #1561

merged 2 commits into from
Jun 19, 2022

Conversation

roytman
Copy link
Collaborator
@roytman roytman commented Jun 18, 2022 •
edited
Loading
  • Remove repetitive calls of getConfig
  • Remove some redundant code
  • Several addition helm related calls optimizations.

Signed-off-by: Alexey Roytman roytman@il.ibm.com

@roytman roytman force-pushed the helm2 branch 2 times, most recently from 5d7e5e0 to c03012e Compare June 18, 2022 18:55
@roytman roytman changed the title Remove redundant code form the helmer Remove redundant code form helmer Jun 18, 2022
@roytman roytman force-pushed the helm2 branch 5 times, most recently from 90e0657 to b98698b Compare June 18, 2022 20:13
@roytman roytman requested review from revit13 and shlomitk1 June 18, 2022 20:53
@roytman
Copy link
Collaborator Author
roytman commented Jun 18, 2022

@shlomitk1 @revit13 , I removed some redundant helm related code.
Could you please take a look that I don't miss something.

@roytman
Remove redundant code form the helmer
c85403b 
Signed-off-by: Alexey Roytman <roytman@il.ibm.com>
shlomitk1
shlomitk1 reviewed Jun 19, 2022
View reviewed changes
manager/controllers/app/blueprint_controller.go Outdated
func (r *BlueprintReconciler) applyChartResource(ctx context.Context, log *zerolog.Logger, chartSpec fapp.ChartSpec,
args map[string]interface{}, blueprint *fapp.Blueprint, releaseName string) (ctrl.Result, error) {
func (r *BlueprintReconciler) applyChartResource(ctx context.Context, cfg *action.Configuration, chartSpec fapp.ChartSpec,
args map[string]interface{}, releaseNameSpace, releaseName string, log *zerolog.Logger) (ctrl.Result, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

releaseNameSpace -> releaseNamespace
Can't it be extracted from the config?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  • releaseNameSpace -> releaseNamespace
    OK
  • Can't it be extracted from the config?
    yes and no. It is stored in one of implementations of kube.Interface when it is Kubernetes.Client, it's available there, but there are 2 other implementations...

shlomitk1
shlomitk1 reviewed Jun 19, 2022
View reviewed changes
Copy link
Contributor
@shlomitk1 shlomitk1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A couple of comments:

  1. In the manager deployment we have an option to deploy the manager pod with a pre-configured chart. See

    fybrik/charts/fybrik/templates/manager-deployment.yaml

    Lines 142 to 145 in 1c1529b

    {{- if .Values.manager.chartVolume }}
    - mountPath: /opt/fybrik/
    name: charts
    {{- end }}
    and

    fybrik/charts/fybrik/values.yaml

    Lines 222 to 227 in 1c1529b

    chartVolume: ""
    # Specify a persistent volume claim, to be mounted by the manager, that can contain
    # helm charts that can be referenced by a FybrikModule. Manager will check if a chart
    # is available on volume first, then try to pull from a Docker registry if it does not exist.
    # To populate the volume, create a 'charts' directory at the root of the mount,
    # and place helm charts within the charts directory.

    Either we disable this option, and the deployment yaml is updated accordingly, or we keep it. In the latter case, perhaps it will make more sense to define an env var that will prevent manager from trying to load a local chart if it is not there.

  2. Keeping HELM_DEBUG env variable may be useful for debugging purposes.

  3. Did WAIT option reduce the number of idle status checks? Please check that the timeout has been indeed propagated to the helmer inner functions (I had once problems with that, 0 was used instead of the passed timeout value, so wait basically did nothing...)

  4. Please set DisableOpenAPIValidation to the correct value.

@roytman
Copy link
Collaborator Author
roytman commented Jun 19, 2022

A couple of comments:

  1. In the manager deployment we have an option to deploy the manager pod with a pre-configured chart. See

    fybrik/charts/fybrik/templates/manager-deployment.yaml

    Lines 142 to 145 in 1c1529b

    {{- if .Values.manager.chartVolume }}
    - mountPath: /opt/fybrik/
    name: charts
    {{- end }}

    and

    fybrik/charts/fybrik/values.yaml

    Lines 222 to 227 in 1c1529b

    chartVolume: ""
    # Specify a persistent volume claim, to be mounted by the manager, that can contain
    # helm charts that can be referenced by a FybrikModule. Manager will check if a chart
    # is available on volume first, then try to pull from a Docker registry if it does not exist.
    # To populate the volume, create a 'charts' directory at the root of the mount,
    # and place helm charts within the charts directory.

    Either we disable this option, and the deployment yaml is updated accordingly, or we keep it. In the latter case, perhaps it will make more sense to define an env var that will prevent manager from trying to load a local chart if it is not there.

  2. Keeping HELM_DEBUG env variable may be useful for debugging purposes.

  3. Did WAIT option reduce the number of idle status checks? Please check that the timeout has been indeed propagated to the helmer inner functions (I had once problems with that, 0 was used instead of the passed timeout value, so wait basically did nothing...)

  4. Please set DisableOpenAPIValidation to the correct value.

  1. I'll return this option.
  2. HELM_DEBUG was used by the helmer proprietary logger, now helmer uses the fybrik logger. So it prints log messages according to the fybrik log level.
  3. If you specify WAIT and timeout 0, the install/upgrade operations fails with timeout error. You have to specify wait and timeout. And yes, it reduces number of status checks.
  4. The correct value of DisableOpenAPIValidation is false and it is the default value. It was set to true as a workaround for the memory leak.

@roytman
Return local deployed charts
f76afaf 
Signed-off-by: Alexey Roytman <roytman@il.ibm.com>
@roytman roytman merged commit 5ce7482 into fybrik:master Jun 19, 2022
@roytman roytman deleted the helm2 branch October 11, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@revit13 revit13 revit13 left review comments

@shlomitk1 shlomitk1 shlomitk1 approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@roytman @revit13 @shlomitk1
0