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

Implement persistence with StatefulSets #201

Merged
merged 93 commits into from
Mar 21, 2018
Merged

Implement persistence with StatefulSets #201

merged 93 commits into from
Mar 21, 2018

Conversation

asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Mar 6, 2018

This PR

  • Changes the type of object we create from Deployment to StatefulSet
  • Adds a Persistence key to the Habitat type
  • Starts splitting controller.go into separate modules
  • Removes docs/api.md and just points users to types.go
  • Adds tests for persistence

The main issue with using Deployments for persistence was that there was no automatic way to assign a different PVC to each Pod, so we ended up with Pods that shared the same Volume.

See #44 and #195 for more in-depth discussions.

NOTE: Tests are included but skipped, as they don't work in our current testing environment (it's not possible to provision PersistentVolumes when running minikube with --vm-driver=none).

Closes #44.
Closes #38.
Closes #181.

@asymmetric
Copy link
Contributor Author

@lilic could you run the deepcopy script against this branch, and see if there's any change?

@asymmetric asymmetric changed the title Implement persistence with StatefulSets [WIP] Implement persistence with StatefulSets Mar 6, 2018
@asymmetric asymmetric force-pushed the asymmetric/sts branch 10 times, most recently from b9d4d1c to 0986653 Compare March 7, 2018 15:49
@asymmetric asymmetric changed the title [WIP] Implement persistence with StatefulSets Implement persistence with StatefulSets Mar 7, 2018
@jeremymv2
Copy link

sosweet

Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Left some initial comments, didn't have time to look deeper into this, as it is a large PR.

@@ -34,7 +34,7 @@ rules:
- apiGroups:
- apps
resources:
- deployments
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not need Volume or something similar here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I don't think so, as it's not the operator that's creating Volumes, it's the StatefulSet.

Copy link
Contributor Author

@asymmetric asymmetric Mar 19, 2018

Choose a reason for hiding this comment

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

You were right btw, we did have to: the PV/PVC ListWatchers running in the operator means we need to assign list and watch permissions.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I am guessing as this is not needed anymore the above comment is not valid, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.

@@ -142,6 +151,22 @@ func (in *HabitatStatus) DeepCopy() *HabitatStatus {
return out
}

// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool you got it to work! 👍 How did you manage to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't :) This is your commit actually, from the Deployments PR.

mountPath: /tmp/foobar
env:
- name: HAB_REDIS
value: '{ "protected-mode": "no" }'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a note of warning here.

Persistence *Persistence `json:"persistence,omitempty"`
}

type Persistence struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call this Storage instead? Things make a bit more sense, for example you then have Size, ClassName as part of Storage. WDYT?

Copy link
Contributor Author

@asymmetric asymmetric Mar 12, 2018

Choose a reason for hiding this comment

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

Not sure that Storage by itself makes it clear that this is persistent rather than ephemeral.

Copy link
Contributor

Choose a reason for hiding this comment

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

PersistantStorage? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid you're gonna have to re-run the script though :)

