Changed to always use namespace when a name is involved #485
Conversation
|
@jkandasa , this can be tested as follows: # make sure the environment is clean:
$ kubectl get deployments --all-namespaces
NAMESPACE NAME READY UP-TO-DATE AVAILABLE AGE
kube-system coredns 2/2 2 2 28h
kube-system default-http-backend 1/1 1 1 28h
kube-system kubernetes-dashboard 1/1 1 1 28h
kube-system nginx-ingress-controller 1/1 1 1 28h
$ kubectl create namespace tenant1
namespace/tenant1 created
$ kubectl create namespace tenant2
namespace/tenant2 created
$ kubectl apply -n tenant1 -f deploy/examples/simplest.yaml
jaeger.jaegertracing.io/simplest created
$ kubectl apply -n tenant2 -f deploy/examples/simplest.yaml
jaeger.jaegertracing.io/simplest created
$ kubectl get deployments -n tenant1
NAME READY UP-TO-DATE AVAILABLE AGE
simplest 1/1 1 1 40s
$ kubectl get deployments -n tenant2
NAME READY UP-TO-DATE AVAILABLE AGE
simplest 1/1 1 1 28s
$ kubectl get jaegers --all-namespaces
NAMESPACE NAME AGE
tenant1 simplest 51s
tenant2 simplest 38s |
jpkrohling
force-pushed
the
285-Two-jaeger-same-names
branch
from
2ef6f4d
to
da63751
Compare
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
jpkrohling
force-pushed
the
285-Two-jaeger-same-names
branch
from
383b296
to
a2deaec
Compare
| "github.com/jaegertracing/jaeger-operator/pkg/inventory" | ||
| ) | ||
|
|
||
| func (r *ReconcileJaeger) applyAccounts(jaeger v1.Jaeger, desired []corev1.ServiceAccount) error { | ||
| opts := client.MatchingLabels(map[string]string{ | ||
| opts := client.InNamespace(jaeger.Namespace).MatchingLabels(map[string]string{ |
There was a problem hiding this comment.
This (and similar changes in the other controller functions) is the actual fix for the reported problem. All other changes are either to make the test more accurate, or to properly log what's going on.
| if err := r.client.Update(context.Background(), &d); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| for _, d := range inv.Delete { | ||
| jaeger.Logger().WithField("account", d.Name).Debug("deleting service account") | ||
| jaeger.Logger().WithFields(log.Fields{ |
There was a problem hiding this comment.
This overrides the namespace from the jaeger.Logger(). Before this change, the log would say that it "deleting service account" in namespace tenant1 when in fact the SA was in the namespace tenant2.
| @@ -54,7 +56,7 @@ func ForAccounts(existing []v1.ServiceAccount, desired []v1.ServiceAccount) Acco | |||
| func accountMap(deps []v1.ServiceAccount) map[string]v1.ServiceAccount { | |||
| m := map[string]v1.ServiceAccount{} | |||
| for _, d := range deps { | |||
| m[d.Name] = d | |||
| m[fmt.Sprintf("%s.%s", d.Namespace, d.Name)] = d | |||
There was a problem hiding this comment.
This kind of change isn't strictly required, as the data it receives should be part of the same namespace. However, it's more accurate to use the namespace in the key, as "tenant1.simplest" shouldn't be seen as the same as "tenant2.simplest". This fix helped me spot the real cause for the bug.
|
@objectiser sorry for such a bit PR. Most of it are changes to the |
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
=========================================
+ Coverage 91.74% 91.9% +0.15%
=========================================
Files 65 64 -1
Lines 3199 3272 +73
=========================================
+ Hits 2935 3007 +72
- Misses 184 185 +1
Partials 80 80
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #485 +/- ##
==========================================
+ Coverage 91.74% 91.94% +0.19%
==========================================
Files 65 65
Lines 3199 3290 +91
==========================================
+ Hits 2935 3025 +90
- Misses 184 185 +1
Partials 80 80
Continue to review full report at Codecov.
|
|
@jpkrohling can you provide a docker image for this PR? |
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Fixes #285 by using namespaces whenever a name is involved. Before this change, deploying a Jaeger instance in a namespace
tenant2when there's already a Jaeger instance in another namespace (tenant1) would cause the underlying objects, such asDeploymentobjects, to be removed in favor of the one in the new namespace.Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de