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

added SupervisorSpec CRD #61

Closed

Conversation

applike-ss
Copy link

@applike-ss applike-ss commented May 11, 2023

Fixes druid-io/druid-operator#313

Description

We're currently managing our supervisor specs as json files, however that does not seem to be ideal - especially in a Infrastructure as Code world where having it applied should only be one command away.

To make our Supervisor specs available for IaC tools, i have crafted this change-set to allow creating SupervisorSpec CRs and apply them to the cluster in the defined state.
This is my first try to hack some support of any kind into an operator in kubernetes, so there may be some things done plain wrong and wrong terminology used in code.

This Change was done in a hackathon under time pressure, so please bear with me as the code will not be perfect.
Let me know what you think anyways.

I'll rebase this at some point, for now i just wanted to make sure to create this PR.


This PR has:

  • been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
  • been tested on a real K8S cluster to ensure creation/modification/removal of a SupervisorSpec 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
  • CRD added (SupervisorSpec)
  • .tool-versions file added for asdf support
  • added zookeeper/mysql to helm-install-druid-operator make job for easier one-shot installs (may be removed before merge)
  • added basic cluster.yaml which was used for testing (may be removed before merge)
  • SupervisorSpec management code

@AdheipSingh
Copy link
Contributor

@applike-ss IMHO you can add your PR into the workflow design here .
Also can you point to the custom resource example for the supervisor spec.

For batch, supervisor, sql we plan to have a single controller which can support all of them.

@applike-ss
Copy link
Author

I've added my example here: 342a330

@applike-ss
Copy link
Author

If i get your CR and code right, then your change is for one-time tasks while mine is for supervisor specs, so long-running stateful tasks. Is this correct?

What do you mean with adding my PR into the workflow design "there"? Do you mean i should adjust my PR description to match the structure of yours?

@AdheipSingh
Copy link
Contributor

@applike-ss thank you. This PR is really helpful, and looking at the CR we are very much aligned.

Lets sync on druid-operator slack for faster iteration.
You can join #druid-operator channel on kubernetes slack.

.tool-versions Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!
Copy link
Collaborator

Choose a reason for hiding this comment

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

please remove this and the below unnecessary comments

Copy link
Author

Choose a reason for hiding this comment

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

This does exist for the druid spec types as well, should i remove those too?

chart/values.yaml Outdated Show resolved Hide resolved
deploy/operator.yaml Outdated Show resolved Hide resolved
cluster.yaml Outdated Show resolved Hide resolved
supervisor-spec.yaml Outdated Show resolved Hide resolved
@itamar-marom
Copy link
Collaborator

@applike-ss Are you up to finishing this? This PR can be useful.

@applike-ss
Copy link
Author

@applike-ss Are you up to finishing this? This PR can be useful.

There's no doubt i would like to finish it up, but i will need to schedule this with my lead. I'm doing this within business hours.

@applike-ss
Copy link
Author

applike-ss commented Jul 21, 2023

I am currently really packed with work and will not be able to finish this in the upcoming month(s).

EDIT: I will get time to work on this in the upcoming weeks for some time.

@itamar-marom itamar-marom marked this pull request as draft July 21, 2023 10:02
@applike-ss
Copy link
Author

FYI: i am proceeding with this PR. Currently setting up my test env and trying to get my PR back into a working state as some things have changed meanwhile

@AdheipSingh
Copy link
Contributor

@applike-ss i would like to discuss the design with you. Please schedule a call on my calendly anytime. I would want to have some core changes in the spec definition.

@applike-ss
Copy link
Author

IDK if it makes sense to sync currently.
i have brought it back in a working state now and removed the status "InSync", as it was hard to keep working and might easily break in the near future.
My employer did only want to grant me 1-2 days of dev time which i used 1 day already to create a somewhat working local test env.
So my fear is that we sync now and waste time because i can't implement everything in time.
I will try to finish this in the future, but that might be rather months than weeks.

@AdheipSingh
Copy link
Contributor

we have ingestion controller now. feel free to contribute to it.
thanks

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.

Proposal: Ingestion Spec controller
3 participants