Make argo events sensor namespace configurable #1463
Conversation
|
In our set up at work, we don't have event buses in the namespaces where the workflows run. Thus the "normal" behavior is that if one tries to deploy a workflow ( With this PR, I was able to test specifying a namespace and not specifying a namespace. When I did not specify a namespace, I got the same behavior as the master branch (i.e. kubernetes error). When I specified the argo events namespace, the argo workflow and sensor successfully get created. When triggered, the triggered workflow runs in the same workspace as the triggering workflow (not the namespace of the sensor) which is the desired behavior. |
…e-sensor-namespace
There was a problem hiding this comment.
argo-workflows delete (was merged in recently) and argo-workflows create both try to delete a sensor as part of the command. There's a slight concern of leaving dangling sensors in custom namespaces if the user issuing the command does not remember to specify the correct one for the sensors
There was a problem hiding this comment.
To revisit my earlier point on tracking where resources get created: One addition that could be done in scope of this PR is that we could add the sensor name, and the sensor namespace to the workflow template we deploy, either as annotations or as labels.
The main point for this is to provide visibility on where resources are deployed as part of the workflow template, but as an added benefit these could be used as another check for create(redeploy)/delete operations when looking for existing sensors
metaflow/plugins/argo/argo_client.py
Outdated
| def __init__(self, namespace=None, sensor_namespace=None): | ||
| self._client = KubernetesClient() | ||
| self._namespace = namespace or "default" | ||
| self._sensor_namespace = sensor_namespace or self._namespace |
There was a problem hiding this comment.
There's a clear tradeoff here with granularity vs. simplicity regarding the ArgoClient. For now we could do with the original implementation of only allowing namespace=, and simply having a different client instance for each namespace we want to operate in.
There was a problem hiding this comment.
I am not sure if that would work. The namespace is also used for knowing where to trigger the workflow. So I have workflow 1 in namespace A that communicates with a sensor in namespace B that triggers workflow 2 in namespace A again. And the way the argo client works is namespace = A in this case. If you were to set namespace = B, I would have to make sure that workflow 2 does not run in namespace B.
- Add a cleanup function during `argo-workflows create` - Clean up sensors and cronworkflows - Refactor ArgoClient to no longer need the `sensor_namespace` parameter.
…-sensor-namespace
| @@ -225,6 +225,7 @@ def create( | |||
| obj.echo_always(str(flow), err=False, no_bold=True) | |||
| # TODO: Support echo-ing Argo Events Sensor template | |||
| else: | |||
| flow.cleanup_previous() | |||
There was a problem hiding this comment.
I believe there is an issue with running the cleanup only as part of the deploy command. Consider the following scenario:
- user has deployed a sensor flow to namespace
dev-A, sensor exists in the same namespace - this feature gets released and the Argo sensor namespace env_var gets configured to
dev-B - the user decides that they want to get rid of their flow, and issue an
argo-workflows deletedirectly.
Wouldn't this only check for the currently configured sensor namespace, leaving the original sensor in dev-A untouched?
There was a problem hiding this comment.
At least in the case of delete it seems sensible to me to keep the concern of cleaning up resources as part of the method.
There was a problem hiding this comment.
For the legacy case, we need to do cleanup before creating a new argo workflow. This can only happen if we clean up before calling flow.deploy().
One reason I did this is that there is some inconsistency in the API. The current implementation already does some sort of "cleanup". For schedules, we create/suspend them within the schedule_workflow_template method which is in the argo_client.py, which is called within the schedule method in argo_workflows.py. However the sensors are created/deleted within the schedule method in argo_workflows.py. So I wanted to move this all into one place, but I couldn't put my code into this schedule method. The previous annotations for the triggers would be overwritten before the schedulemethod.
Currently, the flow.schedule function is used to create both the schedule and the sensor with the assumption that the sensor will get overwritten because it always exists in the same namespace. So in your scenario, if you configured you local environment with dev-B then you would have to do an argo-workflows create in order to create a sensor in dev-B. Once you did that, it would delete the sensor in dev-A due to this line:
…-sensor-namespace
…-sensor-namespace
…-sensor-namespace
…-sensor-namespace
|
bump on this please; looks to be approved, just maybe some formalities around getting it merged/deployed |
|
Bueller? Anyone looking into this PR - its approved but likely need some maintainers to merge/release it |
…-sensor-namespace
…-sensor-namespace
…-sensor-namespace
|
While this continues to work as of today, I recently changed our setup to deploy the required event infra into all of our namespaces so I probably will not update this again. |
ARGO_EVENTS_SENSOR_NAMESPACEto config (defaults toKUBERNETES_NAMESPACE)sensor_namespaceparameter to argo clientResolves #1455