-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[azure-docs] compute log manager docs #26202
base: dpeng817/user_code
Are you sure you want to change the base?
Conversation
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
We need to ensure that the AKS agent has the necessary permissions to write logs to Azure Blob Storage or Azure Data Lake Storage. How | ||
We'll do this with some azure CLI commands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be a sentence fragment with "How" dangling at the end, and an extra space at the start of the next line. Consider revising to: "We need to ensure that the AKS agent has the necessary permissions to write logs to Azure Blob Storage or Azure Data Lake Storage. We'll do this with some Azure CLI commands."
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
Graphite Automations"Add a 'docs-to-migrate' label to PRs with docs" took an action on this PR • (12/03/24)1 label was added to this PR based on Christopher DeCarolis's automation. |
a1892b7
to
450218f
Compare
53aa387
to
8deb104
Compare
Finally, we'll edit our AKS agent deployment to use the new service account. | ||
|
||
```bash | ||
kubectl edit deployment <your-user-cloud-deployment> -n dagster-agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider enabling the user to add this to their helm chart through the value chart, otherwise upgrading their agent becomes murky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
open to this being a follow-up ticket since a bit more involving than the current scope.
<Image src="/images/dagster-cloud/azure/azure-blob-storage-logs.png" alt="Azure Blob Storage logs in Dagster" width={970} height={794} /> | ||
|
||
<Note> | ||
Whether or not the URL will be clickable depends on whether your logs are public or private. If they are private, directly clicking the link would not work, and instead you should use either the Azure CLI or the Azure Portal to access the logs using the URL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so even if the user is authenticated to azure, the provided link would not work?
i am curious if that's the case with s3, it seems a bit like a rough user experience.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you confirm that to be the case, I think we should add this improvement as a ticket in the linear project, unless you know it isn't feasible for some reason.
|
||
## Step 3: Verify logs are being written to Azure Blob Storage | ||
|
||
You should be able to kick off a run of `all_assets_job`, and when you go to the stdout/stderr window of the run page, you should see a log file that directs you to the Azure Blob Storage container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's assuming that the user has faithfully followed previous steps without deviation and is using the quickstart_etl module. Might be a bit confusing to some.
Maybe some wording like, "It's time to kick off a run to test your logging setup. If your code locations still include the quickstart_etl module from step 2 then the all_assets_job
will be present, otherwise any job of your own that emits some long will do"
To complete the steps in this guide, you'll need: | ||
- The Azure CLI installed on your machine. You can download it [here](https://docs.microsoft.com/en-us/cli/azure/install-azure-cli). | ||
- An Azure account with the ability to create resources in Azure Blob Storage or Azure Data Lake Storage. | ||
- An Azure container in Azure Blob Storage or Azure Data Lake Storage where you want to store logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Either the
quickstart_etl
module or any other code location successfully imported containing at least one job or asset to materialize with some logging verbosity included
(see other comment about all_assets_job)
helm upgrade user-cloud dagster-cloud/dagster-cloud-agent -n dagster-agent -f current-values.yaml | ||
``` | ||
|
||
## Step 3: Verify logs are being written to Azure Blob Storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nice to include perhaps some troubleshooting steps as an addendum if the logs don't work.
Ex:
How would the user see if:
- the credentials are wrong
- the credentials do not have the necessary permissions
- the bucket does not exist or isn't writable?
Possibly we don't have a great story there, which would perhaps be follow up work we could add on later.
module: dagster_azure.blob.compute_log_manager | ||
class: AzureBlobComputeLogManager | ||
config: | ||
storage_account: chriscomplogmngr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might want to pick something else here
--identity-name agent-id \ | ||
--resource-group <resource-group> \ | ||
--issuer $(az aks show -g <resource-group> -n <aks-cluster-name> --query "oidcIssuerProfile.issuerUrl" -otsv) \ | ||
--subject system:serviceaccount:dagster-agent:dagster-agent-sa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think longer identifiers would make it more explicit and grokkable for the user.
agent-id -> agent-identity
dagster-agent-sa -> dagster-agent-service-account
Summary & Motivation
Add docs on using the azure blob storage compute log manager in AKS.
How I Tested These Changes
I followed these instructions myself to get it set up.