-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add channels support to the Habitat type #128
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably would want to add a test for the channel in test/e2e/operator_test.go
.
And although it comes from our discussion today, I would like to listen to what other people think in the next week.
pkg/habitat/apis/cr/v1/types.go
Outdated
@@ -29,6 +29,10 @@ const ( | |||
HabitatNameLabel = "habitat-name" | |||
|
|||
TopologyLabel = "topology" | |||
|
|||
// HabitatChannelLabel contains the information about stability of application. | |||
// Examole: 'channel: production' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: s/Examole/Example/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very familiar with the channels
, is it possible to promote a running application to another channel
, for example from unstable
to stable
?
Please add an example and some documentation about channels to the examples directory. Also please add this new field to the https://github.com/kinvolk/habitat-operator/blob/master/docs/api.md#habitatspec. Thanks!
crv1.HabitatLabel: "true", | ||
crv1.HabitatNameLabel: h.Name, | ||
crv1.TopologyLabel: topology.String(), | ||
crv1.HabitatLabel: "true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit confused why choose labels, maybe I am missing some info? From what I can tell you can pass in the argument the same way we do for other options: --channel unstable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the question. I'm passing the label to the deployment just to store the info (from which channel comes the app).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant, will these labels be only used for information purposes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the other purpose may be creating services for specific environments (to expose ports). When you have an application with one name, but with two channels (i.e. staging and production), you can create two services with selectors looking for different channels.
pkg/habitat/apis/cr/v1/types.go
Outdated
@@ -44,6 +48,8 @@ type HabitatSpec struct { | |||
// Image is the Docker image of the Habitat Service. | |||
Image string `json:"image"` | |||
Service Service `json:"service"` | |||
// Channel is the information about stability of the application, expressed as a label in Kubernetes. | |||
Channel string `json:"channel"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a mandatory field? If not, please add optional to the comment.
Would it make sense to have a default value when the user doesn't provide one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Habitats default seems to be unstable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question related to the new field.
All newly built packages are automatically put on You can attach a label to a group of services, Application.Environment, with the idea being you can use this as an organizational principle to do group promotion of various tiers of your application. This is a capability we haven't fully fleshed out/made available yet. For now, you should have the ability to promote a package from We anticipate it would be very unlikely for most users to have Supervisors subscribed directly to |
755b5d1
to
515d109
Compare
83fedaf
to
5256ef2
Compare
PTALA |
@nhlfr it seems your tests failed, I re-trigged the build. Will have a look once they pass. :) |
README.md
Outdated
@@ -61,6 +61,12 @@ To create an example service run: | |||
This will create a single-pod deployment of an `nginx` Habitat service. | |||
More examples are located in the [example directory](https://github.com/kinvolk/habitat-operator/tree/master/examples/). | |||
|
|||
### Promoting an example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Promoting an example
? I'd say "Promoting a Habitat/application"
README.md
Outdated
@@ -61,6 +61,12 @@ To create an example service run: | |||
This will create a single-pod deployment of an `nginx` Habitat service. | |||
More examples are located in the [example directory](https://github.com/kinvolk/habitat-operator/tree/master/examples/). | |||
|
|||
### Promoting an example | |||
|
|||
By default, the example service from `examples/standalone/habitat.yml` uses the `staging` channel. To promote it to the `production` channel, use the following example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this section we should instead briefly explain what promotion is in Habitat (or maybe only link to their docs) and clearly explain how we handle that in the operator.
README.md
Outdated
|
||
By default, the example service from `examples/standalone/habitat.yml` uses the `staging` channel. To promote it to the `production` channel, use the following example: | ||
|
||
kubectl create -f examples/standalone/habitat-promote.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this example should be under examples/promotion
. There should be two files there, one to create an application, named habitat.yml
, and one for the promotion, named promotion.yml
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that specific file is able to promote only the standalone application, it cannot be used to any other example!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples/promotion
directory would contain both the habitat app's manifest and the promotion manifest to promote that app. Or am I missing something?
docs/api.md
Outdated
| Field | Description | Scheme | Required | | ||
| ----- | ----------- | ------ | -------- | | ||
| habitatName | Name of the Habitat object to promote. | string | true | | ||
| oldChannel | Name of the old channel which the Habitat application is using. | string | true | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest: "Name of the channel the Habitat application is on before the promotion."
docs/api.md
Outdated
| ----- | ----------- | ------ | -------- | | ||
| habitatName | Name of the Habitat object to promote. | string | true | | ||
| oldChannel | Name of the old channel which the Habitat application is using. | string | true | | ||
| newChannel | Name of the new channel which the Habitat application is going to use after promotion. | string | true | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest: "Name of the channel the Habitat application is moved to as part of the promotion."
@@ -0,0 +1,8 @@ | |||
apiVersion: habitat.sh/v1 | |||
kind: HabitatPromote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be HabitatPromotion
? Isn't that more correct for a type/kind, i.e. that it is a noun?
51298ff
to
350122d
Compare
Habitat applications can exist in the different "channels" - like "staging", "production", "unstable" etc. Those names can be configured by the user. Those channels will be expressed as labels in Kubernetes. There is also a possibiity of promoting Habitat applications. This action is expressed by HabitatPromote object, which needs to be created in order to perform it. To test that functionality, you can use the example yml files: kubectl create -f examples/standalone/habitat.yml kubectl create -f examples/standalone/habitat-promotion.yml Fixes #126 Signed-off-by: Michal Rostecki <michal@kinvolk.io>
@asymmetric ptala |
if hc.habitatNeedsUpdate(oldHab, newHab) { | ||
hc.enqueue(newHab) | ||
if hc.habitatIsPromotiond(oldHab, newHab) { | ||
// Remove old deployment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You forgot the very important .
at the end! ;)
level.Error(hc.logger).Log("msg", "Failed to delete deployment", "name", deploymentName) | ||
return | ||
} | ||
level.Info(hc.logger).Log("msg", "created deployment", "name", deploymentName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the correct message? Should it mention promotion, as in "promoted deployment"?
@@ -845,6 +953,15 @@ func (hc *HabitatController) conform(key string) error { | |||
return nil | |||
} | |||
|
|||
func (hc *HabitatController) habitatIsPromotiond(oldHabitat, newHabitat *crv1.Habitat) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo, should be "HabitatIsPromoted", or "isHabitatPromoted".
func (hc *HabitatController) worker() { | ||
for hc.processNextItem() { | ||
func (hc *HabitatController) habitatWorker() { | ||
for hc.processNextHabitatItem() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these renames make me think we should split the promotion into its own controller. If this PR is urgent and blocking we can postpone the split, though.
Also, tests are failing. |
Needs to be re-designed and re-implemented |
I'm not deleting the branch just to keep the code visible |
What's the discussion that led to this decision? |
we talked about it offline but the basic outcome was that a promotion should just be another For that the channel probably has to be part of the object name: We don't have to remove the old deployment since that can be easily scripted if labels are set properly... |
Habitat applications can exist in the different "channels" -
like "staging", "production", "unstable" etc. Those names
can be configured by the user.
Those channels will be expressed as labels in Kubernetes.
There is also a possibiity of promoting Habitat applications.
This action is expressed by HabitatPromote object, which needs
to be created in order to perform it.
To test that functionality, you can use the example yml files:
Fixes #126
Signed-off-by: Michal Rostecki michal@kinvolk.io