Fix regression in DataflowTemplatedJobStartOperator (#11167)

Tobiasz Kędzierski · web-flow · commit 8baf657fc2b2 · 2020-10-09T10:21:16.000+02:00
diff --git a/airflow/providers/google/cloud/hooks/dataflow.py b/airflow/providers/google/cloud/hooks/dataflow.py
@@ -530,13 +530,15 @@ def start_template_dataflow(
         append_job_name: bool = True,
         on_new_job_id_callback: Optional[Callable[[str], None]] = None,
         location: str = DEFAULT_DATAFLOW_LOCATION,
+        environment: Optional[Dict] = None,
     ) -> Dict:
         """
         Starts Dataflow template job.
 
         :param job_name: The name of the job.
         :type job_name: str
         :param variables: Map of job runtime environment options.
+            It will update environment argument if passed.
 
             .. seealso::
                 For more information on possible configurations, look at the API documentation
@@ -556,9 +558,48 @@ def start_template_dataflow(
         :type on_new_job_id_callback: callable
         :param location: Job location.
         :type location: str
+        :type environment: Optional, Map of job runtime environment options.
+
+            .. seealso::
+                For more information on possible configurations, look at the API documentation
+                `https://cloud.google.com/dataflow/pipelines/specifying-exec-params
+                <https://cloud.google.com/dataflow/docs/reference/rest/v1b3/RuntimeEnvironment>`__
+
+        :type environment: Optional[dict]
         """
         name = self._build_dataflow_job_name(job_name, append_job_name)
 
+        environment = environment or {}
+        # available keys for runtime environment are listed here:
+        # https://cloud.google.com/dataflow/docs/reference/rest/v1b3/RuntimeEnvironment
+        environment_keys = [
+            'numWorkers',
+            'maxWorkers',
+            'zone',
+            'serviceAccountEmail',
+            'tempLocation',
+            'bypassTempDirValidation',
+            'machineType',
+            'additionalExperiments',
+            'network',
+            'subnetwork',
+            'additionalUserLabels',
+            'kmsKeyName',
+            'ipConfiguration',
+            'workerRegion',
+            'workerZone',
+        ]
+
+        for key in variables:
+            if key in environment_keys:
+                if key in environment:
+                    self.log.warning(
+                        "'%s' parameter in 'variables' will override of "
+                        "the same one passed in 'environment'!",
+                        key,
+                    )
+                environment.update({key: variables[key]})
+
         service = self.get_conn()
         # pylint: disable=no-member
         request = (
@@ -569,7 +610,7 @@ def start_template_dataflow(
                 projectId=project_id,
                 location=location,
                 gcsPath=dataflow_template,
-                body={"jobName": name, "parameters": parameters, "environment": variables},
+                body={"jobName": name, "parameters": parameters, "environment": environment},
             )
         )
         response = request.execute(num_retries=self.num_retries)
diff --git a/airflow/providers/google/cloud/operators/dataflow.py b/airflow/providers/google/cloud/operators/dataflow.py
@@ -281,6 +281,7 @@ class DataflowTemplatedJobStartOperator(BaseOperator):
     :param job_name: The 'jobName' to use when executing the DataFlow template
         (templated).
     :param options: Map of job runtime environment options.
+        It will update environment argument if passed.
 
         .. seealso::
             For more information on possible configurations, look at the API documentation
@@ -316,6 +317,13 @@ class DataflowTemplatedJobStartOperator(BaseOperator):
         Service Account Token Creator IAM role to the directly preceding identity, with first
         account from the list granting this role to the originating account (templated).
     :type impersonation_chain: Union[str, Sequence[str]]
+    :type environment: Optional, Map of job runtime environment options.
+
+        .. seealso::
+            For more information on possible configurations, look at the API documentation
+            `https://cloud.google.com/dataflow/pipelines/specifying-exec-params
+            <https://cloud.google.com/dataflow/docs/reference/rest/v1b3/RuntimeEnvironment>`__
+    :type environment: Optional[dict]
 
     It's a good practice to define dataflow_* parameters in the default_args of the dag
     like the project, zone and staging location.
@@ -373,6 +381,7 @@ class DataflowTemplatedJobStartOperator(BaseOperator):
         'location',
         'gcp_conn_id',
         'impersonation_chain',
+        'environment',
     ]
     ui_color = '#0273d4'
 
@@ -391,6 +400,7 @@ def __init__(  # pylint: disable=too-many-arguments
         delegate_to: Optional[str] = None,
         poll_sleep: int = 10,
         impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+        environment: Optional[Dict] = None,
         **kwargs,
     ) -> None:
         super().__init__(**kwargs)
