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

Fix CRD to support PVC name and labels. Also adding annotations to services. #152

Conversation

lum-splunk
Copy link

@lum-splunk lum-splunk commented Mar 25, 2021

Fixes #135

Description

This PR should resolve the issue mentioned in #135. The PVC name will get prepended to the naming scheme, so that we don't end up with PVC names like "-druid-druid-historicals-0", which are invalid.

This PR also adds the ability to add annotations to services. My main rationale for this was to allow for the creation of internal AWS ELBs with annotations such as:

    service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval: "5"

    service.beta.kubernetes.io/aws-load-balancer-healthcheck-timeout: "3"

    service.beta.kubernetes.io/aws-load-balancer-healthcheck-unhealthy-threshold: "2"

    service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0

This PR has:

  • been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
  • been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Key changed/added files in this PR
  • druid.apache.org_druids.yaml

Also add annotations to services.
@AdheipSingh
Copy link
Contributor

Great !! Ill test it out, i have been using without openAPI for a while so dint come across this issue.
Will check out with this CRD.

cc @himanshug

@dsx
Copy link
Contributor

dsx commented Mar 26, 2021

File chart/templates/crds/druid.apache.org_druids.yaml needs to be updated too.

@AdheipSingh
Copy link
Contributor

Plus once updated in helm , we also need to bump chart version.

@dsx
Copy link
Contributor

dsx commented Mar 26, 2021

And not forget about helm annotation to leave CRD upon uninstall by default. I thinks at this point it would be safer to employ CI to do that, otherwise things will be forgotten.

@himanshug
Copy link
Member

himanshug commented Mar 27, 2021

thanks @lum-splunk , but unfortunately this doesn't work because CRD isn't hand written but generated. this will get over-written immediately.... CRD generation is controlled via marker comments added to https://github.com/druid-io/druid-operator/blob/master/apis/druid/v1alpha1/druid_types.go , so right markers would need to be added there so that generated CRD matches what is proposed here.

@AdheipSingh
Copy link
Contributor

agree with @himanshug
make run will omit the changes anyways, so we need to add comments in types.go, code-gen will use those for crd generation

@lum-splunk
Copy link
Author

@himanshug @AdheipSingh ok, I can make the updates as requested.

After making changes locally, how can I generate the CRD to test its working correctly?

@himanshug
Copy link
Member

@lum-splunk make manifests would re-generate the CRD.

@y25ean
Copy link

y25ean commented Apr 16, 2021

I am currently experiencing this issue, when do you think that this pull request will be merged?

@AdheipSingh AdheipSingh mentioned this pull request Apr 16, 2021
4 tasks
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.

Wrong pvc for middle manager
5 participants