@@ -922,11 +679,20 @@ func habitatKeyFromLabeledResource(r metav1.Object) (string, error) {
func newConfigMap(ip string, h *habv1beta1.Habitat) *apiv1.ConfigMap {
return &apiv1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
// TODO the name should also change to support multiple habitats
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this TODO a bit more?

Copy link
Contributor Author

@asymmetric asymmetric Mar 12, 2018

Choose a reason for hiding this comment

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

Oops, that was just for me, I'll remove. Anyway, I think that making the CM name a constant has the drawback that we can only have one. In the future, this could pose problems if we have more than one ring in the same namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem with is that is how would we handle binding if they are not in one ring. That's why we decided to go with one ring...

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, I didn't mean to say that I thought it through, that's why it was just there for me as a reminder.

h, err := hc.getHabitatFromLabeledResource(d)
if err != nil {
// Could not find Habitat, it must have already been removed.
level.Debug(hc.logger).Log("msg", "Could not find Habitat for StatefulSet", "name", d.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this msg doesn't make much sense to me as a consumer, maybe adding Could not find Habitat resource for StatefulSet would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it just uses the same format as every other object. We don't say "StatefulSet resource" after all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes understand, but it can be confusing for a user as the name conveys two things. :) Anyways, more of a nit for clarity.

@@ -183,3 +187,67 @@ func TestHabitatDelete(t *testing.T) {
t.Fatal(err)
}
}

func TestHabitatPersistence(t *testing.T) {
// We run minikube in a VM on Travis. In that environment, we cannot create PersistentVolumes.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this a bit, what is the reason behind this?

Also if it doesn't work why do we have this test? I am a bit confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is that since we run the operator in containers on Travis, the dynamic volume provisioning that Minikube normally does (when it's on a VM) doesn't work.

I think leaving the test in makes sense because:

  • it took time to write it, so we would lose that by deleting it
  • we can reactivate it when we move away from testing with minikube (which I think we should)
  • it documents that there's a problem and what it is

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally don't like dead code in master, maybe you could just cherry-pick this and keep it in a branch so the work is not lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that if it's marked as skip, then it's fine. It's common for tests to be temporarily marked as skippable IMO. It would be different if it was business logic, but I think it's fine for tests. I'd rather keep this in unless it's blocking for you.

@asymmetric asymmetric force-pushed the asymmetric/sts branch 3 times, most recently from 88654b5 to 66bd8f9 Compare March 12, 2018 16:17
@lilic
Copy link
Contributor

lilic commented Mar 13, 2018

@asymmetric I wanted to try this out but can't seem to compile it locally. But it seems to be running fine on travis. 😕 Does make build work for you?

Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

First true pass over the PR. Left a few comments.

Also don't forget to update the docs/api.md file ;)


q, err := resource.ParseQuantity(ps.Size)
if err != nil {
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we format this error with a prefix, something like "PersistantVolume resource size error: ...".

The reason for this is, until we have the validations in place, the errors this will generate sometimes seem to be very non descriptive.

source := newListWatchFromClientWithLabels(
hc.config.KubernetesClientset.CoreV1().RESTClient(),
"persistentvolumes",
apiv1.NamespaceAll,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be caching all the PV in all the namespaces?

Copy link
Contributor Author

@asymmetric asymmetric Mar 14, 2018

Choose a reason for hiding this comment

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

I guess so, because users can create Habitats (and therefore PVs) in any namespace. Why would this case be different from other resources we watch?

)

hc.pvInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{
DeleteFunc: hc.handlePVDelete,
Copy link
Contributor

Choose a reason for hiding this comment

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

What about on update? Should we react on that as well?

Copy link
Contributor Author

@asymmetric asymmetric Mar 14, 2018

Choose a reason for hiding this comment

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

I think that at least for now, we can avoid implementing storage resize: it's an optional feature that introduces quite a lot of complexity. I'd wait for the changes introduced in this PR to have stabilized a bit (and for someone to request it) before we figure out how to do it best.

But we should definitely make this clear and notify the user somehow, when they change the Size in the manifest, that it's not supported. How to do that though? This is probably one of the things we should add to #200 (and/or to #185?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Then IMO I don't see the value of caching PV informer or rather reacting to the changes. Same for the PVC informer. Clean up of volumes is intentionally manual and the rest is already handled by the StatefulSet. The user is already informed, via monitoring, logs, etc., so I feel like we are just duplicating that here. Especially since we are caching ALL the PVC/PV, that can be a huge number in a normal cluster.

Copy link
Contributor Author

@asymmetric asymmetric Mar 14, 2018

Choose a reason for hiding this comment

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

We are only caching ones with label habitat: true. Also, the point of caching is that they could be removed by anything/anybody else, and we need to be able to notify the user - and most importantly, in the future, update the Status of the Habitat object.

According to the Controller Bible, verse 7:

There are other actors in the system. Just because you haven't changed an object doesn't mean that somebody else hasn't.

Copy link
Contributor

Choose a reason for hiding this comment

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

and we need to be able to notify the user

But that is already done by its respective controller.

Agree that we should react, but we are not doing anything apart from logging. So I don't see the point of it, IMHO. :)

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 will update the Status in #200 :)

} else if !exists {
level.Debug(hc.logger).Log("msg", "No matching Habitat found for PVC", "name", pv.Name)
return
} else {
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 we can get rid of this else as we return in other cases anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think in this case it makes more sense to keep it in in this case, because it's a 3-way situation we're dealing with, and having them together helps (me) map the idea to the code better.

Usually with 2-branch conditionals that's not necessary, but I find that it helps here.

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up agreeing with you :)

@@ -0,0 +1,88 @@
// Copyright (c) 2017 Chef Software Inc. and/or applicable contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

It's 2018 :)

@@ -0,0 +1,69 @@
// Copyright (c) 2017 Chef Software Inc. and/or applicable contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

It's 2018 :)

@@ -0,0 +1,325 @@
// Copyright (c) 2017 Chef Software Inc. and/or applicable contributors
Copy link
Contributor

Choose a reason for hiding this comment

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

It's 2018 :)

@asymmetric
Copy link
Contributor Author

asymmetric commented Mar 14, 2018

About the docs/api.md file: is it useful? I keep forgetting to keep it in sync, and I'd like to know if there's a good reason to do that.

Can't we just have a link to pkg/apis/habitat/v1beta1/types.go? Fields are commented there as well.

@lilic
Copy link
Contributor

lilic commented Mar 14, 2018

About the docs/api.md file: is it useful? I keep forgetting to keep it in sync, and I'd like to know if there's a good reason to do that.

I just find the documentation helpful when needing to reference something, but we could also implement a helper script for that? Or just get rid of it, up to you. :)

@@ -927,6 +684,14 @@ func newConfigMap(ip string, h *habv1beta1.Habitat) *apiv1.ConfigMap {
Labels: map[string]string{
habv1beta1.HabitatLabel: "true",
},
OwnerReferences: []metav1.OwnerReference{
metav1.OwnerReference{
APIVersion: fmt.Sprintf("%s/%s", habv1beta1.GroupName, habv1beta1.Version),
Copy link
Contributor

Choose a reason for hiding this comment

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

There is h.APIVersion you can use.

OwnerReferences: []metav1.OwnerReference{
metav1.OwnerReference{
APIVersion: fmt.Sprintf("%s/%s", habv1beta1.GroupName, habv1beta1.Version),
Kind: "Habitat",
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here there is h.Kind you can use.

base := &appsv1beta1.StatefulSet{
ObjectMeta: metav1.ObjectMeta{
Name: h.Name,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a OwnerReference to the StatefulSet as well, like above in the ConfigMap.

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 currently do that here, but I agree we could simply delegate this to Kubernetes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Kind: "Habitat",
Name: h.Name,
UID: h.UID,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Also I noticed we do not say anything about BlockOwnerDeletion, I would add this here. It is very useful IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if the owner has the "foregroundDeletion" finalizer

Is this our case?

It's not entirely clear for me if any defaults are defined for the owning resource (Habitat). Otherwise, it seems to me that propagationPolicy could be specified by a user in their DELETE request. Is this something we should be worried about?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if it's something to worry about, just thought it was nice to have.

@asymmetric asymmetric force-pushed the asymmetric/sts branch 2 times, most recently from d58e58f to 3a1151d Compare March 14, 2018 14:42
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.

Nits mostly I think.

hc.pvcInformerSynced = hc.pvcInformer.HasSynced
}

// findHabForPVC looks for a matching Habitat object for a PVC.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd name it rather like checkHabForPVC. findHabForPVC makes me think that it will return a Habitat object.

return
}

_, exists, err := hc.habInformer.GetStore().GetByKey(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

That part of code looks like a copy pasta of the findHabForPVC function. Maybe just call it here?

Copy link
Contributor Author

@asymmetric asymmetric Mar 19, 2018

Choose a reason for hiding this comment

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

Actually, I think there are 2 problems with my approach here:

  • We are not telling users that they PersistentVolumes they create (if they're going with static provisioning) need to have the Habitat=true label
  • We find the Habitat by name, which seems like a weak approach. Maybe a label would be better? Considering that the PV might be created beforehand, it seems weird to force the user to know already what the name should be. A label, OTOH, can be added later, and seems a better approach than asking the user to rename the PV.

ReadOnly: false,
}

base.Spec.Template.Spec.Containers[0].VolumeMounts = append(base.Spec.Template.Spec.Containers[0].VolumeMounts, *secretVolumeMount)
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably do spec := &base.Spec.Template.Spec and then spec.Containers[0].VolumeMounts = … to drop a bit of a repeating the same thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even move this spec := … outside this block, so it could be also used for mounting the persistent volume or the ring secret volume.

habInformerSynced cache.InformerSynced
stsInformerSynced cache.InformerSynced
cmInformerSynced cache.InformerSynced
pvInformerSynced cache.InformerSynced
Copy link
Contributor

Choose a reason for hiding this comment

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

What are pvInformerSynced and pvcInformerSynced for? They seem to be unused. Did you forget to use them in WaitForCacheSync call?

To highlight the "happy path".
Makes it clear what the gola of the method is.
"find" made it seem like the method returned a Habitat object, whereas
we are only interested in the side effects.
The operator runs ListWatchers for PVs and PVCs.
It is a duplicate of cluster-role.yml
The `TypeMeta` field is not populated for the object passed to the
handler, so we need to calculate these two fields ourselves.
The updates serve to clarify that both static and dynamic provisioning
are supported, but that users should read the official documentation in
order to find out how to set them up.
The implementation relied on PVs being labeled with Habitat=true, which
does not happen automatically when PVs are dynamically provisioned.
This reverts commit 5910452.

The changes here were related to PV and PVC watching, which have been
removed in a previous patch.
We are not watching PVs and PVCs anymore in this PR.

This reverts commit 453e05a.
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Few questions. Otherwise think we should be done with this soon and you can iterate on this in the future.

@@ -0,0 +1,39 @@
version: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is now both travis and circleci used? 🤔

@@ -1,39 +0,0 @@
# API v1beta1
Copy link
Contributor

Choose a reason for hiding this comment

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

:( 👋

@@ -34,7 +34,7 @@ rules:
- apiGroups:
- apps
resources:
- deployments
Copy link
Contributor

Choose a reason for hiding this comment

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

So I am guessing as this is not needed anymore the above comment is not valid, right?

@@ -347,15 +274,15 @@ func (hc *HabitatController) enqueueCM(obj interface{}) {
}

func (hc *HabitatController) handleCMAdd(obj interface{}) {
hc.enqueueCM(obj)
hc.handleCM(obj)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is in the scope of this PR or not, but since you are redoing the logic of resources being created or not, I open an issue about CM not being deleted when Habitat resource is deleted, so maybe it's something worth looking into. It can be outside of this PR as well, up to you...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍. I'll tackle it in another PR.

"fmt"

"github.com/go-kit/kit/log/level"
habv1beta1 "github.com/habitat-sh/habitat-operator/pkg/apis/habitat/v1beta1"
Copy link
Contributor

Choose a reason for hiding this comment

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

My personal preference is to have the these imports separated with a line break from the outside imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your wish is my command.

"k8s.io/client-go/tools/cache"
)

const persistentVolumeName = "persistent"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this name hardcoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it never changes ;)

It's a bit like the name for the CM volume. We hardcode it, and since we are good programmers, we move it to a constant ;)

Basically, there's no reason to expose this name and make it configurable I think.

@@ -223,7 +224,7 @@ func ConvertSecret(pathToYaml string) (*v1.Secret, error) {
return &s, nil
}

func convertToK8sResource(pathToYaml string, into interface{}) error {
func convertToK8sResource(pathToYaml string, into runtime.Object) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

cool! TIL

@lilic
Copy link
Contributor

lilic commented Mar 20, 2018

BTW since this is a huge breaking change IMO we really need to bump up the version here of the API, so to v2beta1 or something like that. As that we don't get problems if this operator is deployed into a cluster where Habitat resources, e.g. Deployments already exist as that cannot be handled by this operator.

@asymmetric
Copy link
Contributor Author

We agreed offline to keep the current Deployment-based behavior in v1beta1, alongside the new StatetfulSet-based one on v1beta2. This will be done in a folloging PR.

So please go ahead and do your final reviews :)

Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Mainly questions and suggestions. Nothing blocking.

// MountPath is the path at which the PersistentVolume will be mounted.
MountPath string `json:"mountPath"`
// StorageClassName is the name of the StorageClass that the StatefulSet will request.
StorageClassName string `json:"storageClassName"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Can we name this just ClassName as it's under the PersistantStorage anyways. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I gave it the same name it has in the PersistentVolume resource, because it maps 1:1 with that field. My hope is that it will make it easier for people to understand how things tie together.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, that makes sense!

type PersistentStorage struct {
// Size is the volume's size.
// It uses the same format as Kubernetes' size fields, e.g. 10Gi
Size string `json:"size"`
Copy link
Contributor

Choose a reason for hiding this comment

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

You are missing the omitempty, or are these are required? Then we should document that as we don't have the api docs.md anymore. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? Right now we only explicitly document that a field is optional, otherwise it's mandatory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, just before in the docs.md file it was more clear at least to me, what is required. But if it makes sense to you 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT that's also how its' done in upstream.

OwnerReferences: []metav1.OwnerReference{
metav1.OwnerReference{
APIVersion: habv1beta1.SchemeGroupVersion.String(),
Kind: habv1beta1.HabitatKind,
Copy link
Contributor

Choose a reason for hiding this comment

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

What do we use this OwnerReference for now?

Copy link
Contributor Author

@asymmetric asymmetric Mar 21, 2018

Choose a reason for hiding this comment

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

In theory so that the CM is deleted with the Habitat, but apparently that's not working?

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't link to anything for me? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -0,0 +1,37 @@
# Persistent Storage example
Copy link
Contributor

Choose a reason for hiding this comment

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

So the whole point of persistence is that you can mount already existing volumes, can you maybe write up some steps of how a user can do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I explicitly avoided to do that because it's a big topic and I would only make a poorer job at explaining it than the official docs do, which I linked. I anyway added some commits after your comment which should hopefully make things clearer.

BTW, it's not necessary that the PV has been created beforehand, if the cluster supports dynamic provisioning.

TL;DR: IMHO explaining how persistence works in Kubernetes is beside the scope of this README.

@@ -0,0 +1,37 @@
# Persistent Storage example

Habitat objects are translated by the operator into `StatefulSet` objects, which
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Maybe add a note that the user needs to know about basic understanding of persistent volumes in K8s and link to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll add a link to this


const persistentVolumeName = "persistent"

func (hc *HabitatController) newStatefulSet(h *habv1beta1.Habitat) (*appsv1beta1.StatefulSet, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You know now that you extracted this you can actually write a unit test for this. WDYT?

@@ -802,15 +526,16 @@ func (hc *HabitatController) processNextItem() bool {

// conform is where the reconciliation takes place.
// It is invoked when any of the following resources get created, updated or deleted:
// Habitat, Pod, Deployment, ConfigMap.
// Habitat, Pod, StatefulSet, ConfigMap.
func (hc *HabitatController) conform(key string) error {
obj, exists, err := hc.habInformer.GetStore().GetByKey(key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not handle conforming the PersistentVolumeClaim as well? Or is there nothing that needs to be done, when for example Habitat resource gets deleted or updated?

Copy link
Contributor Author

@asymmetric asymmetric Mar 21, 2018

Choose a reason for hiding this comment

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

As mentioned in the example README, we do nothing to the PV/PVC when the Habitat gets deleted, because we want to prioritize data safety (which is also the default behavior with STS).

Do you think we should do something?

To avoid having to have specific instructions for each platform.

On GKE or minikube, users could use the default SC (thereby leaving the
field empty), or could specify them by their name (and those name
differ between GKE and minikube).

Instead, we just ask them to create a custom StorageClass for the
example.
These rules are required for clusters running on minikube.
The file has been removed in a previous commit.
Should act as an entry point for users interested in learning how
persistent storage works in Kubernetes.
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

LGTM if it works :)

kind: ClusterRole
name: cluster-writer
apiGroup: rbac.authorization.k8s.io

Copy link
Contributor

Choose a reason for hiding this comment

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

Line too much?

verbs: ["*"]
- nonResourceURLs: ["*"]
verbs: ["*"]

Copy link
Contributor

Choose a reason for hiding this comment

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

One line too much. :)

Copy link
Contributor Author

@asymmetric asymmetric Mar 21, 2018

Choose a reason for hiding this comment

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

It's required/recommended to have a newline at the end of a file. My editor adds it automatically.

Git complains if you don't have it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Except that's not the end 🤣

apiGroup: rbac.authorization.k8s.io

---

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove line? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually think that's not even needed, not sure why it's there in the first place. There's no sd-build ServiceAccount on my minikube cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrong position 😂 My comment referred to the bottom ClusterRoleBinding.

* specify the `name` of the aforementioned `StorageClass` object under
`spec.persistence.storageClassName` in the Habitat object's manifest

An example `StorageClass` for clusters running on minikube is provided in
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe note it doesn't work if it's vm-driver=none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think anyone's gonna try it with that flag, considering it's not even documented ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

Think it is now. :D

@asymmetric asymmetric merged commit 5e2c533 into master Mar 21, 2018
@asymmetric asymmetric deleted the asymmetric/sts branch March 21, 2018 15:23
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