@@ -407,6 +417,7 @@ def __init__(  # pylint: disable=too-many-arguments
         self.job_id = None
         self.hook: Optional[DataflowHook] = None
         self.impersonation_chain = impersonation_chain
+        self.environment = environment
 
     def execute(self, context):
         self.hook = DataflowHook(
@@ -421,7 +432,6 @@ def set_current_job_id(job_id):
 
         options = self.dataflow_default_options
         options.update(self.options)
-
         job = self.hook.start_template_dataflow(
             job_name=self.job_name,
             variables=options,
@@ -430,6 +440,7 @@ def set_current_job_id(job_id):
             on_new_job_id_callback=set_current_job_id,
             project_id=self.project_id,
             location=self.location,
+            environment=self.environment,
         )
 
         return job
diff --git a/tests/providers/google/cloud/hooks/test_dataflow.py b/tests/providers/google/cloud/hooks/test_dataflow.py
@@ -746,6 +746,7 @@ def test_start_template_dataflow_with_runtime_env(self, mock_conn, mock_dataflow
             parameters=PARAMETERS,
             dataflow_template=TEST_TEMPLATE,
             project_id=TEST_PROJECT,
+            environment={"numWorkers": 17},
         )
         body = {"jobName": mock.ANY, "parameters": PARAMETERS, "environment": RUNTIME_ENV}
         method.assert_called_once_with(
@@ -765,6 +766,52 @@ def test_start_template_dataflow_with_runtime_env(self, mock_conn, mock_dataflow
         )
         mock_uuid.assert_called_once_with()
 
+    @mock.patch(DATAFLOW_STRING.format('uuid.uuid4'), return_value=MOCK_UUID)
+    @mock.patch(DATAFLOW_STRING.format('_DataflowJobsController'))
+    @mock.patch(DATAFLOW_STRING.format('DataflowHook.get_conn'))
+    def test_start_template_dataflow_update_runtime_env(self, mock_conn, mock_dataflowjob, mock_uuid):
+        options_with_runtime_env = copy.deepcopy(RUNTIME_ENV)
+        del options_with_runtime_env["numWorkers"]
+        runtime_env = {"numWorkers": 17}
+        expected_runtime_env = copy.deepcopy(RUNTIME_ENV)
+        expected_runtime_env.update(runtime_env)
+
+        dataflowjob_instance = mock_dataflowjob.return_value
+        dataflowjob_instance.wait_for_done.return_value = None
+        # fmt: off
+        method = (mock_conn.return_value
+                  .projects.return_value
+                  .locations.return_value
+                  .templates.return_value
+                  .launch)
+        # fmt: on
+        method.return_value.execute.return_value = {'job': {'id': TEST_JOB_ID}}
+        self.dataflow_hook.start_template_dataflow(  # pylint: disable=no-value-for-parameter
+            job_name=JOB_NAME,
+            variables=options_with_runtime_env,
+            parameters=PARAMETERS,
+            dataflow_template=TEST_TEMPLATE,
+            project_id=TEST_PROJECT,
+            environment=runtime_env,
+        )
+        body = {"jobName": mock.ANY, "parameters": PARAMETERS, "environment": expected_runtime_env}
+        method.assert_called_once_with(
+            projectId=TEST_PROJECT,
+            location=DEFAULT_DATAFLOW_LOCATION,
+            gcsPath=TEST_TEMPLATE,
+            body=body,
+        )
+        mock_dataflowjob.assert_called_once_with(
+            dataflow=mock_conn.return_value,
+            job_id=TEST_JOB_ID,
+            location=DEFAULT_DATAFLOW_LOCATION,
+            name='test-dataflow-pipeline-{}'.format(MOCK_UUID),
+            num_retries=5,
+            poll_sleep=10,
+            project_number=TEST_PROJECT,
+        )
+        mock_uuid.assert_called_once_with()
+
     @mock.patch(DATAFLOW_STRING.format('_DataflowJobsController'))
     @mock.patch(DATAFLOW_STRING.format('DataflowHook.get_conn'))
     def test_cancel_job(self, mock_get_conn, jobs_controller):
diff --git a/tests/providers/google/cloud/operators/test_dataflow.py b/tests/providers/google/cloud/operators/test_dataflow.py
@@ -261,6 +261,7 @@ def setUp(self):
             dataflow_default_options={"EXTRA_OPTION": "TEST_A"},
             poll_sleep=POLL_SLEEP,
             location=TEST_LOCATION,
+            environment={"maxWorkers": 2},
         )
 
     @mock.patch('airflow.providers.google.cloud.operators.dataflow.DataflowHook')
@@ -287,4 +288,5 @@ def test_exec(self, dataflow_mock):
             on_new_job_id_callback=mock.ANY,
             project_id=None,
             location=TEST_LOCATION,
+            environment={'maxWorkers': 2},
         )

Original file line number	Diff line number	Diff line change
`@@ -261,6 +261,7 @@ def setUp(self):`
`261`	`261`	`dataflow_default_options={"EXTRA_OPTION": "TEST_A"},`
`262`	`262`	`poll_sleep=POLL_SLEEP,`
`263`	`263`	`location=TEST_LOCATION,`
	`264`	`+ environment={"maxWorkers": 2},`
`264`	`265`	`)`
`265`	`266`
`266`	`267`	`@mock.patch('airflow.providers.google.cloud.operators.dataflow.DataflowHook')`
`@@ -287,4 +288,5 @@ def test_exec(self, dataflow_mock):`
`287`	`288`	`on_new_job_id_callback=mock.ANY,`
`288`	`289`	`project_id=None,`
`289`	`290`	`location=TEST_LOCATION,`
	`291`	`+ environment={'maxWorkers': 2},`
`290`	`292`	`)`