-
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
[topology label] add new label operator.habitat.sh/topology
#332
[topology label] add new label operator.habitat.sh/topology
#332
Conversation
habitat-topology
habitat-topology
d0c9b6f
to
5d2dade
Compare
habitat-topology
habitat-topology
5d2dade
to
f212347
Compare
@indradhanush can you PTAL? |
When the operator with this change is deployed, it updates the labels inside the With following events 0s 0s 1 example-standalone-habitat-0.154a601bee720296 Pod spec.containers{habitat-service} Normal Killing kubelet, minikube Killing container with id docker://habitat-service:Need to kill Pod
0s 0s 1 example-standalone-habitat-0.154a601c4f5af632 Pod Normal Scheduled default-scheduler Successfully assigned example-standalone-habitat-0 to minikube
0s 10m 6 example-standalone-habitat.154a5f85a74d6e69 StatefulSet Normal SuccessfulCreate statefulset-controller create Pod example-standalone-habitat-0 in StatefulSet example-standalone-habitat successful
0s 0s 1 example-standalone-habitat.154a601c553527f4 Habitat Normal ConfigMapUpdated habitat-controller Removed peer IP from ConfigMap
0s 0s 2 example-standalone-habitat.154a601c553527f4 Habitat Normal ConfigMapUpdated habitat-controller Removed peer IP from ConfigMap
0s 0s 1 example-standalone-habitat-0.154a601c5a318ee4 Pod Normal SuccessfulMountVolume kubelet, minikube MountVolume.SetUp succeeded for volume "config"
0s 0s 1 example-standalone-habitat-0.154a601c5a3641b6 Pod Normal SuccessfulMountVolume kubelet, minikube MountVolume.SetUp succeeded for volume "default-token-clkdd"
0s 1s 3 example-standalone-habitat.154a601c553527f4 Habitat Normal ConfigMapUpdated habitat-controller Removed peer IP from ConfigMap
0s 0s 1 example-standalone-habitat-0.154a601c8b0e4721 Pod spec.containers{habitat-service} Normal Pulling kubelet, minikube pulling image "habitat/redis-hab"
0s 0s 1 example-standalone-habitat-0.154a601d75eea4aa Pod spec.containers{habitat-service} Normal Pulled kubelet, minikube Successfully pulled image "habitat/redis-hab"
0s 0s 1 example-standalone-habitat-0.154a601d820e072c Pod spec.containers{habitat-service} Normal Created kubelet, minikube Created container
0s 0s 1 example-standalone-habitat-0.154a601d8cdbf692 Pod spec.containers{habitat-service} Normal Started kubelet, minikube Started container
0s 0s 1 example-standalone-habitat.154a601dbc96d81b Habitat Normal ConfigMapUpdated habitat-controller Updated peer IP in ConfigMap cc: @krnowak |
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.
Two things need to happen before we can merge this PR:
- we need to keep the old topology label and we need to remember to add some entry to the changelog when we do a release that this label is deprecated (for say, two releases) and will be dropped in future
- I'd prefer [spec][backwards incompatible] drop support for v1beta1 #331 to be merged first.
pkg/apis/habitat/v1beta1/types.go
Outdated
@@ -30,7 +30,7 @@ const ( | |||
// Example: 'habitat-name: db' | |||
HabitatNameLabel = "habitat-name" | |||
|
|||
TopologyLabel = "topology" | |||
HabitatTopologyLabel = "operator.habitat.sh/topology" |
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'd keep the old constant and add a new one, instead of replacing it. The reason behind it is that the code that was filtering pods by the old topology label will stop working after replacing. So, for now, I would keep the old label intact and add a new 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.
What do you mean by
code that was filtering pods by the old topology label
because adding a new label will update the StatefulSet
and willl also re-create the pod.
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.
After this change something like kubectl get pods -l topology=leader
will not show the pods it used to show, because they won't have the topology
label anymore. They will have operator.habitat.sh/topology
instead. That's why I would like to keep the old label and add a new one. The old label could be removed after some deprecation period.
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.
cool gotcha!
pkg/controller/v1beta1/controller.go
Outdated
habv1beta1.TopologyLabel: topology.String(), | ||
habv1beta1.HabitatLabel: "true", | ||
habv1beta1.HabitatNameLabel: h.Name, | ||
habv1beta1.HabitatTopologyLabel: topology.String(), |
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.
As I mentioned earlier - let's keep the old label too.
habv1beta1.TopologyLabel: topology.String(), | ||
habv1beta1.HabitatLabel: "true", | ||
habv1beta1.HabitatNameLabel: h.Name, | ||
habv1beta1.HabitatTopologyLabel: topology.String(), |
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.
Same here.
f212347
to
1546d15
Compare
Please rebase. |
This is the new label that defines topology, which also marks the deprecation of older label `topology`. This will be removed after three releases. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>
1546d15
to
b665bae
Compare
habitat-topology
operator.habitat.sh/topology
@krnowak rebased! |
@krnowak rebased PTAL :-) |
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.
LFAD.
Would be nice to do the similar thing for other labels we add there I think. |
Renaming the topology label from just
topology
tooperator.habitat.sh/topology
.Fixes #309