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

doc: Add design document #253

Merged
merged 2 commits into from
May 2, 2018

Conversation

asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Apr 23, 2018

Explains:

  • Basic operations
  • Custom versioning
  • Sub-resources
  • peer-watch-file ConfigMap

Signed-off-by: Lorenzo Manacorda lorenzo@kinvolk.io

Closes #253.

doc/design.md Outdated

The `ConfigMap` is maintained by the operator in the following ways:

* When it's empty, it populates it with the IP of one of the running `Pod`s (the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I would drop the notion of a leader here - I'm afraid it will be too confusing for Habitat users, because the leader you mention has nothing to do with any leader stuff in Habitat (neither with leader in service group with leader-follower topology nor with update leader in the service group).

doc/design.md Outdated

* When it's empty, it populates it with the IP of one of the running `Pod`s (the
so called "leader")
* Whenever a leader dies, the operator replaces the IP in the `ConfigMap` with
Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever the Pod dies

doc/design.md Outdated
* When it's empty, it populates it with the IP of one of the running `Pod`s (the
so called "leader")
* Whenever a leader dies, the operator replaces the IP in the `ConfigMap` with
that of another running Pod, which becomes the new leader
Copy link
Contributor

Choose a reason for hiding this comment

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

Pod in backticks?

doc/design.md Outdated

Once the operator is running, it performs the following actions:

* watches for new `Habitat` manifests and deploys corresponding sub-resources
Copy link
Contributor

Choose a reason for hiding this comment

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

In bullet points I prefer to start with a capital letter and end with a full dot.

You may have a different preference, but please make it at least consistent.

doc/design.md Outdated

## Sub-resources

As mentioned above, the operator creates several resources whenever it detects a
Copy link
Contributor

Choose a reason for hiding this comment

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

That makes me think that maybe we should put a label on all the resources we create, so we could tell the users in this document how to recognize a resource that is managed by the operator.

I think we don't put any label on on Deployments and StatefulSets. We do it for ConfigMaps and Pods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do for Deployments and StatefulSets as well.

I'll add a mention of it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a pod template spec, so the pods are labeled. I rather expected the label to be also here for Deployments and here for StatefulSets.

Anyway, I checked it with kubectl and it indeed seems like Deployments and StatefulSets are labeled too. That's a bit magical.

@asymmetric
Copy link
Contributor Author

PTALA.

Copy link
Contributor

@indradhanush indradhanush left a comment

Choose a reason for hiding this comment

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

Mostly nits and some doubts.

`ConfigMap`s, `Secret`s, `PersistentVolume`s) according to its definition.

Another example is changing the Docker image inside a Habitat object. In this
case, the operator first goes to all `StatefulSet`s it manages and updates them
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Statefultsets/Statefulsets makes it easier to read in the rendered version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it's wrong !!! ;)


Another example is changing the Docker image inside a Habitat object. In this
case, the operator first goes to all `StatefulSet`s it manages and updates them
with the new Docker images; afterwards, all `Pod`s from each `StatefulSet` are
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for "Pods" and other similar places.

doc/design.md Outdated
In a non-Kubernetes Habitat setup, every service in a group after the first one
is [started][hab-sg] with the `--peer` flag, pointing to the first service's IP.

Since it's not possible in Kubernetes to launch some members with different
Copy link
Contributor

Choose a reason for hiding this comment

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

members of?

doc/design.md Outdated
The `ConfigMap` is maintained by the operator in the following ways:

* When it's empty, it populates it with the IP of one of the running `Pod`s
* Wenever a `Pod` dies, it makes sure that the IP in the `ConfigMap` is still
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in "Wenever".

doc/design.md Outdated

The `ConfigMap` is maintained by the operator in the following ways:

* When it's empty, it populates it with the IP of one of the running `Pod`s
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a line or two on how the operator decides which Pod should become leader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no leader no more! :) (@krnowak was right that it would confuse people)

doc/design.md Outdated
considered read-only; any modifications to them will be overwritten by the
operator.

The proper way to make changes is **always** to make them on the Habitat object,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "is to always make them"

doc/design.md Outdated
operator.

The proper way to make changes is **always** to make them on the Habitat object,
and let the operator figure out the actual steps it needs to take.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not very clear to me as someone unfamiliar with the operator. When you say make changes on the Habitat object what are other ways by which things can be changed? Do you mean changing the Pod resource for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the point of this section. Is that not clear enough? Suggestions?

doc/design.md Outdated
According to [Semantic Versioning][semver], backwards incompatible changes require
releasing a new major version. Unfortunately, it is [currently not
possible][crd-vers] to run multiple versions of a CRD side-by-side. The
work-around currently relies on two steps:
Copy link
Contributor

Choose a reason for hiding this comment

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

"work-around" should be one word.

Copy link
Contributor

@krnowak krnowak left a comment

Choose a reason for hiding this comment

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

Some typos.

doc/design.md Outdated
is [started][hab-sg] with the `--peer` flag, pointing to the first service's IP.

Since it's not possible in Kubernetes to launch `Pod`s in a
`Deployment`/`StatefulSet`with different arguments, the operator resorts to
Copy link
Contributor

Choose a reason for hiding this comment

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

Space between "StatefulSet" and "with".

doc/design.md Outdated

* When it's empty, it populates it with the IP of one of the running `Pod`s
(which `Pod` gets chosen is irrelevant, the important thing is that
the`Pod` is in the `Running` state)
Copy link
Contributor

Choose a reason for hiding this comment

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

Space between "the" and "Pod".

Copy link
Contributor

@krnowak krnowak Apr 25, 2018

Choose a reason for hiding this comment

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

This one is not fixed.

doc/design.md Outdated
possible][crd-vers] to run multiple versions of a CRD side-by-side. The
workaround currently relies on two steps:

* defining an additional field, `customVersion`
Copy link
Contributor

Choose a reason for hiding this comment

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

Capitalize "defining".

@asymmetric
Copy link
Contributor Author

PTALAA.

@asymmetric
Copy link
Contributor Author

I will squash the hell out of this PR.

@indradhanush
Copy link
Contributor

@krnowak
Copy link
Contributor

krnowak commented Apr 27, 2018

LFAD, but squash some commits first, please.

Lorenzo Manacorda added 2 commits April 30, 2018 13:15
Explains:

* Basic operations
* Custom versioning
* Sub-resources
* `peer-watch-file` `ConfigMap`

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>
`doc/` should be the home for all documentation, excluding the one that
has a place in the root directory (usually files with CAPITALIZED names)

Signed-off-by: Lorenzo Manacorda <lorenzo@kinvolk.io>
@krnowak krnowak merged commit d6ec488 into habitat-sh:master May 2, 2018
@asymmetric asymmetric mentioned this pull request May 2, 2018
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.

3 participants