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

Allow CR with custom dashboard image that is different from general custom image or default image #177

Conversation

aarontams
Copy link
Contributor

@aarontams aarontams commented Jun 5, 2022

Task Allow config the OpenSearchCluster CR to use a specific image for the dashboards only

Limit Currently, the OpenSearchCluster CR only allows customize the dashboard image through the spec.general.image. But the CR value in spec.general.image will also be used by all the opensearch cluster nodes (master, ingest, and data).

Scenario Some users build the opensearch cluster (a.k.a ES cluster) and opensearch dashboard (a.k.a Kibana) separately and put the images in two different docker registry repos. They need a way to individualize both images for opensearch cluster and opensearch dashboard.

Solution To allow the CR to customize the dashboard image only without affecting other components, the simplest way is to introduce an ImageSpec struct within spec.dashboards. For example:

spec:
  dashboards:
    imagePullPolicy: Always
    image: my.io/awesomeapps/opensearch-dashboard:3.2.1-canary

Implementation With this MR, the opensearch operator will choose the dashboard image in the following order:

  1. CR value inspec.dashboards.image if specified.
  2. CR value in spec.general.image if specified.
  3. Default hard coded docker.io/opensearchproject/opensearch-dashboards: + cr.Spec.Dashboards.Version in reconcile-helpers.go.

TODO: Add other useful fields. apiVersion, kind, uid?'
type: string
type: object
type: array
Copy link
Contributor Author

@aarontams aarontams Jun 5, 2022

Choose a reason for hiding this comment

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

When I did the docket-build, as expect the opensearch-operator/config/crd/bases/opensearch.opster.io_opensearchclusters.yaml file was updated.

My question is even before my PR, the updated opensearch.opster.io_opensearchclusters.yaml content is different from charts/opensearch-operator/templates/opensearchclusters.opensearch.opster.io-crd.yaml
I suspected they should be the same but theopensearchclusters.opensearch.opster.io-crd.yaml is not updated every time when the CR struct is changes.

See another PR: https://github.com/Opster/opensearch-k8s-operator/pull/175/files

I am not sure this snippet is needed for PR or not. I can remove it if it is not needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are currently working on automatically updating the CRD in the helm chart. But you can keep the change in.

return
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this code snippet is used 3 times in this go file, refactoring the code to call useCustomImage func for easy maintenance going forward.

@aarontams aarontams force-pushed the add-custom-dashboard-only-image-support branch from 07f315f to a4c7f25 Compare June 5, 2022 15:33
@aarontams
Copy link
Contributor Author

Reviewers
I am not seeing the spec.general.image (imagePullPolicy, imagePullSecrets) defined in crd.md. I am hesitate to add the new fields from this MR to spec.database. Please advise.

@aarontams aarontams force-pushed the add-custom-dashboard-only-image-support branch 2 times, most recently from 65c530c to 57e1517 Compare June 7, 2022 15:29
Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

Hi @aarontams. Thank you for your PR.
Could you please add a unittest for the new functionality (probably in opensearch-operator/pkg/reconcilers/dashboards_test.go)
and please add the new fields to the crd.md (that the fields for spec.general.image are not there is an omission, in general every field of the CRD should be documented).
Otherwise code looks good to me.

TODO: Add other useful fields. apiVersion, kind, uid?'
type: string
type: object
type: array
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are currently working on automatically updating the CRD in the helm chart. But you can keep the change in.

@aarontams
Copy link
Contributor Author

aarontams commented Jun 9, 2022

Could you please add a unittest for the new functionality (probably in opensearch-operator/pkg/reconcilers/dashboards_test.go)

Will do in next PR update.

and please add the new fields to the crd.md (that the fields for spec.general.image are not there is an omission, in general every field of the CRD should be documented).

I was thinking about adding the new fields to crd.md, but didn't see the ImageSpec defined in GeneralConfig.

Yes, I will add that in next PR update.

Thanks for reviewing my PR.

…ustom image or default image

Signed-off-by: aaron.tam <aaron.tam@oracle.com>
@aarontams aarontams force-pushed the add-custom-dashboard-only-image-support branch from 57e1517 to e430b04 Compare June 9, 2022 07:03
@aarontams
Copy link
Contributor Author

@swoehrl-mw
Updated the PR with

  • new fields in crd.md
  • unittest for the new functionality

Looking forward to having you to review it again.

@aarontams aarontams requested a review from swoehrl-mw June 9, 2022 12:02
Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

LGTM

@swoehrl-mw swoehrl-mw merged commit 824c88a into opensearch-project:main Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants