Should resourcelock be in the same namespace as controller?

[ScorpioCPH]

As we discussed here, need to investigate whether the namespace of resourcelock should be the same as controller.

[jlewi]

I suspect it should be because the resource lock is per controller; i.e. we might have different controllers in different namespaces each claiming a different set of TFJobs.

So resource lock should be scoped to a namespace because we only 1 instance of the controller in each namespace accessing the subset of TFJobs assigned to that namespace.

[ScorpioCPH]

After taking a deep look at source code here, I think resourcelock's name and namespace is not related to CRD or controller.

It is used for create new Endpoint object with LeaderElectionRecord annotation, code is here.

@jlewi @gaocegege FYI.

[jlewi]

Thanks for investigating.Can you clarify what you mean unrelated? Shouldn't the resourceLock be deployed in the same namespace as the controller?

It seems strange that the controller would be creating resources in the default namespace.

If I deploy the controller in some name space e.g. "controller-test", then my expectation is that if I delete the namespace "controller-test" all resources associated with the controller should be deleted.

Is there a downside to creating the resourceLock in the same namespace as the controller?

[ScorpioCPH]

Shouldn't the resourceLock be deployed in the same namespace as the controller?

No, it shouldn't. You can deploy ResourceLock and controller in different namespaces.

Is there a downside to creating the resourceLock in the same namespace as the controller?

I don't think so.

Actually, you can deploy controller in many ways:

• Deployment(recommended)
• Binary
• Docker container

It will create only one Endpoint object even if we restart the controller.

my expectation is that if I delete the namespace "controller-test" all resources associated with the controller should be deleted.

SGTM.

But i'm not sure how to get the namespace of controller if user deploy it in themselves way, set a NAME_SPACE env seems a little strange.

How about set optional start flags --resourcelock-name and --resourcelock-namespace and use default namespace by default.

[jlewi]

SGTM.

But i'm not sure how to get the namespace of controller if user deploy it in themselves way, set a NAME_SPACE env seems a little strange.

How about set optional start flags --resourcelock-name and --resourcelock-namespace and use default namespace by default.

I think this a good solution as it allows us to use the same namespace as the controller for the resourcelock when deployed on K8s.

If we use a flag that we set via environment variable when running on K8s, does that mean we can't run the container using the exec form and need to run in a shell because we rely on environment variable expansion?

Why not just continue to use an environment variable (as opposed to flags) and if the environment variable isn't set set use the default namespace?

[ScorpioCPH]

If we use a flag that we set via environment variable when running on K8s, does that mean we can't run the container using the exec form and need to run in a shell because we rely on environment variable expansion?

You can use the ENV statement to set an environment variable in your Dockerfile.

IMHO, We should use environmental variables sparingly:

• They are delete-able, set-able from anywhere, if i inadvertently changed the value of ENV, I can't see the change clearly.
• They're visible be any programs which running on the same HOST, i don't think it is a safe way.

Command-line arguments seems better:

• It is scoped in individual program.
• Can be changed often.

[jlewi]

So I generally agree about environment variables vs. command line flags. But in this particular case, POD_NAMESPACE describes the runtime environment so isn't this a suitable use case for using an environment variable and not a flag?

The only time we want to set a flag is when running on K8s in which case the POD_NAMESPACE environment variable will be set via the downward API.

If you wanted to set namespace via a flag, then I think you would still need to set the environment variable POD_NAMESPACE using the downward API and then you'd have to set the value of the flag to the environment variable e.g. --namespace=$(POD_NAMESPACE). I think this has the disadvantage that you need to run your command inside a shell in order to get environment variable expansion.

[ScorpioCPH]

The simple question: what if I want to run another program with the same POD_NAMESPACE environment variable but different values?

The only time we want to set a flag is when running on K8s

Sorry i'm not very clear about your point here, command line flags and environment variable have nothing about how we deploy the program (docker, k8s, binary, etc).

If you wanted to set namespace via a flag, then I think you would still need to set the environment variable POD_NAMESPACE using the downward API and then you'd have to set the value of the flag to the environment variable e.g. --namespace=$(POD_NAMESPACE).

Why should i still need to set the environment variable?

AFAIK, most components of k8s (kubelet, controller-manager, scheduler) used command line flags instead of environment variable.

[jlewi]

So the problem we want to solve is how to let the tf-operator know what namespace its running in.
The operator needs to know what namespace its in so that it can create other resources (e.g. the resource lock) in the same namespace.

The namespace is a property of the runtime environment i.e. the K8s pod. The environment variable should only be set when running on K8s. Furthermore, the namespace should be controlled by setting the namespace you deploy the operator in. The convention in K8s seems to pass this through via environment variables (e.g. this is what the etcd-operator does).

I think the cleanest way to do this is

• Use the downward API to set an environment variable on the operator deployment
• Have the operator directly read the environment variable.

If the operator uses a flag then we need to set the flag equal to the environment variable. This relies on shell expansion to evaluate the environment variable and set the flag. I think its cleaner to not rely on variable expansion and just access the environment variable directly.

I believe the motivation for this PR was to simplify running the controller locally (not on K8s) in a debug setting. In which case the operator isn't running in a namespace so we can't rely on the downward API to set the environment variable.

This is an exceptional use case since the operator is intended to run on K8s. So I think its perfectly reasonable to make developers explicitly set the NAMESPACE environment variable when running the controller locally.

[ScorpioCPH]

Thanks for your nice explanation, one last question:

Should the environment variable NAMESPACE be required?

I prefer optional.

[jlewi]

Optional is fine.

[gaocegege]

Could we close the issue since #348 is merged 🤔