Skip to content

README: add customize logic section#20

Merged
hongchaodeng merged 2 commits intooperator-framework:masterfrom
hongchaodeng:doc
Feb 20, 2018
Merged

README: add customize logic section#20
hongchaodeng merged 2 commits intooperator-framework:masterfrom
hongchaodeng:doc

Conversation

@hongchaodeng
Copy link
Copy Markdown
Contributor

No description provided.

@hongchaodeng
Copy link
Copy Markdown
Contributor Author

Laying out the workflow of "customization" section.
I'm trying out the memcached application myself to fill out more details.

Comment thread README.md Outdated
Func: KubeApplyFunc,
})

if !obj.Spec.CreateService {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Create is a verb, which should not exist in a declarative config. WithService or service is better.

@hongchaodeng hongchaodeng changed the title [WIP] README: add customize logic section README: add customize logic section Feb 17, 2018
@hongchaodeng
Copy link
Copy Markdown
Contributor Author

Finished.
Ready for review.

@xiang90
Copy link
Copy Markdown

xiang90 commented Feb 17, 2018

lgtm as initial pass

Comment thread README.md Outdated

An operator is used to extend the kube-API and codify application domain knowledge. Operator SDK is designed to provide a way for non-Kubernetes developers to write the business logic in operators easily.

In the following we are customizing the operator logic so that once a new `PlayService` is created it will deploy a Memcached Deployment and (optional) Service.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why play service? It seems random and informal. I would rather use example or just memcached

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any suggestion?

use example or just memcached

The issue is we are trying to avoid duplicate naming and confusion. "example" is already used in "example.com". "memcached" is too specific for default API. Or we can add a section to change the API?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

all the default naming should be derived from user input not a fixed one. Or it won’t make any sense for anyone to even start a project with default options.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So what are the suggestions? Can you be more specific given what we have?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@hongchaodeng I do not have any immediate suggestion. I am just saying what we have right now does not make sense to me.

What I suggested in the google doc is that we should have a sane default not to have a random or unusable default. Having a bad default is even worse than not having a default.

A good default is what most people would adopt it without changing anything. That is why good defaults get derived from a limited number of user inputs or environments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A good default is what most people would adopt it without changing anything.

I agree. The current PlayService doesn't reflect good enough on memcached deployment. Let me make some changes to the workflow.

Comment thread README.md Outdated
Generate a new project:

```
operator-sdk new memcached-operator --api=cache.example.com/v1:Memcached
Copy link
Copy Markdown

@xiang90 xiang90 Feb 19, 2018

Choose a reason for hiding this comment

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

it seems to me that we should enforce the project name to be <domain name>/<operator-name>

in this example, it should be operator-sdk new cache.example.com/memcached.

the default versioning should be alpha1 which can be overwritten by flag options. similarly, other default options should be derived from the project name and can be overwritten by flag options.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

in this example, it should be operator-sdk new cache.example.com/memcached.

But that's not projects are structured. For example, we have coreos/etcd-operator. If we want to comply with this convention, it would be etcd.database.coreos.com/etcd-operator. IMHO, this would make user's life harder to manage the project directory if we couple the naming and the API.

Secondly, this is an example to explicitly show how to API group, version, kind, which are needed to start an operator project. If we have too many defaults that it is less useful for users to customize.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i do not like ask users to provide duplicated information to start with. but if we cannot figure out good defaults, then there should be no defaults. all options should be required as what @fanminshi proposed initially.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@hongchaodeng

also in your example user should do

operator-sdk new database.coreos.com/etcd-operator

which i think includes all information we need to lay out the project initially. Basically all we need is the domain name + operator name. i do not know why we need other information.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

then there should be no defaults

I think this is the way to go. By default we should only have minimal common things. We will generate additional things like API, handlers afterwards.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what is the reason to have separate phases?

This would be cleaner to have separate workflow to handle separate things, generating new custom resources, generate new handlers, etc. instead of coupling everything to the single "new" generating scaffolding command.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

operator-sdk new database.coreos.com/etcd-operator

Do you mean that this would generate project folder 'etcd-operator', with apigroup "database.coreos.com", and kind "etcd" by default?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This would be cleaner to have separate workflow to handle separate things, generating new custom resources, generate new handlers, etc. instead of coupling everything to the single "new" generating scaffolding command.

why it is cleaner? it might be cleaner to the developer but may be tedious for the users. from users perspective, they probably want to have one click experience.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do you mean that this would generate project folder 'etcd-operator', with apigroup "database.coreos.com", and kind "etcd" by default?

yes. also version alpha by default. that is what most users would expect i guess.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK. I see your point now. Let me think about it.

@hongchaodeng
Copy link
Copy Markdown
Contributor Author

Rewritten to reflect the discussion on #20 (comment).

PTAL

Comment thread README.md Outdated

```
operator-sdk new play-operator
operator-sdk new cache.example.com Memcached
Copy link
Copy Markdown

@xiang90 xiang90 Feb 20, 2018

Choose a reason for hiding this comment

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

why not cache.example.com/Memcached? it is easier to parse and more intuitive when we explain that the project name has to be the format of <domain>/<name>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For kube API, the convention is group/version, e.g. apps/v1.
This would avoid the confusion.
We also preserve the ability to specify version later in conventional way cache.example.com/v1.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

when would a user create a new project with version v1 instead of v1alpha1?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what i proposed is to ask user for a single project name. and we try to derive a set of sane default options from that single string.

if we do want to provide the max flexibility and do not want to abstract anything. i would perfer what @fanminshi suggested initially by asking from all options without any defaulting.

the approach you just proposed with separating the api group and CR name with a space seems unintuitive to me.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what i proposed is to ask user for a single project name.

There is one issue -- we can't infer apigroup from the project name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The initial proposal is:

new <project-name> <api-group> <kind>

We basically coalesce <project-name> and <kind>. <api-group> is not something we can easily infer.

Copy link
Copy Markdown

@xiang90 xiang90 Feb 20, 2018

Choose a reason for hiding this comment

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

There is one issue -- we can't infer apigroup from the project name.

why not? you ask users to put that into the project name. project name is basically an abstraction over whatever you want it to be. i already provided an example project name (cache.coreos.com/memcached). it should include everything we ever need with the new command. i mentioned a few times that it is OK to assume the version is always v1alpha1. or you can make the project name to be cache.coreos/v1/memcached. the operator name would be memcached-operator.

again, if we do not want to have the abstraction, i would prefer what @fanminshi proposed initially to require everything without any defaulting/inference or any abstraction.

i failed to see the benefits with the middle ground approach you proposed since it is kind of half done.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@xiang90 @hasbro17 @fanminshi
Do you have any suggestion to solve the problem? I really can't think of any ways to reduce parameters.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you ask users to put that into the project name.

What project name? memcached-operator?

Let's go back to the initial plan:

new <project-name> <api-group> <kind>

e.g.

new memcached-operator cache.example.com Memcached

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Let's define project name is the repo name.

@hongchaodeng
Copy link
Copy Markdown
Contributor Author

After discussion, here are the conclusions:

  • There seems no good "operator project" concept by now. new generator needs API speficied explicitly.
  • --apigroup and --kind flags are added. They are required flags for naming purposes.

Comment thread README.md Outdated

```
kubectl create -f deploy/play-operator/operator.yaml
kubectl create -f deploy/memcached-operator/operator.yaml
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i would like to see operator-sdk up or something that will setup a local minikube cluster. but we can do that later on.

Copy link
Copy Markdown
Contributor Author

@hongchaodeng hongchaodeng Feb 20, 2018

Choose a reason for hiding this comment

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

We will discuss that tomorrow.

Ideally, a workflow like this would be helpful to users:

  • Check if $HOME/.kube/config or $KUBECONFIG exists.
  • If not, minikube start
  • Then deploy operator.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@xiang90 I think something like operator-sdk up is a good idea. I have seen this type of command in many web framework. we can do that later.

Comment thread README.md Outdated

```
operator-sdk new play-operator
operator-sdk new memcached-operator --apigroup=cache.example.com/v1alpha1 --kind=Memcached
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

By linux convention, flags are optional. So if we want to enforce user to input apigroup and kind, we should use input args instead.

for example,
operator-sdk new [project-name] [api-group] [kind]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

i am fine with either way.

@fanminshi
Copy link
Copy Markdown
Contributor

lgtm on initial pass. we can have additional improvement on this doc later.

Comment thread README.md Outdated

An operator is used to extend the kube-API and codify application domain knowledge. Operator SDK is designed to provide non-Kubernetes developers an easy way to write the business logic.

In the following we are adding a custom resource `Memcached`, and customizing the operator logic that creating a new Memcached CR will create a Memcached Deployment and (optional) Service.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the following steps we are . . .

Comment thread README.md Outdated

In the following we are adding a custom resource `Memcached`, and customizing the operator logic that creating a new Memcached CR will create a Memcached Deployment and (optional) Service.

In `pkg/apis/cache/v1alpha1/types.go`, Add to `MemcachedSpec` a new field `WithService`:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add ==> add

Comment thread README.md Outdated
bar
```

Now we have successfully to customize the event handling logic to deploy Memcached service for us.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

successfully to customize ==> successfully customized

@hasbro17
Copy link
Copy Markdown
Contributor

LGTM. I'll need to update the Handler code a bit once I'm done implementing the sdk Actions.

@hongchaodeng hongchaodeng merged commit 8e25fb2 into operator-framework:master Feb 20, 2018
@hongchaodeng hongchaodeng deleted the doc branch February 20, 2018 19:07
m1kola pushed a commit to m1kola/operator-sdk that referenced this pull request Jun 7, 2024
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.

4 participants