syedahsn commented on PR #34966: URL: https://github.com/apache/airflow/pull/34966#issuecomment-1778192202
Looking at this, it looks like a few things were missing. 1. The first is that the `EksCreateClusterTrigger` does not set a status as `failed`, which means [this](https://github.com/apache/airflow/blob/667ab8c6eaceb689f6a1afbd909d88bdf0584342/airflow/providers/amazon/aws/operators/eks.py#L338C9-L338C42) branch never gets run: ``` elif event["status"] == "failed": ``` The solution to this issue would be write a `run` method for the `EksCreateClusterTrigger`, which would check to see if there was an error thrown when creating the cluster, and if so, yield a TriggerEvent with the status as `failed`. This would then be caught in the branch I linked above. 2. There isn't much that needs to be changed in the `EksDeleteClusterTrigger`, except what @Taragolis pointed out, which is that the `self.hook` is a method and not a property, so the correct usage should be `async with self.hook().async_conn as client:`. This is also True for the other instances in the `triggers/eks.py` file. There is one minor bug that I saw, which requires [this](https://github.com/apache/airflow/blob/667ab8c6eaceb689f6a1afbd909d88bdf0584342/airflow/providers/amazon/aws/triggers/eks.py#L128) line ``` await client.delete_cluster(name=self.cluster_name) ``` to be pulled out of the `if` block (i.e. dedent once). This is so that the cluster gets deleted even if `force_delete_compute` is False. 3. As for the `raise event["exception"]` this was a typo. I don't think we need to capture the exception in the event dict. We can simply raise an `AirflowException` as @Lee-W suggested, with the message that the cluster creation failed. The actual exception will be show up in the logs (from the waiter). 4. Lastly, there should be some more tests that capture the cases where a Cluster is not created successfully as well as when deleting the cluster. I think that's everything. I made some quick changes locally, and I was able to test everything, but I would appreciate another look @Taragolis. @theaadya I realize this is a lot, so I'll leave it to you to decide if you want to continue with this. If you do decide to work on this, I'd be more than happy to go over any questions you have. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
